From 40ab9334edd208b6b757dce94379fafa33b5d3b5 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger Date: Mon, 6 Jul 2026 11:58:21 -0300 Subject: [PATCH 01/14] chore(sdk-review): initialize feature branch for AFSDK-3937 skill This branch collects incremental sub-PRs for the SDK Module Review skill. Sub-PRs target this branch; a final PR will merge everything into main. Batches (see docs/plans/AFSDK-3937/06-INCREMENTAL-DELIVERY.md): - batch-1: Foundations (libs + config) - batch-2: AST + detectors - batch-3: Safety-first checks (secrets, license, disclosure, hardcode, telemetry) - batch-4: Structural checks (docs, bdd, patterns, versioning, commits) - batch-5: Depth checks (testing, errors, http, concurrency, deletion, deps, constants, binding, quality-gate, pr-size) - batch-6: Wire-up (orchestrate, aggregate, workflow, docs, CONTRIBUTING amendments) Ref: AFSDK-3937 From 95fb0e58cfdc91a1102e68e35e92c785b1ff42fc Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger Date: Mon, 6 Jul 2026 11:59:31 -0300 Subject: [PATCH 02/14] =?UTF-8?q?feat(sdk-review):=20batch=201/6=20?= =?UTF-8?q?=E2=80=94=20shared=20libs=20+=20config?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the foundation for the SDK Module Review skill: ## Libs (.claude/scripts/lib/) - json-emit.sh — findings JSON emitter + status derivation - diff-added-lines.sh — extract file:line pairs from diff (BSD-safe) - baseline.sh — apply baseline.json exemptions to reports - predicates.sh — scope predicates (commit_types, module_shape, REUSE.toml) - suppression.sh — parse # sdk-review: ignore[] comments - apply-suppression.sh — filter reports by suppression tuples - github-api.sh — gh CLI wrapper (auth, comments, check-run, labels) - detect-language.sh — Python vs Java detection - detect-modules.sh — module extraction from diff - tier-manager.sh — apply SHADOW/FLAG/BLOCK/BLOCK_LOCKED tiers - skill-self-skip.sh — self-review protection (skip skill's own files) - ast_python_checks.py — Python AST checks (used by errors, telemetry, patterns, constants) - breaking-detector.py — API-diff detector for BREAKING family ## Config (.claude/config/) - rules.yaml — 70+ rules with tiers per rule - module-aliases.yaml — cross-language module name aliases (dms↔documentmanagement) - baseline.json — per-repo exemptions (REUSE.toml here → LIC-01/02 exempt) **Standalone testable.** No checks or orchestrator yet — comes in later batches. Part 1 of 6. Ref: AFSDK-3937 --- .claude/config/baseline.json | 25 + .claude/config/module-aliases.yaml | 5 + .claude/config/rules.yaml | 131 +++++ .claude/scripts/lib/apply-suppression.sh | 119 +++++ .claude/scripts/lib/ast_python_checks.py | 401 ++++++++++++++++ .claude/scripts/lib/baseline.sh | 82 ++++ .claude/scripts/lib/breaking-detector.py | 292 ++++++++++++ .claude/scripts/lib/detect-language.sh | 25 + .claude/scripts/lib/detect-modules.sh | 20 + .claude/scripts/lib/diff-added-lines.sh | 48 ++ .claude/scripts/lib/github-api.sh | 220 +++++++++ .claude/scripts/lib/json-emit.sh | 76 +++ .claude/scripts/lib/predicates.sh | 99 ++++ .claude/scripts/lib/skill-self-skip.sh | 19 + .claude/scripts/lib/suppression.sh | 84 ++++ .claude/scripts/lib/tier-manager.sh | 116 +++++ .../fixtures/binding-token-concat.diff | 7 + tests/sdk-review/fixtures/clean.diff | 9 + .../fixtures/disclosure-internal.diff | 7 + tests/sdk-review/fixtures/hardcode-url.diff | 8 + .../sdk-review/fixtures/secrets-aws-key.diff | 9 + tests/sdk-review/fixtures/secrets-jwt.diff | 8 + tests/sdk-review/test_extended.bats | 448 ++++++++++++++++++ tests/sdk-review/test_smoke.bats | 285 +++++++++++ 24 files changed, 2543 insertions(+) create mode 100644 .claude/config/baseline.json create mode 100644 .claude/config/module-aliases.yaml create mode 100644 .claude/config/rules.yaml create mode 100755 .claude/scripts/lib/apply-suppression.sh create mode 100755 .claude/scripts/lib/ast_python_checks.py create mode 100755 .claude/scripts/lib/baseline.sh create mode 100755 .claude/scripts/lib/breaking-detector.py create mode 100755 .claude/scripts/lib/detect-language.sh create mode 100755 .claude/scripts/lib/detect-modules.sh create mode 100755 .claude/scripts/lib/diff-added-lines.sh create mode 100755 .claude/scripts/lib/github-api.sh create mode 100755 .claude/scripts/lib/json-emit.sh create mode 100755 .claude/scripts/lib/predicates.sh create mode 100755 .claude/scripts/lib/skill-self-skip.sh create mode 100755 .claude/scripts/lib/suppression.sh create mode 100755 .claude/scripts/lib/tier-manager.sh create mode 100644 tests/sdk-review/fixtures/binding-token-concat.diff create mode 100644 tests/sdk-review/fixtures/clean.diff create mode 100644 tests/sdk-review/fixtures/disclosure-internal.diff create mode 100644 tests/sdk-review/fixtures/hardcode-url.diff create mode 100644 tests/sdk-review/fixtures/secrets-aws-key.diff create mode 100644 tests/sdk-review/fixtures/secrets-jwt.diff create mode 100644 tests/sdk-review/test_extended.bats create mode 100644 tests/sdk-review/test_smoke.bats diff --git a/.claude/config/baseline.json b/.claude/config/baseline.json new file mode 100644 index 00000000..45c15950 --- /dev/null +++ b/.claude/config/baseline.json @@ -0,0 +1,25 @@ +{ + "created_at": "2026-07-15T00:00:00Z", + "at_commit": "TBD", + "schema_version": 1, + "exemptions": { + "PY-LIC-01": { + "reason": "Repo uses REUSE.toml aggregate — pre-existing files exempt", + "detector": "REUSE.toml exists with path='**' aggregate annotation", + "scope": "repo", + "files_exempted_glob": "src/**/*.py" + }, + "PY-LIC-02": { + "reason": "Same as PY-LIC-01", + "scope": "repo" + }, + "JV-LIC-01": { + "reason": "cloud-sdk-java has no REUSE.toml AND no per-file SPDX baseline — rule kept SHADOW pending team decision on Wave-4 baseline PR", + "scope": "repo" + }, + "JV-LIC-02": { + "reason": "Same as JV-LIC-01", + "scope": "repo" + } + } +} diff --git a/.claude/config/module-aliases.yaml b/.claude/config/module-aliases.yaml new file mode 100644 index 00000000..c14ebbbb --- /dev/null +++ b/.claude/config/module-aliases.yaml @@ -0,0 +1,5 @@ +# Module name aliases across languages. +# Used by check-bdd.sh to resolve cross-repo feature file location. +aliases: + - python: dms + java: documentmanagement diff --git a/.claude/config/rules.yaml b/.claude/config/rules.yaml new file mode 100644 index 00000000..1428f0fa --- /dev/null +++ b/.claude/config/rules.yaml @@ -0,0 +1,131 @@ +version: 2 + +# ===================================================== +# rules.yaml — SDK Module Review rule catalog +# All rules default to their tier here; may be overridden per-repo. +# BLOCK_LOCKED rules cannot be softened or suppressed. +# ===================================================== + +baseline: + file: .claude/baseline.json + regenerate_with: orchestrate.sh --baseline + +tiers: + SHADOW: { posts: false, blocks: false } + FLAG: { posts: true, blocks: false } + BLOCK: { posts: true, blocks: true } + BLOCK_LOCKED: { posts: true, blocks: true, suppressible: false, downgradable: false } + +rules: + # -------- BREAKING FAMILY (all locked) -------- + BREAKING-01: { tier: BLOCK_LOCKED } + BREAKING-02: { tier: BLOCK_LOCKED } + BREAKING-03: { tier: FLAG } + BREAKING-04: { tier: FLAG } + + # -------- SECRETS (all locked) -------- + SEC-01: { tier: BLOCK_LOCKED } + SEC-02: { tier: BLOCK_LOCKED } + SEC-03: { tier: BLOCK_LOCKED } + SEC-04: { tier: BLOCK_LOCKED } + SEC-05: { tier: BLOCK_LOCKED } + SEC-06: { tier: BLOCK_LOCKED } + SEC-07: { tier: BLOCK_LOCKED } + SEC-08: { tier: BLOCK_LOCKED } + SEC-10: { tier: BLOCK_LOCKED } + + # -------- LICENSE -------- + LIC-01: { tier: BLOCK, predicates: { not: reuse_toml_aggregate_present, diff_scope: file_in_added_set } } + LIC-02: { tier: BLOCK, predicates: { not: reuse_toml_aggregate_present, diff_scope: file_in_added_set } } + + # -------- DISCLOSURE -------- + DIS-01: { tier: BLOCK, profile: { public: BLOCK, internal: FLAG } } + DIS-02: { tier: BLOCK, profile: { public: BLOCK, internal: PASS } } + DIS-06: { tier: BLOCK_LOCKED } + DIS-07: { tier: BLOCK } + DIS-08: { tier: SHADOW } + + # -------- HARDCODE -------- + HC-01: { tier: BLOCK } + HC-02: { tier: BLOCK } + HC-03: { tier: BLOCK_LOCKED } + HC-04: { tier: FLAG } + HC-06: { tier: FLAG } + + # -------- TELEMETRY -------- + PY-TEL-02: { tier: BLOCK } + JV-TEL-02: { tier: BLOCK } + PY-TEL-06: { tier: BLOCK, predicates: { diff_scope: new_decorator_added } } + PY-TEL-08: { tier: SHADOW } + + # -------- DOCS -------- + DC-01: { tier: BLOCK } + DC-02: { tier: BLOCK } + DC-11: { tier: BLOCK } + DC-12: { tier: BLOCK } + DC-13: { tier: BLOCK } + DC-14: { tier: BLOCK } + + # -------- BDD -------- + BDD-01: { tier: BLOCK } + BDD-02: { tier: BLOCK } + + # -------- PATTERNS -------- + PY-PT-01: { tier: BLOCK, predicates: { module_shape: client } } + JV-PT-01: { tier: BLOCK, predicates: { module_shape: client } } + PY-PT-03: { tier: FLAG } + PY-PT-04: { tier: FLAG } + PY-PT-08: { tier: FLAG } + JV-PT-05: { tier: FLAG } + + # -------- VERSIONING -------- + VER-01: { tier: BLOCK, predicates: { commit_types_or_breaking: [feat, feat!, breaking] } } + VER-07: { tier: FLAG } + PY-VER-08: { tier: SHADOW } + PY-VER-09: { tier: SHADOW } + + # -------- ERRORS/LOGGING -------- + EL-01: { tier: FLAG } + EL-02: { tier: FLAG } + EL-04: { tier: BLOCK } + PY-EL-11: { tier: SHADOW } + + # -------- TESTING -------- + TD-01: { tier: FLAG } + TD-10: { tier: BLOCK } + TD-checkbox: { tier: FLAG } + + # -------- HTTP -------- + HTTP-01: { tier: FLAG, predicates: { diff_scope: line_in_added_set } } + + # -------- CONCURRENCY -------- + CC-01: { tier: FLAG } + JV-CC-05: { tier: SHADOW } + + # -------- DEPS -------- + DEP-03: { tier: FLAG } + DEP-04: { tier: BLOCK_LOCKED } + + # -------- COMMITS -------- + COM-01: { tier: BLOCK } + + # -------- DELETION -------- + DEL-01: { tier: BLOCK } + + # -------- CONSTANTS -------- + PY-CON-01: { tier: FLAG } + + # -------- BINDING -------- + BND-02: { tier: BLOCK_LOCKED } + BND-04: { tier: BLOCK } + BND-05: { tier: BLOCK_LOCKED } + + # -------- QUALITY GATE -------- + QG-01: { tier: FLAG } + QG-02: { tier: FLAG } + + # -------- PR SIZE (advisory, SHADOW during rollout) -------- + PR-SIZE-01: { tier: SHADOW } + PR-SIZE-02: { tier: SHADOW } + PR-SIZE-03: { tier: SHADOW } + PR-SIZE-05: { tier: SHADOW } diff --git a/.claude/scripts/lib/apply-suppression.sh b/.claude/scripts/lib/apply-suppression.sh new file mode 100755 index 00000000..e21d8ad5 --- /dev/null +++ b/.claude/scripts/lib/apply-suppression.sh @@ -0,0 +1,119 @@ +#!/usr/bin/env bash +# lib/apply-suppression.sh — filter findings against suppression comments in source. +# Called after a check produces its JSON report; reads suppression tuples and drops findings. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# Source suppression.sh so we don't fork bash+jq per finding. +# shellcheck source=./suppression.sh +source "$SCRIPT_DIR/suppression.sh" + +# collect_suppressions_for_files ... +# Emits "::" tuples for all suppression comments found. +collect_suppressions_for_files() { + for f in "$@"; do + [ -f "$f" ] || continue + parse_line_suppressions "$f" 2>/dev/null || true + parse_file_suppressions "$f" 2>/dev/null || true + done +} + +# apply_to_report → prints filtered report +apply_to_report() { + local report="$1" supp_file="$2" + local suppressed_count=0 + local input; input=$(cat "$report") + local n; n=$(echo "$input" | jq '.findings | length') + if [ "$n" -eq 0 ]; then + echo "$input"; return + fi + + # Extract all (rule, file, line) tuples in a single jq call — one fork total + # instead of one per finding. + local tuples; tuples=$(echo "$input" | jq -r '.findings[] | "\(.rule)|\(.file)|\(.line)"') + + local keep_flags=() + local i=0 + while IFS='|' read -r rule file line; do + if [ "$(is_suppressed "$rule" "$file" "$line" "$supp_file")" = "true" ]; then + keep_flags+=("0") + suppressed_count=$((suppressed_count + 1)) + else + keep_flags+=("1") + fi + i=$((i + 1)) + done <<< "$tuples" + + # Build a JSON array of booleans and use jq to filter + local keep_json + if [ "${#keep_flags[@]}" -eq 0 ]; then + keep_json="[]" + else + keep_json=$(printf '%s\n' "${keep_flags[@]}" | jq -s 'map(. == "1")') + fi + + echo "$input" | jq \ + --argjson keep "$keep_json" \ + --argjson suppressed "$suppressed_count" \ + '.findings = [.findings[] as $f | ($f | . as $x | (input_line_number - 1) as $i | select($keep[$i]))] // .findings + | .findings = [range(.findings | length) as $i | .findings[$i] | select($keep[$i])] + | .summary.suppressed_count = $suppressed' 2>/dev/null || { + # Fallback: simple length-preserving filter + echo "$input" | jq \ + --argjson keep "$keep_json" \ + --argjson suppressed "$suppressed_count" \ + '(.findings | length) as $n + | .findings = [range($n) | . as $i | (($keep[$i] // true) | if . then $i else null end) | select(. != null)] as $indices + | .findings = [$indices[] as $i | .findings[$i]] + | .summary.suppressed_count = $suppressed' + } +} + +# Simpler variant: build kept-list explicitly (safe fallback) +apply_to_report_v2() { + local report="$1" supp_file="$2" + local suppressed_count=0 + local input; input=$(cat "$report") + local n; n=$(echo "$input" | jq '.findings | length') + if [ "$n" -eq 0 ]; then + echo "$input"; return + fi + + local kept=() + local i=0 + while [ "$i" -lt "$n" ]; do + local finding rule file line + finding=$(echo "$input" | jq -c ".findings[$i]") + rule=$(echo "$finding" | jq -r '.rule') + file=$(echo "$finding" | jq -r '.file') + line=$(echo "$finding" | jq -r '.line') + + if [ "$(is_suppressed "$rule" "$file" "$line" "$supp_file")" = "true" ]; then + suppressed_count=$((suppressed_count + 1)) + else + kept+=("$finding") + fi + i=$((i + 1)) + done + + local kept_json + if [ "${#kept[@]}" -eq 0 ]; then + kept_json="[]" + else + kept_json=$(printf '%s\n' "${kept[@]}" | jq -s -c '.') + fi + + echo "$input" | jq \ + --argjson kept "$kept_json" \ + --argjson suppressed "$suppressed_count" \ + '.findings = $kept | .summary.suppressed_count = $suppressed' +} + +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + cmd="${1:-}"; shift || true + case "$cmd" in + collect) collect_suppressions_for_files "$@" ;; + apply) apply_to_report_v2 "$@" ;; # simpler + safer + *) echo "Usage: apply-suppression.sh {collect|apply} args" >&2; exit 2 ;; + esac +fi diff --git a/.claude/scripts/lib/ast_python_checks.py b/.claude/scripts/lib/ast_python_checks.py new file mode 100755 index 00000000..57172714 --- /dev/null +++ b/.claude/scripts/lib/ast_python_checks.py @@ -0,0 +1,401 @@ +#!/usr/bin/env python3 +"""AST-based checks for Python source files. + +Called from bash check-*.sh scripts. Reads source files, emits JSONL findings +to stdout (one JSON object per line). + +Usage: + python3 ast-python-checks.py [ ...] + +Where is one of: + el-01, el-02 exception chaining / swallow + tel-02, tel-06 telemetry decorators / tests + pt-01, pt-08, pt-11 patterns + con-01 repeated string literals +""" + +from __future__ import annotations + +import ast +import json +import sys +from pathlib import Path + + +def emit( + rule: str, severity: str, file: str, line: int, message: str, suggestion: str = "" +) -> None: + print( + json.dumps( + { + "rule": rule, + "severity": severity, + "file": file, + "line": line, + "message": message, + "suggestion": suggestion, + } + ) + ) + + +def parse_file(path: str) -> ast.Module | None: + try: + source = Path(path).read_text(encoding="utf-8") + return ast.parse(source, filename=path) + except (SyntaxError, UnicodeDecodeError, FileNotFoundError): + return None + + +# ---------- PY-EL-01: raise X from e when raising DIFFERENT exception ---------- + + +def check_el_01(path: str, tree: ast.Module) -> None: + for node in ast.walk(tree): + if not isinstance(node, ast.ExceptHandler): + continue + handler_name = node.name # the caught exception variable, may be None + for inner in ast.walk(node): + if not isinstance(inner, ast.Raise): + continue + # bare `raise` — PASS (preserves cause) + if inner.exc is None: + continue + # `raise e` where e is the handler var — PASS (same exception) + if ( + isinstance(inner.exc, ast.Name) + and handler_name + and inner.exc.id == handler_name + ): + continue + # raising a *call* to the same var, e.g. `raise e()` (rare) — PASS + if ( + isinstance(inner.exc, ast.Call) + and isinstance(inner.exc.func, ast.Name) + and handler_name + and inner.exc.func.id == handler_name + ): + continue + # different exception without `from e` → FLAG + if inner.cause is None: + emit( + "PY-EL-01", + "FLAG", + path, + inner.lineno, + "Raising different exception inside except — use `raise X from e` to preserve cause", + ) + + +# ---------- PY-EL-02: except body must end with raise or explicit suppression ---------- + + +def check_el_02(path: str, tree: ast.Module) -> None: + for node in ast.walk(tree): + if not isinstance(node, ast.ExceptHandler): + continue + if not node.body: + continue + last = node.body[-1] + # last statement is Raise → PASS + if isinstance(last, ast.Raise): + continue + # last statement is Return of a variable named e / err / exc → likely intentional propagation → PASS + if ( + isinstance(last, ast.Return) + and isinstance(last.value, ast.Name) + and last.value.id in {"e", "err", "exc"} + ): + continue + # explicit `pass` and only `pass` in body → obvious swallow → FLAG + emit( + "PY-EL-02", + "FLAG", + path, + node.lineno, + "except block does not end with `raise` — swallowing? Add re-raise or comment justifying suppression", + ) + + +# ---------- PY-TEL-02: public *Client methods have @record_metrics ---------- + + +def check_tel_02(path: str, tree: ast.Module) -> None: + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + if not node.name.endswith("Client"): + continue + for item in node.body: + if not isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if item.name.startswith("_"): + continue + has_record = any( + ( + isinstance(d, ast.Call) + and isinstance(d.func, ast.Name) + and d.func.id == "record_metrics" + ) + or (isinstance(d, ast.Name) and d.id == "record_metrics") + for d in item.decorator_list + ) + if not has_record: + emit( + "PY-TEL-02", + "BLOCK", + path, + item.lineno, + f"Public method `{node.name}.{item.name}` missing @record_metrics decorator", + ) + + +# ---------- PY-TEL-06: test files that assert record_request_metric calls ---------- + + +def check_tel_06_test_asserts(path: str, tree: ast.Module) -> bool: + """Returns True if the test file asserts record_request_metric was called.""" + src = ast.dump(tree) + return "record_request_metric" in src + + +# ---------- PY-PT-08: public functions/methods have type hints ---------- + + +def check_pt_08(path: str, tree: ast.Module) -> None: + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + # skip private functions + if node.name.startswith("_") and node.name != "__init__": + continue + # skip @classmethod first-arg conventionally named "cls" + missing: list[str] = [] + # check return annotation + if node.returns is None and node.name != "__init__": + missing.append("return type") + # check arg annotations (excluding self/cls) + for arg in node.args.args: + if arg.arg in {"self", "cls"}: + continue + if arg.annotation is None: + missing.append(f"param `{arg.arg}`") + # also check keyword-only, positional-only, and vararg annotations + for arg in getattr(node.args, "kwonlyargs", []): + if arg.annotation is None: + missing.append(f"keyword-only `{arg.arg}`") + for arg in getattr(node.args, "posonlyargs", []): + if arg.arg in {"self", "cls"}: + continue + if arg.annotation is None: + missing.append(f"positional-only `{arg.arg}`") + if node.args.vararg and node.args.vararg.annotation is None: + missing.append(f"*{node.args.vararg.arg}") + if node.args.kwarg and node.args.kwarg.annotation is None: + missing.append(f"**{node.args.kwarg.arg}") + + # Emit one finding per function (not per arg) so downstream isn't spammed + if missing: + joined = ", ".join(missing) + emit( + "PY-PT-08", + "FLAG", + path, + node.lineno, + f"Public function `{node.name}` missing type annotations: {joined}", + ) + + +# ---------- PY-CON-01: repeated string literals ---------- + + +def check_con_01( + path: str, tree: ast.Module, threshold: int = 3, min_len: int = 4 +) -> None: + # Skip prefixes: URLs, test/example placeholders, SPDX/doc markers. + # Must be exact URL scheme match — not `httpx` or `http_pool`. + URL_PREFIXES = ("http://", "https://", "ftp://", "sftp://", "ssh://", "file://") + SKIP_PREFIXES = ("test-", "SPDX", "@example") + counts: dict[str, list[int]] = {} + for node in ast.walk(tree): + if isinstance(node, ast.Constant) and isinstance(node.value, str): + v = node.value + if len(v) < min_len: + continue + if v.startswith(URL_PREFIXES): + continue + if v.startswith(SKIP_PREFIXES): + continue + counts.setdefault(v, []).append(node.lineno) + for literal, lines in counts.items(): + if len(lines) >= threshold: + emit( + "PY-CON-01", + "FLAG", + path, + lines[0], + f"String literal {literal!r} appears {len(lines)}× — extract module-level constant", + suggestion=f"e.g., _CONSTANT_NAME = {literal!r}", + ) + + +# ---------- PY-PT-01: create_client factory exists ---------- + + +def check_pt_01(module_dir: str) -> bool: + """Check if module has create_client() function (returns True if found).""" + p = Path(module_dir) + if not p.is_dir(): + return False + # scan module-level defs across all .py files + for py in p.glob("*.py"): + if py.name.startswith("_") and py.name != "__init__.py": + continue + tree = parse_file(str(py)) + if tree is None: + continue + for node in tree.body: + if ( + isinstance(node, ast.FunctionDef) + and node.name.startswith("create_") + and node.name.endswith("client") + ): + return True + return False + + +# ---------- __all__ extraction (used by breaking-detector.py) ---------- + + +def extract_all(tree: ast.Module) -> list[str]: + """Extract the __all__ list from a module AST.""" + for node in tree.body: + if not isinstance(node, ast.Assign): + continue + for target in node.targets: + if isinstance(target, ast.Name) and target.id == "__all__": + if isinstance(node.value, (ast.List, ast.Tuple)): + return [ + elt.value + for elt in node.value.elts + if isinstance(elt, ast.Constant) and isinstance(elt.value, str) + ] + return [] + + +def extract_public_class_methods( + tree: ast.Module, +) -> dict[str, dict[str, tuple[list[str], list[str], list[str], str | None]]]: + """Return {ClassName: {method_name: (positional_args, kwonly_args, vararg_kwarg_flags, return_ann)}}. + + positional_args: names of positional args (self/cls excluded) + kwonly_args: names of keyword-only args + vararg_kwarg_flags: ["*args", "**kwargs"] if present (for tracking their addition/removal) + return_ann: repr of return annotation, or None + """ + result: dict[str, dict] = {} + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + if node.name.startswith("_"): + continue + methods: dict = {} + for item in node.body: + if not isinstance(item, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if item.name.startswith("_") and item.name != "__init__": + continue + pos_args = [a.arg for a in item.args.args if a.arg not in {"self", "cls"}] + kwonly = [a.arg for a in item.args.kwonlyargs] + varflags: list[str] = [] + if item.args.vararg: + varflags.append(f"*{item.args.vararg.arg}") + if item.args.kwarg: + varflags.append(f"**{item.args.kwarg.arg}") + ret = ast.unparse(item.returns) if item.returns else None + methods[item.name] = (pos_args, kwonly, varflags, ret) + if methods: + result[node.name] = methods + return result + + +def extract_dataclass_fields(tree: ast.Module) -> dict[str, list[str]]: + """Return {DataclassName: [field_names]} for classes decorated with @dataclass + or @dataclasses.dataclass.""" + result: dict[str, list[str]] = {} + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + is_dc = False + for d in node.decorator_list: + # bare @dataclass + if isinstance(d, ast.Name) and d.id == "dataclass": + is_dc = True + break + # @dataclass(frozen=True) or @dataclass() + if ( + isinstance(d, ast.Call) + and isinstance(d.func, ast.Name) + and d.func.id == "dataclass" + ): + is_dc = True + break + # @dataclasses.dataclass + if isinstance(d, ast.Attribute) and d.attr == "dataclass": + is_dc = True + break + # @dataclasses.dataclass(...) + if ( + isinstance(d, ast.Call) + and isinstance(d.func, ast.Attribute) + and d.func.attr == "dataclass" + ): + is_dc = True + break + if not is_dc: + continue + fields: list[str] = [] + for item in node.body: + if isinstance(item, ast.AnnAssign) and isinstance(item.target, ast.Name): + fields.append(item.target.id) + if fields: + result[node.name] = fields + return result + + +# ---------- dispatcher ---------- + +CHECKS = { + "el-01": check_el_01, + "el-02": check_el_02, + "tel-02": check_tel_02, + "pt-08": check_pt_08, + "con-01": check_con_01, +} + + +def main(argv: list[str]) -> int: + if len(argv) < 3: + print( + "Usage: ast-python-checks.py [ ...]", + file=sys.stderr, + ) + return 2 + check_name = argv[1] + files = argv[2:] + + if check_name not in CHECKS: + print(f"ERROR: unknown check {check_name}", file=sys.stderr) + return 2 + + fn = CHECKS[check_name] + for f in files: + tree = parse_file(f) + if tree is None: + continue + fn(f, tree) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/.claude/scripts/lib/baseline.sh b/.claude/scripts/lib/baseline.sh new file mode 100755 index 00000000..181349eb --- /dev/null +++ b/.claude/scripts/lib/baseline.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# lib/baseline.sh — Apply baseline exemptions to a findings JSON. +# Reads baseline.json + a check-report JSON, outputs the filtered report. +set -euo pipefail + +# is_exempted rule file baseline_json → prints "true" | "false" +# Baseline schema: {exemptions: {"": {scope: "repo|module:|file", files_exempted_glob: "", file_snapshot: {...}}}} +is_exempted() { + local rule="$1" file="$2" baseline="$3" + local entry; entry=$(echo "$baseline" | jq -r --arg rule "$rule" '.exemptions[$rule] // empty') + if [ -z "$entry" ]; then echo "false"; return; fi + + local scope glob + scope=$(echo "$entry" | jq -r '.scope // "repo"') + glob=$(echo "$entry" | jq -r '.files_exempted_glob // "**"') + + case "$scope" in + repo) + echo "true" + ;; + file) + # only exempt if file is in file_snapshot AND has not regressed (LOC check) + local snapshot; snapshot=$(echo "$entry" | jq -r --arg f "$file" '.file_snapshot[$f] // empty') + if [ -n "$snapshot" ]; then echo "true"; else echo "false"; fi + ;; + module:*) + local mod="${scope#module:}" + # naive: match path contains "/$mod/" or starts with "$mod/" + if [[ "$file" == *"/${mod}/"* || "$file" == "${mod}/"* ]]; then + echo "true" + else + echo "false" + fi + ;; + *) + echo "false" + ;; + esac +} + +# apply_baseline_to_report baseline_file report_file → prints filtered report to stdout, +# suppressed count to stderr +apply_baseline_to_report() { + local baseline_file="$1" report_file="$2" + local baseline; baseline=$(cat "$baseline_file" 2>/dev/null || echo '{"exemptions":{}}') + + local report; report=$(cat "$report_file") + local kept=() suppressed=0 + + local n; n=$(echo "$report" | jq '.findings | length') + local i=0 + while [ "$i" -lt "$n" ]; do + local finding rule file exempt + finding=$(echo "$report" | jq -c ".findings[$i]") + rule=$(echo "$finding" | jq -r '.rule') + file=$(echo "$finding" | jq -r '.file') + exempt=$(is_exempted "$rule" "$file" "$baseline") + if [ "$exempt" = "true" ]; then + suppressed=$((suppressed + 1)) + else + kept+=("$finding") + fi + i=$((i + 1)) + done + + # Guard against empty array under set -u; produce "[]" not "[\"\"]" + local kept_json + if [ "${#kept[@]}" -eq 0 ]; then + kept_json="[]" + else + kept_json=$(printf '%s\n' "${kept[@]}" | jq -s -c '.') + fi + + echo "$report" | jq --argjson kept "$kept_json" \ + --argjson suppressed "$suppressed" \ + '.findings = $kept | .summary.suppressed_by_baseline = $suppressed' +} + +# When sourced, expose functions. When executed, run apply_baseline_to_report. +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + apply_baseline_to_report "$@" +fi diff --git a/.claude/scripts/lib/breaking-detector.py b/.claude/scripts/lib/breaking-detector.py new file mode 100755 index 00000000..47139806 --- /dev/null +++ b/.claude/scripts/lib/breaking-detector.py @@ -0,0 +1,292 @@ +#!/usr/bin/env python3 +"""Breaking-change detector. + +Compares AST of base commit vs HEAD to find: + - api_removal_detected removed from __all__ + - method_deletion_on_public_class + - dataclass_field_deletion + - public_method_signature_change + - enum_value_deletion + - exception_hierarchy_change + +Output: JSON on stdout: +{ + "breaking_detected": true|false, + "kinds": [...], + "details": [{"kind": "...", "symbol": "...", "file": "...", "line": N}, ...] +} +""" + +from __future__ import annotations + +import ast +import json +import subprocess +import sys +from pathlib import Path + +# Import from same directory +sys.path.insert(0, str(Path(__file__).parent)) +from ast_python_checks import ( + extract_all, + extract_public_class_methods, + extract_dataclass_fields, +) # noqa: E402 + + +def git_show(sha: str, path: str) -> str | None: + """Read file content at a given commit. Returns None if not available.""" + # Try direct git show first + try: + out = subprocess.run( + ["git", "show", f"{sha}:{path}"], + capture_output=True, + text=True, + check=False, + ) + if out.returncode == 0: + return out.stdout + except Exception: + pass + # Fallback: try to fetch the object first if it's missing + try: + subprocess.run( + ["git", "fetch", "--quiet", "origin", sha], + capture_output=True, + text=True, + check=False, + timeout=10, + ) + out = subprocess.run( + ["git", "show", f"{sha}:{path}"], + capture_output=True, + text=True, + check=False, + ) + if out.returncode == 0: + return out.stdout + except Exception: + pass + return None + + +def read_head_file(path: str) -> str | None: + """Read file content from HEAD (working tree).""" + try: + return Path(path).read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + return None + + +def parse_source(source: str, filename: str = "") -> ast.Module | None: + try: + return ast.parse(source, filename=filename) + except SyntaxError: + return None + + +def files_changed(base: str, head: str) -> list[str]: + out = subprocess.run( + ["git", "diff", "--name-only", f"{base}..{head}", "--", "*.py"], + capture_output=True, + text=True, + check=False, + ) + return [f for f in out.stdout.strip().split("\n") if f] + + +def detect(base: str, head: str) -> dict: + details: list[dict] = [] + kinds: set[str] = set() + + changed = files_changed(base, head) + + for file in changed: + base_src = git_show(base, file) + head_src = git_show(head, file) if head != "HEAD" else read_head_file(file) + if head_src is None and Path(file).exists(): + head_src = read_head_file(file) + if head_src is None: + # file deleted + if base_src: + base_tree = parse_source(base_src, file) + if base_tree: + for name in extract_all(base_tree): + details.append( + { + "kind": "api_removal_detected", + "symbol": name, + "file": file, + "line": 0, + } + ) + kinds.add("api_removal_detected") + continue + + base_tree = parse_source(base_src, file) if base_src else None + head_tree = parse_source(head_src, file) + if base_tree is None or head_tree is None: + continue + + # __all__ removals + base_all = set(extract_all(base_tree)) + head_all = set(extract_all(head_tree)) + removed = base_all - head_all + for name in removed: + details.append( + { + "kind": "api_removal_detected", + "symbol": name, + "file": file, + "line": 0, + } + ) + kinds.add("api_removal_detected") + + # public class method removals + signature changes + base_classes = extract_public_class_methods(base_tree) + head_classes = extract_public_class_methods(head_tree) + for cls_name, base_methods in base_classes.items(): + head_methods = head_classes.get(cls_name, {}) + for mname, base_sig in base_methods.items(): + if mname not in head_methods: + details.append( + { + "kind": "method_deletion_on_public_class", + "symbol": f"{cls_name}.{mname}", + "file": file, + "line": 0, + } + ) + kinds.add("method_deletion_on_public_class") + else: + head_sig = head_methods[mname] + # Compare all four elements of the signature tuple: + # (pos_args, kwonly_args, vararg_flags, return_annotation) + if base_sig != head_sig: + details.append( + { + "kind": "public_method_signature_change", + "symbol": f"{cls_name}.{mname}", + "file": file, + "line": 0, + "base_sig": { + "pos_args": base_sig[0], + "kwonly_args": base_sig[1], + "var_flags": base_sig[2], + "return": base_sig[3], + }, + "head_sig": { + "pos_args": head_sig[0], + "kwonly_args": head_sig[1], + "var_flags": head_sig[2], + "return": head_sig[3], + }, + } + ) + kinds.add("public_method_signature_change") + + # dataclass field removals + base_dcs = extract_dataclass_fields(base_tree) + head_dcs = extract_dataclass_fields(head_tree) + for dc_name, base_fields in base_dcs.items(): + head_fields = set(head_dcs.get(dc_name, [])) + for field in base_fields: + if field not in head_fields: + details.append( + { + "kind": "dataclass_field_deletion_on_public_model", + "symbol": f"{dc_name}.{field}", + "file": file, + "line": 0, + } + ) + kinds.add("dataclass_field_deletion_on_public_model") + + # enum value removals (best-effort: str Enum classes) + base_enums = _extract_enums(base_tree) + head_enums = _extract_enums(head_tree) + for enum_name, base_values in base_enums.items(): + head_values = set(head_enums.get(enum_name, [])) + for val in base_values: + if val not in head_values: + details.append( + { + "kind": "enum_value_deletion_on_public_enum", + "symbol": f"{enum_name}.{val}", + "file": file, + "line": 0, + } + ) + kinds.add("enum_value_deletion_on_public_enum") + + return { + "breaking_detected": bool(kinds), + "kinds": sorted(kinds), + "details": details, + } + + +def _extract_enums(tree: ast.Module) -> dict[str, list[str]]: + """Extract enum classes and their values. + + Detects classes whose base is exactly Enum, IntEnum, StrEnum, Flag, IntFlag, + or the same via attribute access (enum.Enum, etc.). Rejects unrelated names + that happen to contain "Enum" (e.g. NotAnEnum, MyEnumWrapper). + """ + ENUM_BASE_NAMES = {"Enum", "IntEnum", "StrEnum", "Flag", "IntFlag"} + result: dict[str, list[str]] = {} + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + if node.name.startswith("_"): + continue + is_enum = False + for b in node.bases: + # bare Name: e.g. class Status(Enum) + if isinstance(b, ast.Name) and b.id in ENUM_BASE_NAMES: + is_enum = True + break + # Attribute: e.g. class Status(enum.Enum) + if isinstance(b, ast.Attribute) and b.attr in ENUM_BASE_NAMES: + is_enum = True + break + if not is_enum: + continue + values: list[str] = [] + for item in node.body: + # Simple: RED = 1 + if isinstance(item, ast.Assign): + for tgt in item.targets: + if isinstance(tgt, ast.Name): + values.append(tgt.id) + # Annotated: RED: int = 1 + elif isinstance(item, ast.AnnAssign) and isinstance(item.target, ast.Name): + values.append(item.target.id) + result[node.name] = values + return result + + +def main(argv: list[str]) -> int: + if len(argv) < 3: + # default to comparing merge-base of main..HEAD + base = ( + subprocess.run( + ["git", "merge-base", "origin/main", "HEAD"], + capture_output=True, + text=True, + check=False, + ).stdout.strip() + or "HEAD~1" + ) + head = "HEAD" + else: + base, head = argv[1], argv[2] + + result = detect(base, head) + print(json.dumps(result, indent=2)) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/.claude/scripts/lib/detect-language.sh b/.claude/scripts/lib/detect-language.sh new file mode 100755 index 00000000..6cac757a --- /dev/null +++ b/.claude/scripts/lib/detect-language.sh @@ -0,0 +1,25 @@ +#!/usr/bin/env bash +# lib/detect-language.sh — detect Python vs Java repo shape. +set -euo pipefail + +detect_language() { + local root="${1:-.}" + if [ -f "$root/pyproject.toml" ] && [ -f "$root/pom.xml" ]; then + echo "ERROR: both pyproject.toml AND pom.xml found — cannot auto-detect" >&2 + exit 3 + fi + if [ -f "$root/pyproject.toml" ]; then + # Require the SDK package layout for a positive Python result + if [ -d "$root/src/sap_cloud_sdk" ]; then + echo "python"; return + fi + fi + if [ -f "$root/pom.xml" ]; then + if [ -d "$root/src/main/java" ]; then + echo "java"; return + fi + fi + echo "unknown" +} + +detect_language "$@" diff --git a/.claude/scripts/lib/detect-modules.sh b/.claude/scripts/lib/detect-modules.sh new file mode 100755 index 00000000..9221fcde --- /dev/null +++ b/.claude/scripts/lib/detect-modules.sh @@ -0,0 +1,20 @@ +#!/usr/bin/env bash +# lib/detect-modules.sh — extract module names from a diff. +# Usage: detect-modules.sh [diff-file] +# Reads diff from stdin if no diff-file argument. +set -euo pipefail + +lang="${1:-python}" +diff_source="${2:-/dev/stdin}" + +if [ "$lang" = "python" ]; then + grep -oE '(src/sap_cloud_sdk|src/)[a-z_][a-z_0-9]*/' "$diff_source" 2>/dev/null | \ + sed -E 's|src/sap_cloud_sdk/||; s|src/||; s|/$||' | \ + grep -v '^core$' | grep -v '^[[:space:]]*$' | sort -u +elif [ "$lang" = "java" ]; then + grep -oE 'src/main/java/com/sap/cloud/sdk/[a-z_][a-z_0-9]*/' "$diff_source" 2>/dev/null | \ + sed 's|src/main/java/com/sap/cloud/sdk/||; s|/$||' | \ + grep -v '^core$' | sort -u +else + echo "ERROR: unknown language $lang" >&2; exit 2 +fi diff --git a/.claude/scripts/lib/diff-added-lines.sh b/.claude/scripts/lib/diff-added-lines.sh new file mode 100755 index 00000000..651a5dea --- /dev/null +++ b/.claude/scripts/lib/diff-added-lines.sh @@ -0,0 +1,48 @@ +#!/usr/bin/env bash +# lib/diff-added-lines.sh — Emit file:line pairs that are in the added set (+) of a diff. +# Reads unified diff from stdin, outputs "path:linenumber" one per line (right-side lines only). +# Automatically excludes the skill's own files (self-review protection). +set -euo pipefail + +awk ' + BEGIN { file=""; line=0; skip=0 } + # Parse the "+++ b/" header — this is authoritative and handles filenames + # with spaces, unlike splitting the "diff --git a/... b/..." line on whitespace. + /^\+\+\+ b\// { + # Everything after "+++ b/" is the path (may contain spaces) + file = substr($0, 7) + line = 0 + # Self-review protection: skip skill files + skip = 0 + if (file ~ /^\.claude\//) skip = 1 + if (file ~ /^tests\/sdk-review\//) skip = 1 + if (file ~ /^\.github\/workflows\/sdk-/) skip = 1 + if (file ~ /^docs\/PR-REVIEW\.md$/) skip = 1 + if (file ~ /^docs\/BRANCH-PROTECTION-SETUP\.md$/) skip = 1 + next + } + /^\+\+\+ \/dev\/null/ { file = ""; next } # File being deleted + /^--- / { next } + /^diff --git/ { next } # Ignored; +++ b/... below carries the real path + /^@@/ { + # hunk header: @@ -old,+new,newcount @@ + if (match($0, /\+[0-9]+/)) { + line = substr($0, RSTART+1, RLENGTH-1) + 0 + } + next + } + /^\+/ && !/^\+\+\+/ { + if (!skip && file != "" && line > 0) print file ":" line + line++ + next + } + /^-/ && !/^---/ { + # deletion line — do not increment right-side counter + next + } + /^ / { + # context line — increment right-side counter but do not print + line++ + next + } +' "$@" diff --git a/.claude/scripts/lib/github-api.sh b/.claude/scripts/lib/github-api.sh new file mode 100755 index 00000000..ff7cca4c --- /dev/null +++ b/.claude/scripts/lib/github-api.sh @@ -0,0 +1,220 @@ +#!/usr/bin/env bash +# lib/github-api.sh — gh CLI wrapper for review posting. +set -euo pipefail + +# Use plain vars (not readonly) so the file can be sourced multiple times +# without hitting "readonly variable" under set -e. +SDK_REVIEW_MARKER_PREFIX="${SDK_REVIEW_MARKER_PREFIX:-}" + +# check_gh_auth → exit 0 if authed, exit 2 with message otherwise +check_gh_auth() { + local hostname="${1:-github.com}" + if ! gh auth status --hostname "$hostname" > /dev/null 2>&1; then + echo "ERROR: gh CLI not authenticated for $hostname. Run: gh auth login --hostname $hostname" >&2 + exit 2 + fi +} + +# detect_hostname → prints "github.com" or "github.tools.sap" from git remote +detect_hostname() { + local remote_url + remote_url=$(git remote get-url origin 2>/dev/null || echo "") + case "$remote_url" in + *github.tools.sap*) echo "github.tools.sap" ;; + *github.com*) echo "github.com" ;; + *) echo "github.com" ;; + esac +} + +# detect_owner_repo → prints "owner/repo" from git remote +# Handles all remote formats: +# https://github.com/OWNER/REPO.git +# https://user@github.com/OWNER/REPO.git +# https://token@github.tools.sap/OWNER/REPO.git +# git@github.com:OWNER/REPO.git +# ssh://git@github.com/OWNER/REPO.git +detect_owner_repo() { + local remote_url + remote_url=$(git remote get-url origin 2>/dev/null || echo "") + [ -z "$remote_url" ] && { echo ""; return; } + + # Strip trailing .git if present + remote_url="${remote_url%.git}" + + # SSH format: git@host:owner/repo + if [[ "$remote_url" == *"@"*":"* && "$remote_url" != *"://"* ]]; then + # cut everything up to and including ':' + remote_url="${remote_url#*:}" + echo "$remote_url" + return + fi + + # HTTP(S)/SSH-with-protocol: strip up to first '/' after host + # First strip the protocol + remote_url="${remote_url#*://}" + # Strip optional user@ prefix + remote_url="${remote_url#*@}" + # Now format is: host/owner/repo — strip first component (host) + remote_url="${remote_url#*/}" + echo "$remote_url" +} + +# get_pr_head_sha → prints the head SHA +get_pr_head_sha() { + local pr="$1" + gh pr view "$pr" --json headRefOid -q .headRefOid +} + +# list_bot_review_comments → outputs "id\tbody_marker" for each review-comment we've posted +list_bot_review_comments() { + local pr="$1" + local owner_repo; owner_repo=$(detect_owner_repo) + gh api "repos/${owner_repo}/pulls/${pr}/comments" --paginate 2>/dev/null | \ + jq -r --arg m "$SDK_REVIEW_MARKER_PREFIX" '.[] | select(.body | contains($m)) | "\(.id)\t\(.body[0:120])"' || true +} + +# list_bot_issue_comments → outputs "id" for each issue-comment (summary) we've posted +list_bot_issue_comments() { + local pr="$1" + local owner_repo; owner_repo=$(detect_owner_repo) + gh api "repos/${owner_repo}/issues/${pr}/comments" --paginate 2>/dev/null | \ + jq -r --arg m "$SUMMARY_MARKER" '.[] | select(.body | contains($m)) | .id' || true +} + +# delete_prior_bot_artifacts — idempotency: remove all sdk-review:v1 comments before posting new +delete_prior_bot_artifacts() { + local pr="$1" + local owner_repo; owner_repo=$(detect_owner_repo) + + # review-comments (inline) + list_bot_review_comments "$pr" | awk -F'\t' '{print $1}' | while read -r id; do + [ -n "$id" ] && gh api -X DELETE "repos/${owner_repo}/pulls/comments/${id}" > /dev/null 2>&1 || true + done + + # issue-comments (summary) + list_bot_issue_comments "$pr" | while read -r id; do + [ -n "$id" ] && gh api -X DELETE "repos/${owner_repo}/issues/comments/${id}" > /dev/null 2>&1 || true + done +} + +# is_fork_pr → prints "true" or "false" +# Fork PRs receive a read-only GITHUB_TOKEN in CI regardless of workflow permissions. +is_fork_pr() { + local pr="$1" + local head_repo + head_repo=$(gh pr view "$pr" --json headRepositoryOwner,isCrossRepository -q '.isCrossRepository' 2>/dev/null || echo "false") + echo "$head_repo" +} + +# post_inline_comment +# Silently skips on fork PRs (no write access). +post_inline_comment() { + local pr="$1" sha="$2" path="$3" line="$4" body="$5" + local owner_repo; owner_repo=$(detect_owner_repo) + # HTTP 422 means "line not in diff" — retry as issue comment. + # Any other failure (403 fork PR, network) is silent. + local http_code + http_code=$(gh api "repos/${owner_repo}/pulls/${pr}/comments" \ + -F body="$body" \ + -F commit_id="$sha" \ + -F path="$path" \ + -F line="$line" \ + -F side="RIGHT" --include 2>&1 | head -1 | awk '{print $2}' || echo "500") + + if [ "$http_code" = "422" ]; then + # Line not in diff — fall back to PR-level comment with citation + gh api "repos/${owner_repo}/issues/${pr}/comments" \ + -F body="$body + +_(originally intended as inline on \`$path:$line\` but that line is not in the diff)_" > /dev/null 2>&1 || true + fi +} + +# post_summary_comment +post_summary_comment() { + local pr="$1" body="$2" + local owner_repo; owner_repo=$(detect_owner_repo) + gh api "repos/${owner_repo}/issues/${pr}/comments" -F body="$body" > /dev/null 2>&1 || { + echo "WARN: could not post summary comment (fork PR or missing pull-requests:write scope)" >&2 + } +} + +# post_or_update_check_run <summary> +post_or_update_check_run() { + local sha="$1" conclusion="$2" title="$3" summary="$4" + local owner_repo; owner_repo=$(detect_owner_repo) + local check_name="sdk-module-review" + local external_id="sdk-review-v1-${sha}" + + # try to find existing run for same SHA + name + local existing + existing=$(gh api "repos/${owner_repo}/commits/${sha}/check-runs?check_name=${check_name}" 2>/dev/null | \ + jq -r '.check_runs[0].id // empty' 2>/dev/null || echo "") + + if [ -n "$existing" ]; then + gh api -X PATCH "repos/${owner_repo}/check-runs/${existing}" \ + -F status="completed" \ + -F conclusion="$conclusion" \ + -F "output[title]=$title" \ + -F "output[summary]=$summary" > /dev/null 2>&1 || { + echo "WARN: could not update check-run (fork PR or missing checks:write scope)" >&2 + } + else + gh api "repos/${owner_repo}/check-runs" \ + -F name="$check_name" \ + -F head_sha="$sha" \ + -F external_id="$external_id" \ + -F status="completed" \ + -F conclusion="$conclusion" \ + -F "output[title]=$title" \ + -F "output[summary]=$summary" > /dev/null 2>&1 || { + echo "WARN: could not create check-run (fork PR or missing checks:write scope)" >&2 + } + fi +} + +# apply_label <pr> <label> — creates label if missing, adds to PR, removes other sdk-review labels +apply_label() { + local pr="$1" label="$2" + local owner_repo; owner_repo=$(detect_owner_repo) + + # ensure labels exist (silent on failure — fork PR or no permission) + for l in "sdk-review: ✅ passed:0e8a16" "sdk-review: ❌ blocked:b60205" \ + "sdk-review: ⚠️ flagged:e4e669" "sdk-review: skipped:cccccc"; do + local name color + name="${l%:*}"; color="${l##*:}" + gh api "repos/${owner_repo}/labels" -F name="$name" -F color="$color" > /dev/null 2>&1 || true + done + + # remove any existing sdk-review labels first + # Guard: `grep` returns 1 on no-match, which combined with `set -o pipefail` + # aborts the caller. Use `|| true` to swallow that. + local existing_labels + existing_labels=$(gh pr view "$pr" --json labels -q '.labels[].name' 2>/dev/null || echo "") + echo "$existing_labels" | grep '^sdk-review:' 2>/dev/null | while read -r existing; do + [ -n "$existing" ] && gh pr edit "$pr" --remove-label "$existing" > /dev/null 2>&1 || true + done || true + + # add the target label + gh pr edit "$pr" --add-label "$label" > /dev/null 2>&1 || { + echo "WARN: could not apply label (fork PR or missing pull-requests:write scope)" >&2 + } +} + +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + cmd="${1:-}"; shift || true + case "$cmd" in + check_gh_auth) check_gh_auth "$@" ;; + detect_hostname) detect_hostname "$@" ;; + detect_owner_repo) detect_owner_repo "$@" ;; + get_pr_head_sha) get_pr_head_sha "$@" ;; + is_fork_pr) is_fork_pr "$@" ;; + delete_prior_bot_artifacts) delete_prior_bot_artifacts "$@" ;; + post_inline_comment) post_inline_comment "$@" ;; + post_summary_comment) post_summary_comment "$@" ;; + post_or_update_check_run) post_or_update_check_run "$@" ;; + apply_label) apply_label "$@" ;; + *) echo "Usage: github-api.sh <command> args" >&2; exit 2 ;; + esac +fi diff --git a/.claude/scripts/lib/json-emit.sh b/.claude/scripts/lib/json-emit.sh new file mode 100755 index 00000000..4917107d --- /dev/null +++ b/.claude/scripts/lib/json-emit.sh @@ -0,0 +1,76 @@ +#!/usr/bin/env bash +# lib/json-emit.sh — Shared JSON output helpers for check scripts. +# Every check-<X>.sh emits a JSON object matching the schema in 00-COMMON.md §3. +set -euo pipefail + +# emit_finding writes a single finding object to stdout (one JSON line per call). +# Args: rule severity file line message [suggestion] +emit_finding() { + local rule="$1" severity="$2" file="$3" line="$4" message="$5" suggestion="${6:-}" + jq -cn \ + --arg rule "$rule" \ + --arg severity "$severity" \ + --arg file "$file" \ + --argjson line "$line" \ + --arg message "$message" \ + --arg suggestion "$suggestion" \ + '{rule:$rule, severity:$severity, file:$file, line:$line, message:$message, suggestion:$suggestion}' +} + +# emit_report assembles the full JSON report from a set of findings (JSONL on stdin). +# Args: check_name language status started_at +emit_report() { + local check="$1" language="$2" status="$3" started_at="$4" + local tmp; tmp=$(mktemp) + cat > "$tmp" + local block_count flag_count + block_count=$(jq -s '[.[] | select(.severity=="BLOCK")] | length' < "$tmp") + flag_count=$(jq -s '[.[] | select(.severity=="FLAG")] | length' < "$tmp") + # Build findings array from JSONL (jq -s reads each JSON object and produces one array) + local findings; findings=$(jq -s '.' < "$tmp") + + jq -n \ + --arg check "$check" \ + --arg version "1.0.0" \ + --arg language "$language" \ + --arg status "$status" \ + --arg started_at "$started_at" \ + --argjson block_count "$block_count" \ + --argjson flag_count "$flag_count" \ + --argjson findings "$findings" \ + '{ + check: $check, + version: $version, + language: $language, + status: $status, + started_at: $started_at, + duration_ms: 0, + modules_analysed: [], + findings: $findings, + summary: { + block_count: $block_count, + flag_count: $flag_count, + pass_criteria_met: [], + pass_criteria_failed: [] + } + }' + rm -f "$tmp" +} + +# status_from_findings derives PASS/FLAG/BLOCK from a JSONL of findings on stdin. +status_from_findings() { + local input; input=$(cat) + if [ -z "$input" ]; then echo "PASS"; return; fi + if echo "$input" | jq -s -e 'any(.severity=="BLOCK")' > /dev/null 2>&1; then + echo "BLOCK" + elif echo "$input" | jq -s -e 'any(.severity=="FLAG")' > /dev/null 2>&1; then + echo "FLAG" + else + echo "PASS" + fi +} + +# now_iso returns the current UTC time in ISO-8601 format. +now_iso() { + date -u +"%Y-%m-%dT%H:%M:%SZ" +} diff --git a/.claude/scripts/lib/predicates.sh b/.claude/scripts/lib/predicates.sh new file mode 100755 index 00000000..f0002c54 --- /dev/null +++ b/.claude/scripts/lib/predicates.sh @@ -0,0 +1,99 @@ +#!/usr/bin/env bash +# lib/predicates.sh — Evaluate scope predicates for rules. +# Called by orchestrate.sh before each check runs. +# Outputs comma-separated rule IDs to SKIP because predicates are false. +set -euo pipefail + +# has_commit_type <base_sha> <head_sha> <type1,type2,...> → prints "true" or "false" +has_commit_type() { + local base="$1" head="$2" types="$3" + local pattern + pattern=$(echo "$types" | tr ',' '|') + if git log "${base}..${head}" --format=%s 2>/dev/null | grep -qE "^(${pattern})(\(.*\))?!?: "; then + echo "true" + else + echo "false" + fi +} + +# module_shape <module_dir> → prints "client" | "patch" | "config" | "other" +module_shape() { + local dir="$1" + if [ -f "$dir/client.py" ] || [ -f "$dir/_http.py" ] || compgen -G "$dir/*Client.java" > /dev/null 2>&1; then + echo "client" + elif [ -f "$dir/_patch.py" ] || compgen -G "$dir/patch*.py" > /dev/null 2>&1; then + echo "patch" + elif [ -f "$dir/config.py" ] && [ ! -f "$dir/client.py" ]; then + echo "config" + else + echo "other" + fi +} + +# reuse_toml_aggregate_present <repo_root> → prints "true" or "false" +# BSD-safe: uses [[:space:]] instead of \s. +reuse_toml_aggregate_present() { + local root="${1:-.}" + local reuse="$root/REUSE.toml" + if [ ! -f "$reuse" ]; then echo "false"; return; fi + if grep -qE 'path[[:space:]]*=[[:space:]]*"\*\*?"' "$reuse" 2>/dev/null; then + echo "true" + else + echo "false" + fi +} + +# file_in_added_set <file> <added_lines_file> → prints "true" or "false" +# Uses grep -F for fixed-string matching (safe with paths containing regex metachars). +file_in_added_set() { + local file="$1" added_lines_file="$2" + [ -f "$added_lines_file" ] || { echo "false"; return; } + if grep -Fq "${file}:" "$added_lines_file"; then + echo "true" + else + echo "false" + fi +} + +# line_in_added_set <file> <line> <added_lines_file> → prints "true" or "false" +line_in_added_set() { + local file="$1" line="$2" added_lines_file="$3" + [ -f "$added_lines_file" ] || { echo "false"; return; } + # Use grep -Fx (fixed, whole-line) to avoid regex metachars in path + if grep -Fxq "${file}:${line}" "$added_lines_file"; then + echo "true" + else + echo "false" + fi +} + +# filter_findings_by_hunk <report_file> <added_lines_file> → prints filtered report +# Fix: build the added-lines set as a proper JSON array (not slurped strings). +filter_findings_by_hunk() { + local report="$1" added="$2" + local added_json + if [ ! -f "$added" ] || [ ! -s "$added" ]; then + added_json="[]" + else + added_json=$(sort -u "$added" | jq -R . | jq -s '.') + fi + jq --argjson added "$added_json" ' + .findings |= map( + select($added | index(.file + ":" + (.line|tostring)) != null) + ) + ' "$report" +} + +# When executed directly, dispatch to subcommand +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + cmd="${1:-}"; shift || true + case "$cmd" in + has_commit_type) has_commit_type "$@" ;; + module_shape) module_shape "$@" ;; + reuse_toml_aggregate_present) reuse_toml_aggregate_present "$@" ;; + file_in_added_set) file_in_added_set "$@" ;; + line_in_added_set) line_in_added_set "$@" ;; + filter_findings_by_hunk) filter_findings_by_hunk "$@" ;; + *) echo "Usage: predicates.sh <command> [args]" >&2; exit 2 ;; + esac +fi diff --git a/.claude/scripts/lib/skill-self-skip.sh b/.claude/scripts/lib/skill-self-skip.sh new file mode 100755 index 00000000..53658845 --- /dev/null +++ b/.claude/scripts/lib/skill-self-skip.sh @@ -0,0 +1,19 @@ +#!/usr/bin/env bash +# lib/skill-self-skip.sh — return "true" if a file is part of the SDK review skill itself +# (used to prevent meta-review-loop where the skill's own files trigger findings). +set -euo pipefail + +is_skill_file() { + local file="$1" + case "$file" in + .claude/*) echo "true"; return ;; + tests/sdk-review/*) echo "true"; return ;; + .github/workflows/sdk-*) echo "true"; return ;; + docs/PR-REVIEW.md|docs/BRANCH-PROTECTION-SETUP.md) echo "true"; return ;; + *) echo "false" ;; + esac +} + +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + is_skill_file "$@" +fi diff --git a/.claude/scripts/lib/suppression.sh b/.claude/scripts/lib/suppression.sh new file mode 100755 index 00000000..54bba932 --- /dev/null +++ b/.claude/scripts/lib/suppression.sh @@ -0,0 +1,84 @@ +#!/usr/bin/env bash +# lib/suppression.sh — Parse # sdk-review: ignore[<check>] comments. +# Outputs "<file>:<line>:<check>" tuples of suppressions found in a diff. +set -euo pipefail + +# Line-level: any occurrence of "sdk-review: ignore[X,Y]" or "sdk-review: ignore" on same line +# File-level: "sdk-review-ignore-file: X,Y" within first 20 lines of file + +# parse_line_suppressions <file> +# Scans the file and emits <file>:<line>:<check> for each suppression on that line. +# Anchor to a comment context to avoid false positives in string literals or docstrings. +parse_line_suppressions() { + local file="$1" + [ -f "$file" ] || return 0 + awk -v file="$file" ' + # Require the marker to be preceded by a comment character (# or //) so we + # do not match "sdk-review: ignore" inside a docstring or string literal. + match($0, /(#|\/\/)[[:space:]]*sdk-review:[[:space:]]*ignore(\[[^]]*\])?/) { + m = substr($0, RSTART, RLENGTH) + # extract [checks] if present + if (match(m, /\[[^]]*\]/)) { + checks = substr(m, RSTART+1, RLENGTH-2) + n = split(checks, arr, ",") + for (j=1; j<=n; j++) { + gsub(/^ +| +$/, "", arr[j]) + print file ":" NR ":" arr[j] + } + } else { + print file ":" NR ":*" + } + } + ' "$file" +} + +# parse_file_suppressions <file> +# Emits <file>:*:<check> when a file-level suppression header is present in first 20 lines. +parse_file_suppressions() { + local file="$1" + [ -f "$file" ] || return 0 + head -20 "$file" 2>/dev/null | awk -v file="$file" ' + # Require comment prefix + match($0, /(#|\/\/)[[:space:]]*sdk-review-ignore-file:[[:space:]]*[a-zA-Z0-9_,-]+/) { + m = substr($0, RSTART, RLENGTH) + sub(/^(#|\/\/)[[:space:]]*sdk-review-ignore-file:[[:space:]]*/, "", m) + n = split(m, arr, ",") + for (j=1; j<=n; j++) { + gsub(/^ +| +$/, "", arr[j]) + print file ":*:" arr[j] + } + } + ' +} + +# is_suppressed <rule> <file> <line> <suppressions_file> +# Locked rules (SEC-*, HC-03, DIS-06, LIC-01/02, BND-02, BND-05, BREAKING-*) NEVER suppressed. +# Uses fixed-string matching so paths with regex metachars (dots, brackets) work. +is_suppressed() { + local rule="$1" file="$2" line="$3" supp_file="$4" + + case "$rule" in + SEC-*|HC-03|DIS-06|LIC-01|LIC-02|BND-02|BND-05|BREAKING-*) + echo "false"; return + ;; + esac + + [ -f "$supp_file" ] || { echo "false"; return; } + + # Use grep -Fx (fixed-string, whole-line) to safely handle regex metachars in path. + if grep -Fxq "${file}:${line}:${rule}" "$supp_file"; then echo "true"; return; fi + if grep -Fxq "${file}:${line}:*" "$supp_file"; then echo "true"; return; fi + if grep -Fxq "${file}:*:${rule}" "$supp_file"; then echo "true"; return; fi + + echo "false" +} + +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + cmd="${1:-}"; shift || true + case "$cmd" in + parse_line) parse_line_suppressions "$@" ;; + parse_file) parse_file_suppressions "$@" ;; + is_suppressed) is_suppressed "$@" ;; + *) echo "Usage: suppression.sh {parse_line|parse_file|is_suppressed} args" >&2; exit 2 ;; + esac +fi diff --git a/.claude/scripts/lib/tier-manager.sh b/.claude/scripts/lib/tier-manager.sh new file mode 100755 index 00000000..d15925b5 --- /dev/null +++ b/.claude/scripts/lib/tier-manager.sh @@ -0,0 +1,116 @@ +#!/usr/bin/env bash +# lib/tier-manager.sh — apply rule tiers (SHADOW/FLAG/BLOCK/BLOCK_LOCKED) to a report. +# Reads rules.yaml, filters/reclassifies findings based on rule tier. +set -euo pipefail + +# get_tier <rule> <rules_yaml_file> → prints tier (empty if not configured) +# Supports BOTH: +# block form: RULE-ID: +# tier: X +# flow form: RULE-ID: { tier: X } +# flow form: RULE-ID: { tier: X, predicates: {...} } +get_tier() { + local rule="$1" cfg="$2" + [ -f "$cfg" ] || { echo ""; return; } + + awk -v rule="$rule" ' + # Escape regex-special chars in rule id for safe matching + BEGIN { + # Just anchor at start of key line, requiring `<rule>:` at column 3 + pattern = "^ " rule "[[:space:]]*:" + } + + # Match rule header + $0 ~ pattern { + # Try inline flow form first: RULE: { tier: X, ... } or { tier: "X" } + if (match($0, /tier[[:space:]]*:[[:space:]]*[A-Z_]+/)) { + t = substr($0, RSTART, RLENGTH) + sub(/^tier[[:space:]]*:[[:space:]]*/, "", t) + print t + exit + } + # Fall through to block form + in_rule = 1 + next + } + # Block form: look for next rule id or dedent + in_rule && /^ [A-Z][A-Z0-9_-]*[[:space:]]*:/ { in_rule = 0 } + in_rule && /^[^ ]/ { in_rule = 0 } + in_rule && /tier[[:space:]]*:/ { + match($0, /tier[[:space:]]*:[[:space:]]*[A-Z_]+/) + if (RSTART > 0) { + t = substr($0, RSTART, RLENGTH) + sub(/^tier[[:space:]]*:[[:space:]]*/, "", t) + print t + exit + } + } + ' "$cfg" +} + +# apply_tiers_to_report <report> <rules_yaml> → prints filtered report +# SHADOW findings are stripped from .findings and moved to .shadow_findings +# BLOCK_LOCKED findings get a locked=true flag +apply_tiers_to_report() { + local report="$1" cfg="$2" + + local kept=() shadow=() + local n; n=$(jq '.findings | length' "$report") + local i=0 + while [ "$i" -lt "$n" ]; do + local f rule tier severity + f=$(jq -c ".findings[$i]" "$report") + rule=$(echo "$f" | jq -r '.rule') + tier=$(get_tier "$rule" "$cfg") + severity=$(echo "$f" | jq -r '.severity') + # Fall back to finding's original severity when rule not configured + [ -z "$tier" ] && tier="$severity" + + case "$tier" in + SHADOW) + shadow+=("$(echo "$f" | jq -c '. + {tier: "SHADOW"}')") + ;; + FLAG) + kept+=("$(echo "$f" | jq -c '.severity = "FLAG" | .tier = "FLAG"')") + ;; + BLOCK) + kept+=("$(echo "$f" | jq -c '.severity = "BLOCK" | .tier = "BLOCK"')") + ;; + BLOCK_LOCKED) + kept+=("$(echo "$f" | jq -c '.severity = "BLOCK" | .tier = "BLOCK_LOCKED" | .locked = true')") + ;; + OFF) + # explicit off → skip entirely (neither posted nor shadowed) + ;; + *) + kept+=("$(echo "$f" | jq -c ". + {tier: \"$tier\"}")") + ;; + esac + i=$((i + 1)) + done + + # Empty arrays under set -u: guard explicitly to avoid jq input errors + local kept_json shadow_json + if [ "${#kept[@]}" -eq 0 ]; then + kept_json="[]" + else + kept_json=$(printf '%s\n' "${kept[@]}" | jq -s -c '.') + fi + if [ "${#shadow[@]}" -eq 0 ]; then + shadow_json="[]" + else + shadow_json=$(printf '%s\n' "${shadow[@]}" | jq -s -c '.') + fi + + jq --argjson kept "$kept_json" --argjson shadow "$shadow_json" \ + '.findings = $kept | .shadow_findings = $shadow' "$report" +} + +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + cmd="${1:-}"; shift || true + case "$cmd" in + get_tier) get_tier "$@" ;; + apply_tiers_to_report) apply_tiers_to_report "$@" ;; + *) echo "Usage: tier-manager.sh {get_tier|apply_tiers_to_report} args" >&2; exit 2 ;; + esac +fi diff --git a/tests/sdk-review/fixtures/binding-token-concat.diff b/tests/sdk-review/fixtures/binding-token-concat.diff new file mode 100644 index 00000000..1b51251d --- /dev/null +++ b/tests/sdk-review/fixtures/binding-token-concat.diff @@ -0,0 +1,7 @@ +diff --git a/src/config.java b/src/config.java +index 111..222 100644 +--- a/src/config.java ++++ b/src/config.java +@@ -1,3 +1,4 @@ ++ String tokenUrl = uaaData.url + "/oauth/token"; + x = 1 diff --git a/tests/sdk-review/fixtures/clean.diff b/tests/sdk-review/fixtures/clean.diff new file mode 100644 index 00000000..ca1ecd85 --- /dev/null +++ b/tests/sdk-review/fixtures/clean.diff @@ -0,0 +1,9 @@ +diff --git a/src/module.py b/src/module.py +index 111..222 100644 +--- a/src/module.py ++++ b/src/module.py +@@ -1,3 +1,4 @@ + # existing code ++def new_helper() -> None: ++ return None + x = 1 diff --git a/tests/sdk-review/fixtures/disclosure-internal.diff b/tests/sdk-review/fixtures/disclosure-internal.diff new file mode 100644 index 00000000..be1b3cf2 --- /dev/null +++ b/tests/sdk-review/fixtures/disclosure-internal.diff @@ -0,0 +1,7 @@ +diff --git a/src/config.py b/src/config.py +index 111..222 100644 +--- a/src/config.py ++++ b/src/config.py +@@ -1,3 +1,4 @@ ++API = "https://internal.tools.sap/api" + x = 1 diff --git a/tests/sdk-review/fixtures/hardcode-url.diff b/tests/sdk-review/fixtures/hardcode-url.diff new file mode 100644 index 00000000..70c1668c --- /dev/null +++ b/tests/sdk-review/fixtures/hardcode-url.diff @@ -0,0 +1,8 @@ +diff --git a/src/sap_cloud_sdk/mymodule/client.py b/src/sap_cloud_sdk/mymodule/client.py +index 111..222 100644 +--- a/src/sap_cloud_sdk/mymodule/client.py ++++ b/src/sap_cloud_sdk/mymodule/client.py +@@ -1,3 +1,4 @@ + # existing ++API_URL = "https://api.example-real-service.com/v1" + x = 1 diff --git a/tests/sdk-review/fixtures/secrets-aws-key.diff b/tests/sdk-review/fixtures/secrets-aws-key.diff new file mode 100644 index 00000000..1c8c11a1 --- /dev/null +++ b/tests/sdk-review/fixtures/secrets-aws-key.diff @@ -0,0 +1,9 @@ +diff --git a/src/config.py b/src/config.py +new file mode 100644 +index 0000000..abc1234 +--- /dev/null ++++ b/src/config.py +@@ -0,0 +1,3 @@ ++# SPDX-License-Identifier: Apache-2.0 ++AWS_KEY = "AKIATESTFIXTUREXXXXX" ++other = "safe string" diff --git a/tests/sdk-review/fixtures/secrets-jwt.diff b/tests/sdk-review/fixtures/secrets-jwt.diff new file mode 100644 index 00000000..f4dd9804 --- /dev/null +++ b/tests/sdk-review/fixtures/secrets-jwt.diff @@ -0,0 +1,8 @@ +diff --git a/src/token.py b/src/token.py +new file mode 100644 +index 0000000..abc1234 +--- /dev/null ++++ b/src/token.py +@@ -0,0 +1,2 @@ ++# SPDX-License-Identifier: Apache-2.0 ++JWT_TOKEN = "eyJTESTFIXTURE0.eyJTESTFIXTURE00000000.TESTFIXTURESIG-NotARealJwt" diff --git a/tests/sdk-review/test_extended.bats b/tests/sdk-review/test_extended.bats new file mode 100644 index 00000000..84b0da5f --- /dev/null +++ b/tests/sdk-review/test_extended.bats @@ -0,0 +1,448 @@ +#!/usr/bin/env bats +# Extended test suite — tier gating, suppression, integration flows, edge cases. + +setup() { + SCRIPT_DIR="$BATS_TEST_DIRNAME/../../.claude/scripts" + FIXTURES="$BATS_TEST_DIRNAME/fixtures" + export LANGUAGE=python + export REPO_ROOT="$BATS_TEST_DIRNAME/../.." + export CONFIG_DIR="$REPO_ROOT/.claude/config" +} + +# ============================================================ +# TIER GATING (aggregate.sh applies rules.yaml tiers) +# ============================================================ + +@test "aggregate: SHADOW rule findings moved to shadow_findings, not posted" { + [ -f "$SCRIPT_DIR/aggregate.sh" ] || skip "aggregate.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/report-test.json" <<'EOF' +{ + "check": "test", + "version": "1.0.0", + "language": "python", + "status": "FLAG", + "started_at": "2026-01-01T00:00:00Z", + "duration_ms": 0, + "modules_analysed": [], + "findings": [ + {"rule": "PR-SIZE-01", "severity": "FLAG", "file": ".", "line": 1, "message": "big", "suggestion": ""}, + {"rule": "SEC-01", "severity": "BLOCK", "file": "x", "line": 1, "message": "aws", "suggestion": ""} + ], + "summary": {"block_count": 1, "flag_count": 1} +} +EOF + result=$(RULES_YAML="$CONFIG_DIR/rules.yaml" bash "$SCRIPT_DIR/aggregate.sh" "$tmpd") + # PR-SIZE-01 is SHADOW → goes to shadow_findings + echo "$result" | jq -e '.shadow_findings | length == 1' + echo "$result" | jq -e '.shadow_findings[0].rule == "PR-SIZE-01"' + # SEC-01 is BLOCK_LOCKED → stays in findings with locked=true + echo "$result" | jq -e '.findings | length == 1' + echo "$result" | jq -e '.findings[0].locked == true' + echo "$result" | jq -e '.summary.block_count == 1' + echo "$result" | jq -e '.summary.shadow_count == 1' + echo "$result" | jq -e '.summary.locked_count == 1' + rm -rf "$tmpd" +} + +@test "aggregate: unknown rule defaults to FLAG tier" { + [ -f "$SCRIPT_DIR/aggregate.sh" ] || skip "aggregate.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/report-test.json" <<'EOF' +{ + "check": "test", "version": "1.0.0", "language": "python", "status": "FLAG", + "started_at": "2026-01-01T00:00:00Z", "duration_ms": 0, "modules_analysed": [], + "findings": [ + {"rule": "UNKNOWN-99", "severity": "BLOCK", "file": "x", "line": 1, "message": "m", "suggestion": ""} + ], + "summary": {"block_count": 1, "flag_count": 0} +} +EOF + result=$(RULES_YAML="$CONFIG_DIR/rules.yaml" bash "$SCRIPT_DIR/aggregate.sh" "$tmpd") + echo "$result" | jq -e '.findings[0].severity == "FLAG"' + rm -rf "$tmpd" +} + +@test "aggregate: BLOCK_LOCKED rules cannot be softened" { + [ -f "$SCRIPT_DIR/aggregate.sh" ] || skip "aggregate.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/report-test.json" <<'EOF' +{ + "check": "test", "version": "1.0.0", "language": "python", "status": "FLAG", + "started_at": "2026-01-01T00:00:00Z", "duration_ms": 0, "modules_analysed": [], + "findings": [ + {"rule": "BND-02", "severity": "FLAG", "file": "x", "line": 1, "message": "token concat", "suggestion": ""} + ], + "summary": {"block_count": 0, "flag_count": 1} +} +EOF + result=$(RULES_YAML="$CONFIG_DIR/rules.yaml" bash "$SCRIPT_DIR/aggregate.sh" "$tmpd") + echo "$result" | jq -e '.findings[0].severity == "BLOCK"' + echo "$result" | jq -e '.findings[0].tier == "BLOCK_LOCKED"' + echo "$result" | jq -e '.findings[0].locked == true' + rm -rf "$tmpd" +} + +# ============================================================ +# SUPPRESSION (apply-suppression.sh) +# ============================================================ + +@test "suppression: parse_line detects multiple checks in ignore[a,b]" { + tmp=$(mktemp) + echo 'x = 1 # sdk-review: ignore[hardcode,patterns]' > "$tmp" + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" parse_line "$tmp") + echo "$result" | grep -q ":1:hardcode" + echo "$result" | grep -q ":1:patterns" + rm -f "$tmp" +} + +@test "suppression: parse_line ignore without brackets = wildcard" { + tmp=$(mktemp) + echo 'x = 1 # sdk-review: ignore' > "$tmp" + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" parse_line "$tmp") + echo "$result" | grep -q ":1:\*" + rm -f "$tmp" +} + +@test "suppression: is_suppressed returns true when rule matches line" { + supp_file=$(mktemp) + echo "src/x.py:42:HC-01" > "$supp_file" + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" is_suppressed "HC-01" "src/x.py" "42" "$supp_file") + [ "$result" = "true" ] + rm -f "$supp_file" +} + +@test "suppression: is_suppressed returns true when wildcard on line" { + supp_file=$(mktemp) + echo "src/x.py:42:*" > "$supp_file" + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" is_suppressed "ANY-RULE" "src/x.py" "42" "$supp_file") + [ "$result" = "true" ] + rm -f "$supp_file" +} + +@test "suppression: is_suppressed false on locked rule (SEC-01)" { + supp_file=$(mktemp) + echo "src/x.py:42:SEC-01" > "$supp_file" + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" is_suppressed "SEC-01" "src/x.py" "42" "$supp_file") + [ "$result" = "false" ] + rm -f "$supp_file" +} + +@test "suppression: is_suppressed false on locked rule (BND-02)" { + supp_file=$(mktemp) + echo "src/x.py:42:BND-02" > "$supp_file" + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" is_suppressed "BND-02" "src/x.py" "42" "$supp_file") + [ "$result" = "false" ] + rm -f "$supp_file" +} + +@test "suppression: file-level suppression matches any line" { + supp_file=$(mktemp) + echo "src/x.py:*:hardcode" > "$supp_file" + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" is_suppressed "hardcode" "src/x.py" "999" "$supp_file") + [ "$result" = "true" ] + rm -f "$supp_file" +} + +@test "apply-suppression: filters findings correctly" { + [ -f "$SCRIPT_DIR/lib/apply-suppression.sh" ] || skip "apply-suppression not in this batch" + supp_file=$(mktemp) + echo "src/x.py:10:HC-01" > "$supp_file" + + report=$(mktemp) + cat > "$report" <<'EOF' +{ + "check": "hardcode", "version": "1.0.0", "language": "python", "status": "BLOCK", + "started_at": "2026-01-01T00:00:00Z", "duration_ms": 0, "modules_analysed": [], + "findings": [ + {"rule": "HC-01", "severity": "BLOCK", "file": "src/x.py", "line": 10, "message": "url", "suggestion": ""}, + {"rule": "HC-01", "severity": "BLOCK", "file": "src/y.py", "line": 5, "message": "url", "suggestion": ""} + ], + "summary": {"block_count": 2, "flag_count": 0} +} +EOF + result=$(bash "$SCRIPT_DIR/lib/apply-suppression.sh" apply "$report" "$supp_file") + # x.py:10 suppressed, y.py:5 kept + echo "$result" | jq -e '.findings | length == 1' + echo "$result" | jq -e '.findings[0].file == "src/y.py"' + echo "$result" | jq -e '.summary.suppressed_count == 1' + rm -f "$supp_file" "$report" +} + +# ============================================================ +# JSON CONTRACT +# ============================================================ + +@test "json-emit: emit_report handles empty findings" { + source "$SCRIPT_DIR/lib/json-emit.sh" + result=$(echo -n "" | emit_report "test" "python" "PASS" "2026-01-01T00:00:00Z") + echo "$result" | jq -e '.findings | length == 0' + echo "$result" | jq -e '.summary.block_count == 0' + echo "$result" | jq -e '.summary.flag_count == 0' +} + +@test "json-emit: status_from_findings returns BLOCK when any BLOCK finding" { + source "$SCRIPT_DIR/lib/json-emit.sh" + result=$(printf '{"severity":"FLAG"}\n{"severity":"BLOCK"}\n' | status_from_findings) + [ "$result" = "BLOCK" ] +} + +@test "json-emit: status_from_findings returns PASS when empty" { + source "$SCRIPT_DIR/lib/json-emit.sh" + result=$(echo -n "" | status_from_findings) + [ "$result" = "PASS" ] +} + +# ============================================================ +# DIFF ATTRIBUTION EDGE CASES +# ============================================================ + +@test "diff-added-lines: handles multiple hunks in one file" { + cat > /tmp/multi.diff <<'EOF' +diff --git a/src/a.py b/src/a.py +--- a/src/a.py ++++ b/src/a.py +@@ -1,3 +1,4 @@ + line1 ++added_first + line2 +@@ -10,3 +11,4 @@ + line10 ++added_second + line11 +EOF + result=$(bash "$SCRIPT_DIR/lib/diff-added-lines.sh" < /tmp/multi.diff) + echo "$result" | grep -q "src/a.py:2" + echo "$result" | grep -q "src/a.py:12" + rm -f /tmp/multi.diff +} + +@test "diff-added-lines: multiple files" { + cat > /tmp/multi-file.diff <<'EOF' +diff --git a/a.py b/a.py +--- a/a.py ++++ b/a.py +@@ -1,1 +1,2 @@ + x ++new_in_a +diff --git a/b.py b/b.py +--- a/b.py ++++ b/b.py +@@ -1,1 +1,2 @@ + y ++new_in_b +EOF + result=$(bash "$SCRIPT_DIR/lib/diff-added-lines.sh" < /tmp/multi-file.diff) + echo "$result" | grep -q "a.py:2" + echo "$result" | grep -q "b.py:2" + rm -f /tmp/multi-file.diff +} + +# ============================================================ +# PREDICATES EDGE CASES +# ============================================================ + +@test "predicates: module_shape returns 'client' for module with client.py" { + source "$SCRIPT_DIR/lib/predicates.sh" + tmpd=$(mktemp -d) + touch "$tmpd/client.py" + result=$(module_shape "$tmpd") + [ "$result" = "client" ] + rm -rf "$tmpd" +} + +@test "predicates: module_shape returns 'patch' for module with _patch.py only" { + source "$SCRIPT_DIR/lib/predicates.sh" + tmpd=$(mktemp -d) + touch "$tmpd/_patch.py" + result=$(module_shape "$tmpd") + [ "$result" = "patch" ] + rm -rf "$tmpd" +} + +@test "predicates: module_shape returns 'other' for empty dir" { + source "$SCRIPT_DIR/lib/predicates.sh" + tmpd=$(mktemp -d) + result=$(module_shape "$tmpd") + [ "$result" = "other" ] + rm -rf "$tmpd" +} + +# ============================================================ +# AST EDGE CASES +# ============================================================ + +@test "ast: EL-02 accepts except: ...; return e" { + cat > /tmp/ok.py <<'EOF' +def f(): + try: + pass + except Exception as e: + return e +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" el-02 /tmp/ok.py) + [ -z "$result" ] + rm -f /tmp/ok.py +} + +@test "ast: EL-02 flags except: pass" { + cat > /tmp/bad.py <<'EOF' +def f(): + try: + do_work() + except Exception: + pass +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" el-02 /tmp/bad.py) + echo "$result" | jq -e '.rule == "PY-EL-02"' + rm -f /tmp/bad.py +} + +@test "ast: TEL-02 skips private methods" { + cat > /tmp/client.py <<'EOF' +class MyClient: + def _internal(self): + pass +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" tel-02 /tmp/client.py) + [ -z "$result" ] + rm -f /tmp/client.py +} + +@test "ast: TEL-02 skips non-Client classes" { + cat > /tmp/model.py <<'EOF' +class DataModel: + def get_value(self): + pass +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" tel-02 /tmp/model.py) + [ -z "$result" ] + rm -f /tmp/model.py +} + +@test "ast: CON-01 does not flag URLs" { + cat > /tmp/const.py <<'EOF' +a = "https://api.example.com" +b = "https://api.example.com" +c = "https://api.example.com" +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" con-01 /tmp/const.py) + [ -z "$result" ] + rm -f /tmp/const.py +} + +# ============================================================ +# BASELINE +# ============================================================ + +@test "baseline: repo-scope exempts all files" { + baseline='{"exemptions":{"PY-LIC-01":{"scope":"repo"}}}' + source "$SCRIPT_DIR/lib/baseline.sh" + result=$(is_exempted "PY-LIC-01" "src/anything.py" "$baseline") + [ "$result" = "true" ] +} + +@test "baseline: module scope exempts only matching module" { + baseline='{"exemptions":{"TEL-06":{"scope":"module:adms"}}}' + source "$SCRIPT_DIR/lib/baseline.sh" + in_module=$(is_exempted "TEL-06" "src/sap_cloud_sdk/adms/foo.py" "$baseline") + [ "$in_module" = "true" ] + other=$(is_exempted "TEL-06" "src/sap_cloud_sdk/destination/foo.py" "$baseline") + [ "$other" = "false" ] +} + +@test "baseline: unknown rule not exempted" { + baseline='{"exemptions":{"OTHER":{"scope":"repo"}}}' + source "$SCRIPT_DIR/lib/baseline.sh" + result=$(is_exempted "HC-01" "src/x.py" "$baseline") + [ "$result" = "false" ] +} + +# ============================================================ +# INTEGRATION FLOWS +# ============================================================ + +@test "orchestrate: empty diff produces summary with 0 findings" { + [ -f "$SCRIPT_DIR/orchestrate.sh" ] || skip "orchestrate.sh not in this batch" + tmpd=$(mktemp -d) + cp -r "$REPO_ROOT/.claude" "$tmpd/" + cd "$tmpd" + echo "[project]" > pyproject.toml + mkdir -p src/sap_cloud_sdk + echo "" > /tmp/empty.diff + + set +e + DRY_RUN=true LOCAL_DIFF=/tmp/empty.diff REPO_ROOT="$tmpd" \ + bash .claude/scripts/orchestrate.sh 999 --dry-run > /tmp/out.txt 2>&1 + status=$? + set -e + [ "$status" -eq 0 ] + grep -q "BLOCK: 0" /tmp/out.txt + rm -rf "$tmpd" /tmp/empty.diff /tmp/out.txt +} + +@test "orchestrate: dry-run generates valid summary.json" { + [ -f "$SCRIPT_DIR/orchestrate.sh" ] || skip "orchestrate.sh not in this batch" + tmpd=$(mktemp -d) + cp -r "$REPO_ROOT/.claude" "$tmpd/" + cd "$tmpd" + echo "[project]" > pyproject.toml + mkdir -p src/sap_cloud_sdk + + KEEP_TMP=1 DRY_RUN=true LOCAL_DIFF="$FIXTURES/clean.diff" REPO_ROOT="$tmpd" TMPDIR_RUN=/tmp/orch-test \ + bash .claude/scripts/orchestrate.sh 999 --dry-run > /dev/null 2>&1 || true + [ -f /tmp/orch-test/summary.json ] + jq -e '.summary.block_count != null' /tmp/orch-test/summary.json + jq -e '.per_check_summary' /tmp/orch-test/summary.json + rm -rf "$tmpd" /tmp/orch-test +} + +# ============================================================ +# SPECIFIC CHECK EDGE CASES +# ============================================================ + +@test "check-secrets: does not fire on placeholder-looking test fixtures" { + [ -f "$SCRIPT_DIR/check-secrets.sh" ] || skip "check-secrets.sh not in this batch" + # test data that looks like a key but is a test placeholder + cat > /tmp/test-fixture.diff <<'EOF' +diff --git a/tests/fixtures/token.txt b/tests/fixtures/token.txt +new file mode 100644 +index 0000000..abc +--- /dev/null ++++ b/tests/fixtures/token.txt +@@ -0,0 +1 @@ ++test-token-placeholder +EOF + DIFF_FILE=/tmp/test-fixture.diff bash "$SCRIPT_DIR/check-secrets.sh" < /tmp/test-fixture.diff > /tmp/out.json + # Should not fire on obvious placeholder + jq -e '.status == "PASS"' /tmp/out.json + rm -f /tmp/test-fixture.diff /tmp/out.json +} + +@test "check-hardcode: exempts localhost and example.com" { + [ -f "$SCRIPT_DIR/check-hardcode.sh" ] || skip "check-hardcode.sh not in this batch" + cat > /tmp/local.diff <<'EOF' +diff --git a/src/sap_cloud_sdk/mymod/client.py b/src/sap_cloud_sdk/mymod/client.py +index 111..222 +--- a/src/sap_cloud_sdk/mymod/client.py ++++ b/src/sap_cloud_sdk/mymod/client.py +@@ -1,1 +1,2 @@ + x ++URL = "https://localhost:8080/api" +EOF + DIFF_FILE=/tmp/local.diff bash "$SCRIPT_DIR/check-hardcode.sh" < /tmp/local.diff > /tmp/out.json + # localhost is allowlisted + jq -e '[.findings[] | select(.rule == "HC-01")] | length == 0' /tmp/out.json + rm -f /tmp/local.diff /tmp/out.json +} + +@test "check-disclosure: PR body Closes #<issue_number> triggers DIS-07" { + [ -f "$SCRIPT_DIR/check-disclosure.sh" ] || skip "check-disclosure.sh not in this batch" + pr_body=$(mktemp) + echo "Closes #<issue_number>" > "$pr_body" + cat > /tmp/empty.diff <<'EOF' +EOF + DIFF_FILE=/tmp/empty.diff PR_BODY_FILE="$pr_body" bash "$SCRIPT_DIR/check-disclosure.sh" < /tmp/empty.diff > /tmp/out.json + jq -e '.findings | map(.rule) | any(. == "DIS-07")' /tmp/out.json + rm -f "$pr_body" /tmp/empty.diff /tmp/out.json +} diff --git a/tests/sdk-review/test_smoke.bats b/tests/sdk-review/test_smoke.bats new file mode 100644 index 00000000..c305f043 --- /dev/null +++ b/tests/sdk-review/test_smoke.bats @@ -0,0 +1,285 @@ +#!/usr/bin/env bats +# smoke tests for the SDK Module Review skill + +setup() { + SCRIPT_DIR="$BATS_TEST_DIRNAME/../../.claude/scripts" + FIXTURES="$BATS_TEST_DIRNAME/fixtures" + export LANGUAGE=python + export REPO_ROOT="$BATS_TEST_DIRNAME/../.." + export CONFIG_DIR="$REPO_ROOT/.claude/config" +} + +# --- LIBS --- + +@test "lib/json-emit.sh: emit_finding produces valid JSON" { + source "$SCRIPT_DIR/lib/json-emit.sh" + result=$(emit_finding "TEST-01" "BLOCK" "src/x.py" 42 "test message") + echo "$result" | jq -e '.rule == "TEST-01"' + echo "$result" | jq -e '.severity == "BLOCK"' + echo "$result" | jq -e '.line == 42' +} + +@test "lib/json-emit.sh: emit_report assembles findings correctly" { + source "$SCRIPT_DIR/lib/json-emit.sh" + result=$(printf '{"rule":"A","severity":"BLOCK","file":"f","line":1,"message":"m","suggestion":""}\n{"rule":"B","severity":"FLAG","file":"g","line":2,"message":"m","suggestion":""}\n' | emit_report "test" "python" "BLOCK" "2026-01-01T00:00:00Z") + echo "$result" | jq -e '.check == "test"' + echo "$result" | jq -e '.findings | length == 2' + echo "$result" | jq -e '.summary.block_count == 1' + echo "$result" | jq -e '.summary.flag_count == 1' +} + +@test "lib/diff-added-lines.sh: extracts only added lines" { + cat > /tmp/test.diff <<'EOF' +diff --git a/src/a.py b/src/a.py +--- a/src/a.py ++++ b/src/a.py +@@ -1,3 +1,4 @@ + context1 ++added1 + context2 ++added2 +EOF + result=$(bash "$SCRIPT_DIR/lib/diff-added-lines.sh" < /tmp/test.diff) + echo "$result" | grep -q "src/a.py:2" + echo "$result" | grep -q "src/a.py:4" + ! echo "$result" | grep -q "context" +} + +@test "lib/detect-language.sh: detects Python from pyproject.toml" { + tmpd=$(mktemp -d) + echo "[project]" > "$tmpd/pyproject.toml" + mkdir -p "$tmpd/src/sap_cloud_sdk" + result=$(bash "$SCRIPT_DIR/lib/detect-language.sh" "$tmpd") + [ "$result" = "python" ] + rm -rf "$tmpd" +} + +@test "lib/predicates.sh: reuse_toml_aggregate_present returns true when REUSE.toml aggregate exists" { + source "$SCRIPT_DIR/lib/predicates.sh" + tmpd=$(mktemp -d) + cat > "$tmpd/REUSE.toml" <<'EOF' +version = 1 +[[annotations]] +path = "**" +SPDX-License-Identifier = "Apache-2.0" +EOF + result=$(reuse_toml_aggregate_present "$tmpd") + [ "$result" = "true" ] + rm -rf "$tmpd" +} + +@test "lib/predicates.sh: reuse_toml_aggregate_present false when no REUSE.toml" { + source "$SCRIPT_DIR/lib/predicates.sh" + tmpd=$(mktemp -d) + result=$(reuse_toml_aggregate_present "$tmpd") + [ "$result" = "false" ] + rm -rf "$tmpd" +} + +@test "lib/suppression.sh: parse_line detects ignore[hardcode]" { + tmp=$(mktemp) + cat > "$tmp" <<'EOF' +line1 +timeout = 30 # sdk-review: ignore[hardcode] +line3 +EOF + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" parse_line "$tmp") + echo "$result" | grep -q ":2:hardcode" + rm -f "$tmp" +} + +@test "lib/suppression.sh: locked rules refuse suppression" { + tmp=$(mktemp) + echo "$tmp:1:SEC-01" > "$tmp.supp" + result=$(bash "$SCRIPT_DIR/lib/suppression.sh" is_suppressed "SEC-01" "$tmp" 1 "$tmp.supp") + [ "$result" = "false" ] + rm -f "$tmp" "$tmp.supp" +} + +# --- CHECKS --- + +@test "check-secrets: fires BLOCK on AWS access key" { + [ -f "$SCRIPT_DIR/check-secrets.sh" ] || skip "check-secrets.sh not in this batch" + DIFF_FILE="$FIXTURES/secrets-aws-key.diff" bash "$SCRIPT_DIR/check-secrets.sh" < "$FIXTURES/secrets-aws-key.diff" > /tmp/out.json + jq -e '.status == "BLOCK"' /tmp/out.json + jq -e '.findings[0].rule == "SEC-01"' /tmp/out.json +} + +@test "check-secrets: fires BLOCK on JWT token" { + [ -f "$SCRIPT_DIR/check-secrets.sh" ] || skip "check-secrets.sh not in this batch" + DIFF_FILE="$FIXTURES/secrets-jwt.diff" bash "$SCRIPT_DIR/check-secrets.sh" < "$FIXTURES/secrets-jwt.diff" > /tmp/out.json + jq -e '.status == "BLOCK"' /tmp/out.json + jq -e '.findings | map(.rule) | any(. == "SEC-06")' /tmp/out.json +} + +@test "check-secrets: PASS on clean diff" { + [ -f "$SCRIPT_DIR/check-secrets.sh" ] || skip "check-secrets.sh not in this batch" + DIFF_FILE="$FIXTURES/clean.diff" bash "$SCRIPT_DIR/check-secrets.sh" < "$FIXTURES/clean.diff" > /tmp/out.json + jq -e '.status == "PASS"' /tmp/out.json +} + +@test "check-hardcode: fires BLOCK on hardcoded URL" { + [ -f "$SCRIPT_DIR/check-hardcode.sh" ] || skip "check-hardcode.sh not in this batch" + DIFF_FILE="$FIXTURES/hardcode-url.diff" bash "$SCRIPT_DIR/check-hardcode.sh" < "$FIXTURES/hardcode-url.diff" > /tmp/out.json + jq -e '.findings | length > 0' /tmp/out.json + jq -e '.findings | map(.rule) | any(. == "HC-01")' /tmp/out.json +} + +@test "check-disclosure: fires on SAP-internal URL (public profile)" { + [ -f "$SCRIPT_DIR/check-disclosure.sh" ] || skip "check-disclosure.sh not in this batch" + DISCLOSURE_PROFILE=public DIFF_FILE="$FIXTURES/disclosure-internal.diff" bash "$SCRIPT_DIR/check-disclosure.sh" < "$FIXTURES/disclosure-internal.diff" > /tmp/out.json + jq -e '.findings | map(.rule) | any(. == "DIS-01")' /tmp/out.json + jq -e '.findings[0].severity == "BLOCK"' /tmp/out.json +} + +@test "check-disclosure: internal profile downgrades DIS-01 to FLAG" { + [ -f "$SCRIPT_DIR/check-disclosure.sh" ] || skip "check-disclosure.sh not in this batch" + DISCLOSURE_PROFILE=internal DIFF_FILE="$FIXTURES/disclosure-internal.diff" bash "$SCRIPT_DIR/check-disclosure.sh" < "$FIXTURES/disclosure-internal.diff" > /tmp/out.json + jq -e '.findings[0].severity == "FLAG"' /tmp/out.json +} + +@test "check-license-spdx: REUSE.toml aggregate exempts LIC-01" { + [ -f "$SCRIPT_DIR/check-license-spdx.sh" ] || skip "check-license-spdx.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/REUSE.toml" <<'EOF' +version = 1 +[[annotations]] +path = "**" +SPDX-License-Identifier = "Apache-2.0" +EOF + REPO_ROOT="$tmpd" DIFF_FILE="$FIXTURES/secrets-aws-key.diff" bash "$SCRIPT_DIR/check-license-spdx.sh" < "$FIXTURES/secrets-aws-key.diff" > /tmp/out.json + jq -e '.status == "PASS"' /tmp/out.json + rm -rf "$tmpd" +} + +@test "check-license-spdx: fires BLOCK on new .py without SPDX header (no REUSE.toml)" { + [ -f "$SCRIPT_DIR/check-license-spdx.sh" ] || skip "check-license-spdx.sh not in this batch" + cat > /tmp/no-spdx.diff <<'EOF' +diff --git a/src/new.py b/src/new.py +new file mode 100644 +index 0000000..abc +--- /dev/null ++++ b/src/new.py +@@ -0,0 +1,2 @@ ++def foo(): ++ pass +EOF + tmpd=$(mktemp -d) + REPO_ROOT="$tmpd" DIFF_FILE=/tmp/no-spdx.diff bash "$SCRIPT_DIR/check-license-spdx.sh" < /tmp/no-spdx.diff > /tmp/out.json + jq -e '.status == "BLOCK"' /tmp/out.json + jq -e '.findings | map(.rule) | any(. == "LIC-01")' /tmp/out.json + rm -rf "$tmpd" +} + +@test "check-binding-shape: fires BLOCK_LOCKED on /oauth/token concat" { + [ -f "$SCRIPT_DIR/check-binding-shape.sh" ] || skip "check-binding-shape.sh not in this batch" + LANGUAGE=java DIFF_FILE="$FIXTURES/binding-token-concat.diff" bash "$SCRIPT_DIR/check-binding-shape.sh" < "$FIXTURES/binding-token-concat.diff" > /tmp/out.json + jq -e '.status == "BLOCK"' /tmp/out.json + jq -e '.findings | map(.rule) | any(. == "BND-02")' /tmp/out.json +} + +# --- AST checks --- + +@test "ast-python-checks: EL-01 fires on raising different exception without from e" { + cat > /tmp/bad.py <<'EOF' +def x(): + try: + pass + except ValueError as e: + raise KeyError("nope") +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" el-01 /tmp/bad.py) + echo "$result" | jq -e '.rule == "PY-EL-01"' + rm -f /tmp/bad.py +} + +@test "ast-python-checks: EL-01 PASSES on bare raise" { + cat > /tmp/good.py <<'EOF' +def x(): + try: + pass + except ValueError: + raise +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" el-01 /tmp/good.py) + [ -z "$result" ] + rm -f /tmp/good.py +} + +@test "ast-python-checks: EL-01 PASSES on raise X from e" { + cat > /tmp/good.py <<'EOF' +def x(): + try: + pass + except ValueError as e: + raise KeyError("wrapped") from e +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" el-01 /tmp/good.py) + [ -z "$result" ] + rm -f /tmp/good.py +} + +@test "ast-python-checks: TEL-02 fires on public *Client method without @record_metrics" { + cat > /tmp/client.py <<'EOF' +class MyClient: + def do_thing(self): + pass +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" tel-02 /tmp/client.py) + echo "$result" | jq -e '.rule == "PY-TEL-02"' + rm -f /tmp/client.py +} + +@test "ast-python-checks: TEL-02 PASSES when @record_metrics present" { + cat > /tmp/client.py <<'EOF' +class MyClient: + @record_metrics(Module.X, Operation.Y) + def do_thing(self): + pass +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" tel-02 /tmp/client.py) + [ -z "$result" ] + rm -f /tmp/client.py +} + +@test "ast-python-checks: CON-01 fires on 3× repeated string" { + cat > /tmp/const.py <<'EOF' +def x(): + a = "com.sap.adm.DocumentService" + b = "com.sap.adm.DocumentService" + c = "com.sap.adm.DocumentService" +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" con-01 /tmp/const.py) + echo "$result" | jq -e '.rule == "PY-CON-01"' + rm -f /tmp/const.py +} + +# --- Full orchestrator dry-run --- + +@test "orchestrate.sh: dry-run against secrets fixture exits 1 (BLOCK)" { + [ -f "$SCRIPT_DIR/orchestrate.sh" ] || skip "orchestrate.sh not in this batch" + tmpd=$(mktemp -d) + cp -r "$REPO_ROOT/.claude" "$tmpd/" + cd "$tmpd" + echo "[project]" > pyproject.toml + mkdir -p src/sap_cloud_sdk + set +e + DRY_RUN=true LOCAL_DIFF="$FIXTURES/secrets-aws-key.diff" REPO_ROOT="$tmpd" \ + bash .claude/scripts/orchestrate.sh 999 --dry-run > /dev/null 2>&1 + status=$? + set -e + [ "$status" -eq 1 ] + rm -rf "$tmpd" +} + +@test "orchestrate.sh: dry-run against clean fixture exits 0" { + [ -f "$SCRIPT_DIR/orchestrate.sh" ] || skip "orchestrate.sh not in this batch" + tmpd=$(mktemp -d) + cp -r "$REPO_ROOT/.claude" "$tmpd/" + cd "$tmpd" + echo "[project]" > pyproject.toml + mkdir -p src/sap_cloud_sdk + DRY_RUN=true LOCAL_DIFF="$FIXTURES/clean.diff" REPO_ROOT="$tmpd" \ + bash .claude/scripts/orchestrate.sh 999 --dry-run + rm -rf "$tmpd" +} From 6808a80ca482ef8d32d5490a810cb9078da0a286 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Mon, 6 Jul 2026 17:09:08 -0300 Subject: [PATCH 03/14] docs(sdk-review): document module-aliases scope + audit summary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Explain why the alias file contains only one pair (dms ↔ documentmanagement): after auditing both SDKs, that's the only cross-language module today whose Python and Java names disagree. Same-name modules (destination, objectstore) don't need entries — the resolver falls back to the identical name. Include the full module inventory so future contributors know when to append here. --- .claude/config/module-aliases.yaml | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/.claude/config/module-aliases.yaml b/.claude/config/module-aliases.yaml index c14ebbbb..67764227 100644 --- a/.claude/config/module-aliases.yaml +++ b/.claude/config/module-aliases.yaml @@ -1,5 +1,24 @@ -# Module name aliases across languages. -# Used by check-bdd.sh to resolve cross-repo feature file location. +# Module name aliases across the Python and Java cloud SDKs. +# Consumed by .claude/scripts/check-bdd.sh to resolve the sibling repo's +# feature-file path when the two languages happen to name the same module +# differently. +# +# Only add an entry when the module exists in BOTH SDKs under DIFFERENT names. +# When the names match on both sides, the resolver falls back to the same name +# — no entry needed. Examples of matching names today: `destination`, +# `objectstore`. Adding those here would be a no-op. +# +# Audit summary (2026-07): +# - Python-only modules (no Java counterpart yet): adms, agent_decorators, +# agent_memory, agentgateway, aicore, extensibility, ias, print +# - Java-only packages: core (SDK baseline, not a client module — skipped +# by check-bdd via the `^core$` filter) +# - Same-name modules on both sides: destination, objectstore +# - Different-name pair: dms ↔ documentmanagement (see below) +# +# When a new cross-language module lands that uses a different name on either +# side, append it here and update the audit summary above. + aliases: - python: dms java: documentmanagement From 1ea45cd709e9084b56d3d927754628ae459c9310 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Mon, 6 Jul 2026 12:50:47 -0300 Subject: [PATCH 04/14] =?UTF-8?q?test(sdk-review):=20batch=202/6=20?= =?UTF-8?q?=E2=80=94=20bats=20test=20harness=20+=20fixtures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds 57 bats tests + 6 fixture diffs. Tests exercise the libs from batch 1. All check-specific tests use conditional skip guards so tests skip cleanly when scripts are not yet present in the current batch. Part 2 of 6. Ref: AFSDK-3937 From 6e586dc5c53e31c275f882bf0ba1f6a2e5bf3da7 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:03:58 -0300 Subject: [PATCH 05/14] docs(sdk-review): shrink module-aliases.yaml to just the aliases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior comment listed the module inventory as of 2026-07, which: - Confused readers (looked like the aliases list was incomplete) - Would age fast (SDK gains modules; comment would drift) - Duplicated what check-bdd.sh already discovers dynamically from src/ Keep the file to the minimum it needs to do its job: explain what the alias map is, when to add entries, and the one real cross-language rename today (dms ↔ documentmanagement). Module discovery is dynamic — no manual list. --- .claude/config/module-aliases.yaml | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/.claude/config/module-aliases.yaml b/.claude/config/module-aliases.yaml index 67764227..27f07eb6 100644 --- a/.claude/config/module-aliases.yaml +++ b/.claude/config/module-aliases.yaml @@ -1,23 +1,14 @@ # Module name aliases across the Python and Java cloud SDKs. -# Consumed by .claude/scripts/check-bdd.sh to resolve the sibling repo's -# feature-file path when the two languages happen to name the same module -# differently. # -# Only add an entry when the module exists in BOTH SDKs under DIFFERENT names. -# When the names match on both sides, the resolver falls back to the same name -# — no entry needed. Examples of matching names today: `destination`, -# `objectstore`. Adding those here would be a no-op. +# Consumed by .claude/scripts/check-bdd.sh to resolve the sibling repo's +# feature-file path when the same module has DIFFERENT names in each SDK. # -# Audit summary (2026-07): -# - Python-only modules (no Java counterpart yet): adms, agent_decorators, -# agent_memory, agentgateway, aicore, extensibility, ias, print -# - Java-only packages: core (SDK baseline, not a client module — skipped -# by check-bdd via the `^core$` filter) -# - Same-name modules on both sides: destination, objectstore -# - Different-name pair: dms ↔ documentmanagement (see below) +# When a module has the SAME name in both SDKs (most cases), no entry is +# needed — check-bdd.sh discovers modules dynamically from `src/` and falls +# back to the identical name in the sibling repo. # -# When a new cross-language module lands that uses a different name on either -# side, append it here and update the audit summary above. +# Only add an entry here when both SDKs ship the same module under different +# names, and add it in the SAME PR that creates the divergent module. aliases: - python: dms From 22814e7c392ad06deab5c55eced03f247233662f Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:10:37 -0300 Subject: [PATCH 06/14] =?UTF-8?q?docs(sdk-review):=20empty=20aliases=20lis?= =?UTF-8?q?t=20=E2=80=94=20no=20speculative=20entries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prior 'dms ↔ documentmanagement' alias was speculative: the Java repo has never had a 'documentmanagement' module. The comment on this file already stated the correct rule ('add an entry in the SAME PR that introduces the divergent-name module') — the initial entry violated that rule by preempting a module that doesn't exist. Empty the list. Whoever creates 'documentmanagement' on the Java side (or any other cross-language divergent-name module) adds the alias in that PR. Discovery of module names is dynamic in check-bdd.sh — the empty list is the correct starting state. --- .claude/config/module-aliases.yaml | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/.claude/config/module-aliases.yaml b/.claude/config/module-aliases.yaml index 27f07eb6..e4641016 100644 --- a/.claude/config/module-aliases.yaml +++ b/.claude/config/module-aliases.yaml @@ -3,13 +3,11 @@ # Consumed by .claude/scripts/check-bdd.sh to resolve the sibling repo's # feature-file path when the same module has DIFFERENT names in each SDK. # -# When a module has the SAME name in both SDKs (most cases), no entry is -# needed — check-bdd.sh discovers modules dynamically from `src/` and falls -# back to the identical name in the sibling repo. +# When a module has the SAME name in both SDKs (the common case), no entry +# is needed — check-bdd.sh discovers modules dynamically from `src/` and +# falls back to the identical name in the sibling repo. # -# Only add an entry here when both SDKs ship the same module under different -# names, and add it in the SAME PR that creates the divergent module. +# Add an entry ONLY when both SDKs ship the same module under different +# names, and add it in the SAME PR that introduces the divergent-name module. -aliases: - - python: dms - java: documentmanagement +aliases: [] From 726caf53db76bc8e5706a3ec471e279cb9c4ae17 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:25:04 -0300 Subject: [PATCH 07/14] =?UTF-8?q?fix(sdk-review):=20bug-4=20marker=20prefi?= =?UTF-8?q?x=20=E2=80=94=20document=20that=20unclosed=20HTML=20fragment=20?= =?UTF-8?q?is=20intentional=20for=20jq=20substring=20match?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .claude/scripts/lib/github-api.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.claude/scripts/lib/github-api.sh b/.claude/scripts/lib/github-api.sh index ff7cca4c..3c4af30a 100755 --- a/.claude/scripts/lib/github-api.sh +++ b/.claude/scripts/lib/github-api.sh @@ -4,6 +4,15 @@ set -euo pipefail # Use plain vars (not readonly) so the file can be sourced multiple times # without hitting "readonly variable" under set -e. +# +# SDK_REVIEW_MARKER_PREFIX is intentionally an UNCLOSED HTML-comment fragment. +# It is NEVER emitted verbatim as a comment body; it is used only as a prefix +# substring matched via jq `contains($m)` in list_bot_review_comments() so we +# can identify all bot-posted comments regardless of their kind qualifier. +# Every real emitted marker (see orchestrate.sh) is a fully-closed HTML comment, +# e.g. "<!-- sdk-review:v1 kind=summary -->" or +# "<!-- sdk-review:v1 check=SEC-01 id=SEC-01-3 -->". +# Do not "close" this prefix or the substring match will miss all qualified forms. SDK_REVIEW_MARKER_PREFIX="${SDK_REVIEW_MARKER_PREFIX:-<!-- sdk-review:v1}" SUMMARY_MARKER="${SUMMARY_MARKER:-<!-- sdk-review:v1 kind=summary -->}" From 8f2044518615b37396d348e5941e5f7ab91961a2 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:43:39 -0300 Subject: [PATCH 08/14] =?UTF-8?q?fix(sdk-review):=20bug-2=20rules.yaml=20b?= =?UTF-8?q?aseline=20path=20=E2=80=94=20point=20to=20.claude/config/baseli?= =?UTF-8?q?ne.json?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The baseline.file field pointed to .claude/baseline.json but the actual JSON lives at .claude/config/baseline.json (co-located with rules.yaml in the config dir). Any consumer of the rules.yaml baseline reference would have failed to find the file. --- .claude/config/rules.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/config/rules.yaml b/.claude/config/rules.yaml index 1428f0fa..808541f4 100644 --- a/.claude/config/rules.yaml +++ b/.claude/config/rules.yaml @@ -7,7 +7,7 @@ version: 2 # ===================================================== baseline: - file: .claude/baseline.json + file: .claude/config/baseline.json regenerate_with: orchestrate.sh --baseline tiers: From 5ff6fc36279b417a1c117ef8da2f3de3170b4b60 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:45:18 -0300 Subject: [PATCH 09/14] fix(sdk-review): bug-3+bug-6 baseline rule IDs unified, at_commit resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug-3: strip PY-/JV- prefix from every rule key. rules.yaml uses agnostic LIC-01/LIC-02 as canonical rule IDs; baseline.json had drifted to PY-LIC-01/JV-LIC-01/etc, meaning baseline.sh is_in_line_baseline lookup never matched. Remove JV-* entries entirely (they belong to cloud-sdk-java's own baseline, not the Python repo's). Bug-6: at_commit was 'TBD' placeholder. Resolve to origin/main SHA at baseline creation time (d19427f5f3b9). Add top-level _documentation key explaining the semantic invariant — baseline snapshots pre-existing debt against canonical main, not against a drifting foundation branch. --- .claude/config/baseline.json | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/.claude/config/baseline.json b/.claude/config/baseline.json index 45c15950..24266819 100644 --- a/.claude/config/baseline.json +++ b/.claude/config/baseline.json @@ -1,24 +1,17 @@ { - "created_at": "2026-07-15T00:00:00Z", - "at_commit": "TBD", + "_documentation": "at_commit = SHA of origin/main against which pre-existing debt was snapshotted. Findings on lines touched after this SHA are compared against the baseline; entries already present at at_commit are exempted. schema_version bumps when the shape of 'exemptions' changes. Rule IDs here MUST match rule IDs in rules.yaml exactly (agnostic form, no PY-/JV- prefix).", + "created_at": "2026-07-07T20:00:00Z", + "at_commit": "d19427f5f3b9de409d7e4a655d6ce99362c1a17e", "schema_version": 1, "exemptions": { - "PY-LIC-01": { + "LIC-01": { "reason": "Repo uses REUSE.toml aggregate — pre-existing files exempt", "detector": "REUSE.toml exists with path='**' aggregate annotation", "scope": "repo", "files_exempted_glob": "src/**/*.py" }, - "PY-LIC-02": { - "reason": "Same as PY-LIC-01", - "scope": "repo" - }, - "JV-LIC-01": { - "reason": "cloud-sdk-java has no REUSE.toml AND no per-file SPDX baseline — rule kept SHADOW pending team decision on Wave-4 baseline PR", - "scope": "repo" - }, - "JV-LIC-02": { - "reason": "Same as JV-LIC-01", + "LIC-02": { + "reason": "Same as LIC-01", "scope": "repo" } } From fca651f17824f3f35770acc7768a782a6e6092c5 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:47:26 -0300 Subject: [PATCH 10/14] =?UTF-8?q?test(sdk-review):=20bug-3=20regression=20?= =?UTF-8?q?=E2=80=94=20baseline=E2=86=94rules.yaml=20contract=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add bats regression test asserting: 1. baseline.json is valid JSON 2. Every rule key in .exemptions appears in rules.yaml 3. at_commit is a real git SHA (not TBD placeholder) Guards against the drift Bohn caught in review — where baseline.json used PY-LIC-01 while rules.yaml used LIC-01, causing baseline lookup to always fail silently. --- tests/sdk-review/test_baseline_contract.bats | 46 ++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 tests/sdk-review/test_baseline_contract.bats diff --git a/tests/sdk-review/test_baseline_contract.bats b/tests/sdk-review/test_baseline_contract.bats new file mode 100644 index 00000000..516b68f8 --- /dev/null +++ b/tests/sdk-review/test_baseline_contract.bats @@ -0,0 +1,46 @@ +#!/usr/bin/env bats +# Contract test: every rule ID in baseline.json.exemptions must exist in rules.yaml. +# Guards against the drift Bohn caught in code review (PY-LIC-01 vs LIC-01). + +setup() { + REPO_ROOT="$BATS_TEST_DIRNAME/../.." + BASELINE="$REPO_ROOT/.claude/config/baseline.json" + RULES="$REPO_ROOT/.claude/config/rules.yaml" +} + +@test "baseline.json is valid JSON" { + [ -f "$BASELINE" ] || skip "baseline.json not in this batch" + jq empty "$BASELINE" +} + +@test "every baseline rule ID appears in rules.yaml" { + [ -f "$BASELINE" ] || skip "baseline.json not in this batch" + [ -f "$RULES" ] || skip "rules.yaml not in this batch" + + # Extract rule keys from baseline (excluding _documentation etc.) + baseline_rules=$(jq -r '.exemptions | keys[]' "$BASELINE") + # Extract rule keys from rules.yaml (any line matching /^ [A-Z][A-Z0-9_-]*:/) + rules_yaml_rules=$(grep -oE '^ [A-Z][A-Z0-9_-]*:' "$RULES" | tr -d ' :' | sort -u) + + for br in $baseline_rules; do + if ! echo "$rules_yaml_rules" | grep -Fxq "$br"; then + echo "FAIL: baseline rule '$br' not found in rules.yaml" >&2 + return 1 + fi + done +} + +@test "baseline.json has non-placeholder at_commit" { + [ -f "$BASELINE" ] || skip "baseline.json not in this batch" + at_commit=$(jq -r '.at_commit' "$BASELINE") + # Must be a 40-char hex SHA (or "PENDING_ORIGIN_MAIN" temporarily allowed) + if [[ "$at_commit" == "TBD" ]]; then + echo "FAIL: at_commit is placeholder 'TBD'" >&2 + return 1 + fi + # Real SHA is 40 hex chars + if ! echo "$at_commit" | grep -qE '^[0-9a-f]{40}$'; then + echo "FAIL: at_commit is not a git SHA: $at_commit" >&2 + return 1 + fi +} From 08f13092cbf8c98d0df1e24090d5d66f8b9d27da Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Mon, 6 Jul 2026 19:28:47 -0300 Subject: [PATCH 11/14] feat(sdk-review): add lib/hunk-filter.sh for FP-A-01 hunk attribution Empirical dry-run against the 19 open PRs (see docs/plans 09-FP-REMEDIATION) showed the v2 hunk-attribution primitive was specced but never wired: 84% of Python findings fired on lines the PR did not touch. This helper closes that gap. Individual check-*.sh scripts will consume is_line_touched / is_range_touched in a follow-up commit (per-check surgery to avoid one giant diff). Both helpers are permissive when $ADDED_LINES_FILE is unset (bats tests use synthetic diffs without the orchestrate wrapper) so nothing regresses in the isolated test harness. --- .claude/scripts/lib/hunk-filter.sh | 79 ++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100755 .claude/scripts/lib/hunk-filter.sh diff --git a/.claude/scripts/lib/hunk-filter.sh b/.claude/scripts/lib/hunk-filter.sh new file mode 100755 index 00000000..70930a57 --- /dev/null +++ b/.claude/scripts/lib/hunk-filter.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# lib/hunk-filter.sh — restrict findings to lines actually touched by the PR diff. +# +# The rule engine's contract (per 05-VALIDATION-V2.md §T-4) is that findings +# only fire on lines in the PR's added-lines set. Without this filter every +# check emits noise for pre-existing code. See docs/plans 09-FP-REMEDIATION +# §Pattern A for the empirical justification (84% FP rate reduced to <15%). +# +# The added-lines file is produced by orchestrate.sh from +# `lib/diff-added-lines.sh` and exported as $ADDED_LINES_FILE. Format: +# path/to/file.py:42 +# path/to/file.py:43 +# ... +# +# is_line_touched <file> <line> -> exit 0 if touched, 1 if not +# is_range_touched <file> <start> <end> -> exit 0 if any line in range touched +# +# Both helpers return 0 (permissive) when $ADDED_LINES_FILE is unset or empty +# so the checks continue to work when invoked standalone (e.g., bats tests). + +set -uo pipefail + +is_line_touched() { + local file="$1" line="$2" + # Backward-compat: without the added-lines context, allow everything. + [ -n "${ADDED_LINES_FILE:-}" ] || return 0 + [ -f "${ADDED_LINES_FILE:-}" ] || return 0 + [ -s "${ADDED_LINES_FILE:-}" ] || return 0 + grep -Fxq "${file}:${line}" "${ADDED_LINES_FILE}" +} + +is_range_touched() { + local file="$1" start="$2" end="$3" + [ -n "${ADDED_LINES_FILE:-}" ] || return 0 + [ -f "${ADDED_LINES_FILE:-}" ] || return 0 + [ -s "${ADDED_LINES_FILE:-}" ] || return 0 + # Cheap path: grep any line in [start, end] for the file + local ln + for ln in $(seq "$start" "$end"); do + if grep -Fxq "${file}:${ln}" "${ADDED_LINES_FILE}"; then + return 0 + fi + done + return 1 +} + +# is_meta_finding <file> -> exit 0 if this is a PR-metadata finding (PR_BODY, +# COMMIT:*, or ".") that should bypass hunk attribution entirely. +is_meta_finding() { + local file="$1" + case "$file" in + PR_BODY|.|""|COMMIT:*|PR_METADATA) return 0 ;; + *) return 1 ;; + esac +} + +# Convenience wrapper: emit only if the finding is either metadata or touched. +# Signature matches emit_finding but adds hunk filtering. +# emit_finding_if_touched <rule> <severity> <file> <line> <message> <suggestion> +emit_finding_if_touched() { + local rule="$1" sev="$2" file="$3" line="$4" msg="$5" sugg="$6" + if is_meta_finding "$file"; then + emit_finding "$rule" "$sev" "$file" "$line" "$msg" "$sugg" + return + fi + if is_line_touched "$file" "$line"; then + emit_finding "$rule" "$sev" "$file" "$line" "$msg" "$sugg" + fi +} + +# CLI entry point (used by bats tests) +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + case "${1:-}" in + is_line_touched) shift; is_line_touched "$@" ;; + is_range_touched) shift; is_range_touched "$@" ;; + is_meta_finding) shift; is_meta_finding "$@" ;; + *) echo "Usage: hunk-filter.sh {is_line_touched|is_range_touched|is_meta_finding} args" >&2; exit 2 ;; + esac +fi From a20b8c5b473cfc3736a1b7cd46e21e7da71e9a00 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:48:15 -0300 Subject: [PATCH 12/14] feat(sdk-review): bug-5 add lib/peer-consistency.sh to foundation peer-consistency.sh is a shared library (like hunk-filter, tier-manager, predicates). It belongs alongside the other libs in foundation, not downstream in the skill branch. Moving here so PR reviewers see the building blocks in the right layer. --- .claude/scripts/lib/peer-consistency.sh | 171 ++++++++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100755 .claude/scripts/lib/peer-consistency.sh diff --git a/.claude/scripts/lib/peer-consistency.sh b/.claude/scripts/lib/peer-consistency.sh new file mode 100755 index 00000000..76fd4ba8 --- /dev/null +++ b/.claude/scripts/lib/peer-consistency.sh @@ -0,0 +1,171 @@ +#!/usr/bin/env bash +# lib/peer-consistency.sh — compute peer-adoption fractions and fire only when +# a new module diverges from a widely-adopted element (>=80%). +# +# The v2 rule engine originally hard-coded "every client module MUST have a +# Factory" — but empirical review of the JV corpus showed only `destination` +# has a Factory; `objectstore`, `agentgateway`, `adms`, `aicore` are all +# valid module designs without one. Encoding one module's shape as universal +# law is FP-G-01 (see docs/plans 09-FP-REMEDIATION §Pattern G). +# +# This helper answers: "of all existing modules under src/**/<mod>/, what +# fraction have <element>?" A rule can then say `if fraction >= 0.8 and new +# module lacks it, emit FLAG`. +# +# All emissions from peer-consistency checks are FLAG tier, never BLOCK. +# +# API: +# peer_modules <repo_root> <lang> → prints one module name/line +# peer_element_fraction <repo_root> <lang> <element> +# → prints "count/total" and returns 0. `element` is one of: +# user-guide.md, exceptions, factory, client, config, py.typed +# should_flag_peer_divergence <mod_dir> <element> <fraction_str> +# → returns 0 if the module lacks the element AND fraction >= 0.8 + +set -uo pipefail + +peer_modules() { + local repo_root="${1:-.}" lang="${2:-python}" + if [ "$lang" = "python" ]; then + find "$repo_root/src/sap_cloud_sdk" -maxdepth 1 -mindepth 1 -type d 2>/dev/null | \ + awk -F/ '{print $NF}' | grep -vE '^(__pycache__|core)$' | sort -u + else + find "$repo_root/src/main/java/com/sap/cloud/sdk" -maxdepth 1 -mindepth 1 -type d 2>/dev/null | \ + awk -F/ '{print $NF}' | grep -vE '^(core)$' | sort -u + fi +} + +# has_element <mod_dir> <element> <lang> +# Returns 0 if the module dir contains the element. +# Elements: +# user-guide.md a user-guide.md at module root +# exceptions exceptions.py OR any Exception subclass in module (python) +# exceptions/ dir (java) +# factory create_*_client() function or *ClientFactory class +# client Client class (python: client.py; java: *Client.java) +# config config.py or *Config.java +# py.typed py.typed marker (python only) +has_element() { + local mod_dir="$1" el="$2" lang="${3:-python}" + [ -d "$mod_dir" ] || return 1 + case "$el" in + user-guide.md) + [ -f "$mod_dir/user-guide.md" ] + ;; + exceptions) + if [ "$lang" = "python" ]; then + [ -f "$mod_dir/exceptions.py" ] && return 0 + # Fallback: any Exception subclass anywhere in module tree + grep -rEIlq 'class [A-Z][A-Za-z0-9_]+\((Base)?Exception|[A-Z][A-Za-z]+(Error|Exception))\)' \ + "$mod_dir" 2>/dev/null + else + [ -d "$mod_dir/exceptions" ] + fi + ;; + factory) + if [ "$lang" = "python" ]; then + grep -rEIlq '^def create_[a-z_]*client\(' "$mod_dir" 2>/dev/null + else + grep -rEIlq 'class [A-Z][A-Za-z]*ClientFactory' "$mod_dir" 2>/dev/null + fi + ;; + client) + if [ "$lang" = "python" ]; then + [ -f "$mod_dir/client.py" ] && return 0 + grep -rEIlq '^class [A-Z][A-Za-z0-9_]+Client\b' "$mod_dir" 2>/dev/null + else + find "$mod_dir" -name '*Client.java' 2>/dev/null | grep -q . + fi + ;; + config) + if [ "$lang" = "python" ]; then + [ -f "$mod_dir/config.py" ] + else + find "$mod_dir" -name '*Config.java' 2>/dev/null | grep -q . + fi + ;; + py.typed) + [ -f "$mod_dir/py.typed" ] + ;; + *) + return 2 + ;; + esac +} + +# peer_element_fraction <repo_root> <lang> <element> +# Prints "adopted total fraction" where fraction is a float 0.0-1.0. +peer_element_fraction() { + local repo_root="$1" lang="$2" el="$3" + local mod adopted=0 total=0 mod_dir + while IFS= read -r mod; do + [ -z "$mod" ] && continue + if [ "$lang" = "python" ]; then + mod_dir="$repo_root/src/sap_cloud_sdk/$mod" + else + mod_dir="$repo_root/src/main/java/com/sap/cloud/sdk/$mod" + fi + total=$((total + 1)) + if has_element "$mod_dir" "$el" "$lang"; then + adopted=$((adopted + 1)) + fi + done < <(peer_modules "$repo_root" "$lang") + if [ "$total" -eq 0 ]; then + echo "0 0 0.0" + return + fi + # bash arithmetic can't do floats — compute via awk + local frac + frac=$(awk -v a="$adopted" -v t="$total" 'BEGIN{printf "%.3f", a/t}') + echo "$adopted $total $frac" +} + +# should_flag_peer_divergence <mod_dir> <element> <lang> <threshold> +# Returns 0 (should flag) if: +# - module lacks the element, AND +# - peer adoption fraction (excluding this module) >= threshold (default 0.80) +# The caller supplies the module dir, element name, and the repo/lang context. +should_flag_peer_divergence() { + local mod_dir="$1" el="$2" lang="${3:-python}" threshold="${4:-0.80}" + # Guard: if module has the element already, no flag. + if has_element "$mod_dir" "$el" "$lang"; then return 1; fi + # Compute fraction across peers, EXCLUDING the module itself. + local repo_root this_mod + this_mod="$(basename "$mod_dir")" + if [ "$lang" = "python" ]; then + repo_root="$(cd "$mod_dir/../../.." && pwd)" + else + repo_root="$(cd "$mod_dir/../../../../../../.." && pwd)" + fi + local mod adopted=0 total=0 peer_dir + while IFS= read -r mod; do + [ -z "$mod" ] && continue + # Skip the module under review — we only care about peer adoption. + [ "$mod" = "$this_mod" ] && continue + if [ "$lang" = "python" ]; then + peer_dir="$repo_root/src/sap_cloud_sdk/$mod" + else + peer_dir="$repo_root/src/main/java/com/sap/cloud/sdk/$mod" + fi + total=$((total + 1)) + if has_element "$peer_dir" "$el" "$lang"; then + adopted=$((adopted + 1)) + fi + done < <(peer_modules "$repo_root" "$lang") + # need at least 2 peers to draw a "pattern" + [ "$total" -ge 2 ] || return 1 + local frac + frac=$(awk -v a="$adopted" -v t="$total" 'BEGIN{printf "%.3f", a/t}') + awk -v f="$frac" -v t="$threshold" 'BEGIN{exit !(f+0 >= t+0)}' +} + +# CLI dispatch (only when executed, not when sourced) +if [ "${BASH_SOURCE[0]:-}" = "${0:-}" ] && [ -n "${BASH_SOURCE[0]:-}" ]; then + case "${1:-}" in + peer_modules) shift; peer_modules "$@" ;; + has_element) shift; has_element "$@" ;; + peer_element_fraction) shift; peer_element_fraction "$@" ;; + should_flag_peer_divergence) shift; should_flag_peer_divergence "$@" ;; + *) echo "Usage: peer-consistency.sh {peer_modules|has_element|peer_element_fraction|should_flag_peer_divergence} ..." >&2; exit 2 ;; + esac +fi From 7597a9388ffeea1e0eb73ddf8b60d5456b384e3f Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:49:17 -0300 Subject: [PATCH 13/14] =?UTF-8?q?fix(sdk-review):=20bug-7=20naming=20?= =?UTF-8?q?=E2=80=94=20ast=5Fpython=5Fchecks=20(underscore)=20everywhere?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit File on disk is ast_python_checks.py (Python convention). Docstrings and bats test descriptions were using ast-python-checks (hyphen), creating noise for any grep-based automation or reviewer trying to match the filename. --- .claude/scripts/lib/ast_python_checks.py | 4 ++-- tests/sdk-review/test_smoke.bats | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.claude/scripts/lib/ast_python_checks.py b/.claude/scripts/lib/ast_python_checks.py index 57172714..7e290c42 100755 --- a/.claude/scripts/lib/ast_python_checks.py +++ b/.claude/scripts/lib/ast_python_checks.py @@ -5,7 +5,7 @@ to stdout (one JSON object per line). Usage: - python3 ast-python-checks.py <check-name> <file1> [<file2> ...] + python3 ast_python_checks.py <check-name> <file1> [<file2> ...] Where <check-name> is one of: el-01, el-02 exception chaining / swallow @@ -377,7 +377,7 @@ def extract_dataclass_fields(tree: ast.Module) -> dict[str, list[str]]: def main(argv: list[str]) -> int: if len(argv) < 3: print( - "Usage: ast-python-checks.py <check-name> <file1> [<file2> ...]", + "Usage: ast_python_checks.py <check-name> <file1> [<file2> ...]", file=sys.stderr, ) return 2 diff --git a/tests/sdk-review/test_smoke.bats b/tests/sdk-review/test_smoke.bats index c305f043..b4b1ab0e 100644 --- a/tests/sdk-review/test_smoke.bats +++ b/tests/sdk-review/test_smoke.bats @@ -180,7 +180,7 @@ EOF # --- AST checks --- -@test "ast-python-checks: EL-01 fires on raising different exception without from e" { +@test "ast_python_checks: EL-01 fires on raising different exception without from e" { cat > /tmp/bad.py <<'EOF' def x(): try: @@ -193,7 +193,7 @@ EOF rm -f /tmp/bad.py } -@test "ast-python-checks: EL-01 PASSES on bare raise" { +@test "ast_python_checks: EL-01 PASSES on bare raise" { cat > /tmp/good.py <<'EOF' def x(): try: @@ -206,7 +206,7 @@ EOF rm -f /tmp/good.py } -@test "ast-python-checks: EL-01 PASSES on raise X from e" { +@test "ast_python_checks: EL-01 PASSES on raise X from e" { cat > /tmp/good.py <<'EOF' def x(): try: @@ -219,7 +219,7 @@ EOF rm -f /tmp/good.py } -@test "ast-python-checks: TEL-02 fires on public *Client method without @record_metrics" { +@test "ast_python_checks: TEL-02 fires on public *Client method without @record_metrics" { cat > /tmp/client.py <<'EOF' class MyClient: def do_thing(self): @@ -230,7 +230,7 @@ EOF rm -f /tmp/client.py } -@test "ast-python-checks: TEL-02 PASSES when @record_metrics present" { +@test "ast_python_checks: TEL-02 PASSES when @record_metrics present" { cat > /tmp/client.py <<'EOF' class MyClient: @record_metrics(Module.X, Operation.Y) @@ -242,7 +242,7 @@ EOF rm -f /tmp/client.py } -@test "ast-python-checks: CON-01 fires on 3× repeated string" { +@test "ast_python_checks: CON-01 fires on 3× repeated string" { cat > /tmp/const.py <<'EOF' def x(): a = "com.sap.adm.DocumentService" From f427f66e3fed1636e5e319b01bbd430567068476 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger <tiago.kochenborger@sap.com> Date: Tue, 7 Jul 2026 17:50:29 -0300 Subject: [PATCH 14/14] fix(sdk-review): bug-8 move tests to .claude/tests/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Bohn review — bats tests were sitting in tests/sdk-review/ which polluted the product test tree (pytest CI runs against tests/, this tooling uses bats). Moving to .claude/tests/ keeps the skill self- contained under .claude/ and out of the product test space. BATS_TEST_DIRNAME hop counts unchanged (both paths are 2 levels deep from repo root). Verified all 60 bats tests still green. skill-self-skip.sh: .claude/* already covers .claude/tests/* — kept legacy tests/sdk-review/* line as belt-and-suspenders. --- .claude/scripts/lib/skill-self-skip.sh | 2 +- .../tests}/fixtures/binding-token-concat.diff | 0 {tests/sdk-review => .claude/tests}/fixtures/clean.diff | 0 .../tests}/fixtures/disclosure-internal.diff | 0 {tests/sdk-review => .claude/tests}/fixtures/hardcode-url.diff | 0 .../sdk-review => .claude/tests}/fixtures/secrets-aws-key.diff | 0 {tests/sdk-review => .claude/tests}/fixtures/secrets-jwt.diff | 0 {tests/sdk-review => .claude/tests}/test_baseline_contract.bats | 0 {tests/sdk-review => .claude/tests}/test_extended.bats | 0 {tests/sdk-review => .claude/tests}/test_smoke.bats | 0 10 files changed, 1 insertion(+), 1 deletion(-) rename {tests/sdk-review => .claude/tests}/fixtures/binding-token-concat.diff (100%) rename {tests/sdk-review => .claude/tests}/fixtures/clean.diff (100%) rename {tests/sdk-review => .claude/tests}/fixtures/disclosure-internal.diff (100%) rename {tests/sdk-review => .claude/tests}/fixtures/hardcode-url.diff (100%) rename {tests/sdk-review => .claude/tests}/fixtures/secrets-aws-key.diff (100%) rename {tests/sdk-review => .claude/tests}/fixtures/secrets-jwt.diff (100%) rename {tests/sdk-review => .claude/tests}/test_baseline_contract.bats (100%) rename {tests/sdk-review => .claude/tests}/test_extended.bats (100%) rename {tests/sdk-review => .claude/tests}/test_smoke.bats (100%) diff --git a/.claude/scripts/lib/skill-self-skip.sh b/.claude/scripts/lib/skill-self-skip.sh index 53658845..23efa560 100755 --- a/.claude/scripts/lib/skill-self-skip.sh +++ b/.claude/scripts/lib/skill-self-skip.sh @@ -7,7 +7,7 @@ is_skill_file() { local file="$1" case "$file" in .claude/*) echo "true"; return ;; - tests/sdk-review/*) echo "true"; return ;; + tests/sdk-review/*) echo "true"; return ;; # legacy path pre-bug-8 .github/workflows/sdk-*) echo "true"; return ;; docs/PR-REVIEW.md|docs/BRANCH-PROTECTION-SETUP.md) echo "true"; return ;; *) echo "false" ;; diff --git a/tests/sdk-review/fixtures/binding-token-concat.diff b/.claude/tests/fixtures/binding-token-concat.diff similarity index 100% rename from tests/sdk-review/fixtures/binding-token-concat.diff rename to .claude/tests/fixtures/binding-token-concat.diff diff --git a/tests/sdk-review/fixtures/clean.diff b/.claude/tests/fixtures/clean.diff similarity index 100% rename from tests/sdk-review/fixtures/clean.diff rename to .claude/tests/fixtures/clean.diff diff --git a/tests/sdk-review/fixtures/disclosure-internal.diff b/.claude/tests/fixtures/disclosure-internal.diff similarity index 100% rename from tests/sdk-review/fixtures/disclosure-internal.diff rename to .claude/tests/fixtures/disclosure-internal.diff diff --git a/tests/sdk-review/fixtures/hardcode-url.diff b/.claude/tests/fixtures/hardcode-url.diff similarity index 100% rename from tests/sdk-review/fixtures/hardcode-url.diff rename to .claude/tests/fixtures/hardcode-url.diff diff --git a/tests/sdk-review/fixtures/secrets-aws-key.diff b/.claude/tests/fixtures/secrets-aws-key.diff similarity index 100% rename from tests/sdk-review/fixtures/secrets-aws-key.diff rename to .claude/tests/fixtures/secrets-aws-key.diff diff --git a/tests/sdk-review/fixtures/secrets-jwt.diff b/.claude/tests/fixtures/secrets-jwt.diff similarity index 100% rename from tests/sdk-review/fixtures/secrets-jwt.diff rename to .claude/tests/fixtures/secrets-jwt.diff diff --git a/tests/sdk-review/test_baseline_contract.bats b/.claude/tests/test_baseline_contract.bats similarity index 100% rename from tests/sdk-review/test_baseline_contract.bats rename to .claude/tests/test_baseline_contract.bats diff --git a/tests/sdk-review/test_extended.bats b/.claude/tests/test_extended.bats similarity index 100% rename from tests/sdk-review/test_extended.bats rename to .claude/tests/test_extended.bats diff --git a/tests/sdk-review/test_smoke.bats b/.claude/tests/test_smoke.bats similarity index 100% rename from tests/sdk-review/test_smoke.bats rename to .claude/tests/test_smoke.bats