diff --git a/.claude/commands/review-new-module.md b/.claude/commands/review-new-module.md new file mode 100644 index 00000000..85dde012 --- /dev/null +++ b/.claude/commands/review-new-module.md @@ -0,0 +1,19 @@ +# /review-new-module — orchestrator slash command +Run the SDK Module Review skill against a PR. + +## Usage +- `/review-new-module ` — full review, posts findings +- `/review-new-module --dry-run` — analysis only, no posting + +## What it does +1. Detects language (Python vs Java) via `pyproject.toml` / `pom.xml` +2. Fetches PR diff + body +3. Runs 20 deterministic checks in parallel (secrets, license, disclosure, hardcode, telemetry, docs, bdd, patterns, versioning, commits, errors-logging, testing-depth, http-hygiene, concurrency, deps-supply, deletion-hygiene, constants, binding-shape, quality-gate-parity, pr-size) +4. Applies baseline exemptions + scope predicates + tier gating +5. Detects breaking changes via AST diff +6. Posts 4 signals: inline comments, summary comment, check-run, label + +## Implementation +Run: `bash .claude/scripts/orchestrate.sh ` + +The orchestrator is 100% deterministic — no LLM calls in CI. When run inside Claude Code, the LLM can enrich the summary comment before posting (this happens naturally in the session). diff --git a/.claude/config/rules.yaml b/.claude/config/rules.yaml index 808541f4..6c60bb70 100644 --- a/.claude/config/rules.yaml +++ b/.claude/config/rules.yaml @@ -107,7 +107,7 @@ rules: DEP-04: { tier: BLOCK_LOCKED } # -------- COMMITS -------- - COM-01: { tier: BLOCK } + COM-01: { tier: FLAG } # -------- DELETION -------- DEL-01: { tier: BLOCK } diff --git a/.claude/scripts/aggregate.sh b/.claude/scripts/aggregate.sh new file mode 100755 index 00000000..91dbc25a --- /dev/null +++ b/.claude/scripts/aggregate.sh @@ -0,0 +1,137 @@ +#!/usr/bin/env bash +# aggregate.sh — merge N check reports into a single summary, applying rule tiers. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +TMPDIR_RUN="$1" +RULES_YAML="${RULES_YAML:-$SCRIPT_DIR/../config/rules.yaml}" + +# get_tier — reads rules.yaml, returns tier or empty +get_tier() { + local rule="$1" + # rules.yaml format: " RULE-ID: { tier: X, ... }" + awk -v rule="$rule" ' + match($0, "^ " rule ":") { + if (match($0, /tier:[[:space:]]*[A-Z_]+/)) { + t = substr($0, RSTART, RLENGTH) + sub(/^tier:[[:space:]]*/, "", t) + print t + exit + } + } + ' "$RULES_YAML" 2>/dev/null +} + +# Collect all report-*.json files. Skip files that are not valid JSON +# (e.g. a check that timed out or crashed producing partial stdout) so a +# single bad report can't take the whole aggregation down. +raw_reports=$(ls "$TMPDIR_RUN"/report-*.json 2>/dev/null | sort) +reports="" +for r in $raw_reports; do + if jq -e '.' "$r" >/dev/null 2>&1; then + reports="$reports $r" + else + echo "WARN: skipping invalid JSON report: $r" >&2 + fi +done +if [ -z "$reports" ]; then + echo '{"findings":[],"shadow_findings":[],"summary":{"block_count":0,"flag_count":0,"shadow_count":0,"locked_count":0},"per_check_summary":{}}' + exit 0 +fi + +# Merge raw findings, then re-classify each finding by tier from rules.yaml +merged=$(mktemp) +# shellcheck disable=SC2086 +jq -s '.' $reports > "$merged" + +# For each finding, look up its rule tier and adjust +retagged_findings="[]" +retagged_shadow="[]" +locked_count=0 + +n=$(jq '[.[] | .findings // [] | length] | add // 0' "$merged") +if [ "$n" -gt 0 ]; then + # Flatten findings, stamping each with its parent report's check name so the + # per-check table can count POSTED findings per check (bug-T-01: table count + # must reconcile with the findings that actually post). + all_findings=$(jq '[.[] | .check as $c | (.findings // [])[] | . + {check: $c}]' "$merged") + rule_ids=$(echo "$all_findings" | jq -r '.[] | .rule' | sort -u) + + # Build lookup: rule -> tier + tier_map='{}' + while IFS= read -r rule; do + [ -z "$rule" ] && continue + tier=$(get_tier "$rule") + [ -z "$tier" ] && tier="FLAG" # default + tier_map=$(echo "$tier_map" | jq --arg r "$rule" --arg t "$tier" '. + {($r): $t}') + done <<< "$rule_ids" + + # Split findings into posted vs shadow based on tier + retagged_findings=$(echo "$all_findings" | jq --argjson tiers "$tier_map" ' + map( + . as $f + | ($tiers[$f.rule] // "FLAG") as $tier + | if $tier == "SHADOW" then empty + elif $tier == "BLOCK_LOCKED" then . + {severity: "BLOCK", tier: "BLOCK_LOCKED", locked: true} + elif $tier == "BLOCK" then . + {severity: "BLOCK", tier: "BLOCK"} + elif $tier == "FLAG" then . + {severity: "FLAG", tier: "FLAG"} + else . + {tier: $tier} end + ) + ') + retagged_shadow=$(echo "$all_findings" | jq --argjson tiers "$tier_map" ' + map( + . as $f + | ($tiers[$f.rule] // "FLAG") as $tier + | if $tier == "SHADOW" then . + {tier: "SHADOW"} else empty end + ) + ') + locked_count=$(echo "$retagged_findings" | jq '[.[] | select(.locked == true)] | length') +fi + +block_count=$(echo "$retagged_findings" | jq '[.[] | select(.severity == "BLOCK")] | length') +flag_count=$(echo "$retagged_findings" | jq '[.[] | select(.severity == "FLAG")] | length') +shadow_count=$(echo "$retagged_shadow" | jq 'length') +# per_check_summary: count must match the POSTED findings (after tier-gating / +# suppression), not the raw per-report count. Otherwise the table shows more +# than the inline+summary comments and the numbers never reconcile. Build the +# count from retagged_findings (what actually posts); derive status from the +# surviving severities so a check whose findings were all shadowed shows PASS. +per_check=$(jq -n \ + --argjson merged "$(cat "$merged")" \ + --argjson posted "$retagged_findings" \ + ' + # Start from every check that produced a report (so PASS checks still shown). + ($merged | map(.check)) as $checks + | reduce $checks[] as $c ({}; + . + { ($c): { + count: ($posted | map(select(.check == $c)) | length), + status: ( + ($posted | map(select(.check == $c))) as $f + | if ($f | map(select(.severity=="BLOCK")) | length) > 0 then "BLOCK" + elif ($f | map(select(.severity=="FLAG")) | length) > 0 then "FLAG" + else "PASS" end) + } }) + ') + +jq -n \ + --argjson findings "$retagged_findings" \ + --argjson shadow "$retagged_shadow" \ + --argjson block "$block_count" \ + --argjson flag "$flag_count" \ + --argjson shadow_c "$shadow_count" \ + --argjson locked "$locked_count" \ + --argjson per_check "$per_check" \ + '{ + version: "1.0.0", + findings: $findings, + shadow_findings: $shadow, + summary: { + block_count: $block, + flag_count: $flag, + shadow_count: $shadow_c, + locked_count: $locked + }, + per_check_summary: $per_check + }' + +rm -f "$merged" diff --git a/.claude/scripts/check-bdd.sh b/.claude/scripts/check-bdd.sh new file mode 100755 index 00000000..791ccefe --- /dev/null +++ b/.claude/scripts/check-bdd.sh @@ -0,0 +1,136 @@ +#!/usr/bin/env bash +# check-bdd.sh — feature file existence + cross-language parity via alias map. +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" + +LANGUAGE="${LANGUAGE:-python}" +REPO_ROOT="${REPO_ROOT:-.}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +SDK_SIBLING_PATH="${SDK_SIBLING_PATH:-}" +CONFIG_DIR="${CONFIG_DIR:-$REPO_ROOT/.claude/config}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# Extract new/modified modules from diff +if [ "$LANGUAGE" = "python" ]; then + modules=$(echo "$diff_content" | grep -oE 'src/sap_cloud_sdk/[a-z_]+/' 2>/dev/null | sed 's|src/sap_cloud_sdk/||; s|/$||' | grep -v '^core$' | sort -u || true) +else + modules=$(echo "$diff_content" | grep -oE 'src/main/java/com/sap/cloud/sdk/[a-z_]+/' 2>/dev/null | sed 's|src/main/java/com/sap/cloud/sdk/||; s|/$||' | grep -v '^core$' | sort -u || true) +fi + +# resolve alias mapping (python name → java name and vice versa) +# The YAML is a simple list of pairs: +# aliases: +# - python: dms +# java: documentmanagement +# awk on `$NF` (last field) extracts the value cleanly even when the key +# column varies. We only pair a python: line with the very next java: line. +resolve_sibling_name() { + local mod="$1" dir="$LANGUAGE" + local aliases="$CONFIG_DIR/module-aliases.yaml" + if [ ! -f "$aliases" ]; then echo "$mod"; return; fi + if [ "$dir" = "python" ]; then + awk -v mod="$mod" ' + /^[[:space:]]*-?[[:space:]]*python:[[:space:]]*/ { + sub(/^[^:]*:[[:space:]]*/, ""); p_name=$0; next + } + /^[[:space:]]*java:[[:space:]]*/ { + sub(/^[^:]*:[[:space:]]*/, ""); j_name=$0 + if (p_name == mod) { print j_name; exit } + p_name="" + } + ' "$aliases" + return + else + awk -v mod="$mod" ' + /^[[:space:]]*-?[[:space:]]*python:[[:space:]]*/ { + sub(/^[^:]*:[[:space:]]*/, ""); p_name=$0; next + } + /^[[:space:]]*java:[[:space:]]*/ { + sub(/^[^:]*:[[:space:]]*/, ""); j_name=$0 + if (j_name == mod) { print p_name; exit } + p_name="" + } + ' "$aliases" + return + fi +} + +while IFS= read -r mod; do + [ -z "$mod" ] && continue + + # Path for THIS repo's feature file + # FP-C-01: BDD-01 must accept ANY .feature file under the module's integration + # dir, not just `.feature`. + if [ "$LANGUAGE" = "python" ]; then + feature_dir="$REPO_ROOT/tests/$mod/integration" + feature_path="$feature_dir/$mod.feature" # kept for BDD-02 sibling comparison + mod_dir="$REPO_ROOT/src/sap_cloud_sdk/$mod" + else + feature_dir="$REPO_ROOT/src/test/resources/com/sap/applicationfoundation/$mod/integration" + feature_path="$feature_dir/$mod.feature" # kept for BDD-02 sibling comparison + mod_dir="$REPO_ROOT/src/main/java/com/sap/cloud/sdk/$mod" + fi + + # BDD-01: at least one .feature file exists in the module's integration dir + # (only fire if module has any source files and PR creates the module). + has_any_feature=false + if [ -d "$feature_dir" ]; then + if compgen -G "$feature_dir/*.feature" > /dev/null 2>&1; then + has_any_feature=true + fi + fi + + if [ "$has_any_feature" = "false" ] && [ -d "$mod_dir" ]; then + # Detect if this PR creates any new file INSIDE the module. + # `new file mode` is on its own line before the `+++ b/` header, + # so we scan block-by-block to link them. + is_new_module=$(echo "$diff_content" | awk -v mod="$mod" -v lang="$LANGUAGE" ' + BEGIN { + if (lang == "python") pat = "src/sap_cloud_sdk/" mod "/" + else pat = "src/main/java/com/sap/cloud/sdk/" mod "/" + } + /^diff --git/ { is_new = 0; next } + /^new file mode/ { is_new = 1; next } + /^\+\+\+ b\// { + if (is_new && index($0, pat) > 0) { print "true"; exit } + } + ') + if [ "$is_new_module" = "true" ]; then + emit_finding "BDD-01" "BLOCK" "tests/$mod/integration/" 1 \ + "New module '$mod' has no .feature files under integration/" \ + "Create at least one $feature_dir/*.feature with cross-language-consistent scenarios" >> "$findings" + fi + fi + + # BDD-02: sibling repo parity + sibling_name=$(resolve_sibling_name "$mod") + [ -z "$sibling_name" ] && sibling_name="$mod" + + if [ -n "$SDK_SIBLING_PATH" ] && [ -d "$SDK_SIBLING_PATH" ]; then + if [ "$LANGUAGE" = "python" ]; then + # Python repo — sibling is Java + sibling_feature="$SDK_SIBLING_PATH/src/test/resources/com/sap/applicationfoundation/$sibling_name/integration/$sibling_name.feature" + sibling_module_dir="$SDK_SIBLING_PATH/src/main/java/com/sap/cloud/sdk/$sibling_name" + else + sibling_feature="$SDK_SIBLING_PATH/tests/$sibling_name/integration/$sibling_name.feature" + sibling_module_dir="$SDK_SIBLING_PATH/src/sap_cloud_sdk/$sibling_name" + fi + # If the sibling module dir exists, and sibling has no feature but we do (or vice versa) + if [ -d "$sibling_module_dir" ] && [ ! -f "$sibling_feature" ] && [ -f "$feature_path" ]; then + emit_finding "BDD-02" "FLAG" "$feature_path" 1 \ + "Sibling SDK ($sibling_name) has module but no BDD feature — parity broken" "" >> "$findings" + fi + if [ -d "$sibling_module_dir" ] && [ -f "$sibling_feature" ] && [ ! -f "$feature_path" ] && [ -d "$mod_dir" ]; then + emit_finding "BDD-02" "BLOCK" "tests/.../$mod.feature" 1 \ + "Module '$mod' exists in sibling SDK ($sibling_name) with BDD feature — this repo must have equivalent" "" >> "$findings" + fi + fi +done <<< "$modules" + +status=$(status_from_findings < "$findings") +emit_report "bdd" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-binding-shape.sh b/.claude/scripts/check-binding-shape.sh new file mode 100755 index 00000000..4cc98efd --- /dev/null +++ b/.claude/scripts/check-binding-shape.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# check-binding-shape.sh — placeholder: stub check emitting empty findings. +# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.binding-shape +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +REPO_ROOT="${REPO_ROOT:-.}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# BND-02 (LOCKED): token URL via string concat + "/oauth/token" +# FP-N-01: pre-filter — only lines mentioning /oauth/token can match. +# FP-O-01: skip test files (multi-module: /src/test/…) — a hardcoded +# token path in a test fixture is not production binding code. +echo "$diff_content" | awk ' + BEGIN { file=""; line=0; skip=0 } + /^diff --git a\// { + file=$4; sub(/^b\//, "", file); line=0 + skip = (file ~ /(^|\/)src\/test\// || file ~ /(^|\/)(tests?|mocks?)\//) ? 1 : 0 + next + } + /^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next } + /^\+/ && !/^\+\+\+/ { + c = substr($0, 2) + if (!skip && c ~ /\/oauth\/token/) { print file "\t" line "\t" c } + line++ + next + } + /^ / { line++; next } +' | while IFS=$'\t' read -r file line_num content; do + [ -z "$file" ] && continue + # Self-review protection: skip skill files + if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi + if echo "$content" | grep -qE 'rstrip\("/"\)[[:space:]]*\+[[:space:]]*"/oauth/token"|\.replaceAll\("/\+\$", ""\)[[:space:]]*\+[[:space:]]*"/oauth/token"'; then + emit_finding "BND-02" "BLOCK" "$file" "$line_num" \ + "BTP token URL built via string concat — different services expose different fields" \ + "Use HttpUrl.parse().newBuilder() or honour a 'token_url' field if present" >> "$findings" + fi + if echo "$content" | grep -qE '\+[[:space:]]*"/oauth/token"'; then + emit_finding "BND-02" "BLOCK" "$file" "$line_num" \ + "Hardcoded /oauth/token path — BTP services vary" "" >> "$findings" + fi +done + +status=$(status_from_findings < "$findings") +emit_report "binding-shape" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-commits.sh b/.claude/scripts/check-commits.sh new file mode 100755 index 00000000..9dcea2b7 --- /dev/null +++ b/.claude/scripts/check-commits.sh @@ -0,0 +1,42 @@ +#!/usr/bin/env bash +# check-commits.sh — Conventional Commits enforcement. +# +# The repos squash-merge PRs, so ONLY the final squashed subject (which +# defaults to the PR title) becomes a commit on main. Checking every +# intermediate commit floods the review with findings for messages that +# will be discarded. We therefore check a single subject: +# 1. PR_TITLE (env, set by orchestrate from the PR metadata) if available +# 2. else the most recent non-merge commit subject (HEAD) +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" + +LANGUAGE="${LANGUAGE:-python}" +HEAD_SHA="${HEAD_SHA:-HEAD}" +PR_TITLE="${PR_TITLE:-}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT + +prefix_regex='^(feat|fix|refactor|chore|docs|test|ci|build|perf|style|revert)(\([a-z0-9_/-]+\))?!?:[[:space:]]+.+' + +# Pick the single subject that will land on main after squash-merge. +if [ -n "$PR_TITLE" ]; then + subject="$PR_TITLE" + location="PR_TITLE" +else + # Most recent non-merge commit subject. + subject=$({ git log "$HEAD_SHA" --no-merges -1 --format='%s' 2>/dev/null || true; }) + location="COMMIT:$({ git rev-parse "$HEAD_SHA" 2>/dev/null || echo HEAD; })" +fi + +if [ -n "$subject" ] && ! echo "$subject" | grep -qiE '^Merge '; then + if ! echo "$subject" | grep -qE "$prefix_regex"; then + emit_finding "COM-01" "FLAG" "$location" 1 \ + "Squash-merge subject '$subject' does not follow Conventional Commits" \ + "The PR title becomes the squashed commit — prefix with feat:/fix:/chore:/docs:/test:/refactor:/ci:/build:/perf:/style:/revert:" >> "$findings" + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "commits" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-concurrency.sh b/.claude/scripts/check-concurrency.sh new file mode 100755 index 00000000..1ecc4043 --- /dev/null +++ b/.claude/scripts/check-concurrency.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash +# check-concurrency.sh — placeholder: stub check emitting empty findings. +# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.concurrency +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" +source "$SCRIPT_DIR/lib/hunk-filter.sh" +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +REPO_ROOT="${REPO_ROOT:-.}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# CC-01: asyncio.Queue with no accompanying Lock/set — flag when queue+append pattern present +# FP-N-01: pre-filter — only lines mentioning asyncio.Queue can match. +echo "$diff_content" | awk ' + BEGIN { file=""; line=0 } + /^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next } + /^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next } + /^\+/ && !/^\+\+\+/ { + c = substr($0, 2) + if (c ~ /asyncio\.Queue\(/) { print file "\t" line "\t" c } + line++ + next + } + /^ / { line++; next } +' | while IFS=$'\t' read -r file line_num content; do + [ -z "$file" ] && continue + if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi + if echo "$content" | grep -qE 'asyncio\.Queue\('; then + # Check if same file has a Lock or set() nearby + if [ -f "$REPO_ROOT/$file" ] && ! grep -qE 'asyncio\.Lock|set\(\)' "$REPO_ROOT/$file" 2>/dev/null; then + emit_finding_if_touched "CC-01" "FLAG" "$file" "$line_num" \ + "asyncio.Queue without dedup — if items must be unique, add a set() + asyncio.Lock" "" >> "$findings" + fi + fi +done + +status=$(status_from_findings < "$findings") +emit_report "concurrency" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-constants.sh b/.claude/scripts/check-constants.sh new file mode 100755 index 00000000..3b561a3b --- /dev/null +++ b/.claude/scripts/check-constants.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# check-constants.sh — placeholder: stub check emitting empty findings. +# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.constants +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" +source "$SCRIPT_DIR/lib/hunk-filter.sh" +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +REPO_ROOT="${REPO_ROOT:-.}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# CON-01 via AST on Python files +if [ "$LANGUAGE" = "python" ]; then + changed=$(echo "$diff_content" | grep -oE '^\+\+\+ b/src/.*\.py' | sed 's|^+++ b/||' | sort -u) + # Filter out skill files (self-review protection) + filtered="" + while IFS= read -r f; do + [ -z "$f" ] && continue + if [ "$(is_skill_file "$f")" = "true" ]; then continue; fi + filtered="$filtered $f" + done <<< "$changed" + if [ -n "$filtered" ]; then + raw_con=$(mktemp); trap 'rm -f "$raw_con"' EXIT + # shellcheck disable=SC2086 + python3 "$SCRIPT_DIR/lib/ast_python_checks.py" con-01 $filtered 2>/dev/null > "$raw_con" || true + # FP-A-01: filter to touched lines only + while IFS= read -r finding; do + [ -z "$finding" ] && continue + f=$(echo "$finding" | python3 -c "import json,sys;o=json.loads(sys.stdin.read());print(o.get('file',''))") + ln=$(echo "$finding" | python3 -c "import json,sys;o=json.loads(sys.stdin.read());print(o.get('line',0))") + [ -z "$f" ] && continue + if is_line_touched "$f" "$ln"; then + echo "$finding" >> "$findings" + fi + done < "$raw_con" + rm -f "$raw_con" + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "constants" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-deletion-hygiene.sh b/.claude/scripts/check-deletion-hygiene.sh new file mode 100755 index 00000000..f26f29c6 --- /dev/null +++ b/.claude/scripts/check-deletion-hygiene.sh @@ -0,0 +1,43 @@ +#!/usr/bin/env bash +# check-deletion-hygiene.sh — placeholder: stub check emitting empty findings. +# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.deletion-hygiene +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" +# DEL-01 uses synthetic path "src/:1" — hunk-filter is sourced for +# emit_finding_if_touched (via is_meta_finding, which treats non-file paths +# as metadata and passes them through). No filtering applied here. +source "$SCRIPT_DIR/lib/hunk-filter.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +REPO_ROOT="${REPO_ROOT:-.}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# DEL-01: symbol removed from __all__ but references remain in codebase +if [ "$LANGUAGE" = "python" ]; then + # Extract removed __all__ entries + removed_symbols=$(echo "$diff_content" | awk ' + /\+\+\+ b\/.*__init__\.py$/ { in_init=1; next } + /^diff --git/ { in_init=0 } + in_init && /^-[[:space:]]*"[A-Za-z_][A-Za-z0-9_]*"/ { + match($0, /"[^"]+"/); print substr($0, RSTART+1, RLENGTH-2) + } + ') + while IFS= read -r sym; do + [ -z "$sym" ] && continue + # search codebase (excluding the file where it was removed). + # grep -E doesn't universally honor \b — use portable (^|non-word) anchors. + hits=$({ grep -rEIln "(^|[^A-Za-z0-9_])${sym}([^A-Za-z0-9_]|$)" "$REPO_ROOT/src" 2>/dev/null || true; } | wc -l | tr -d \' \') + if [ "$hits" -gt 0 ]; then + emit_finding "DEL-01" "BLOCK" "src/" 1 \ + "Symbol '$sym' removed from __all__ but still referenced in $hits files" "" >> "$findings" + fi + done <<< "$removed_symbols" +fi + +status=$(status_from_findings < "$findings") +emit_report "deletion-hygiene" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-deps-supply.sh b/.claude/scripts/check-deps-supply.sh new file mode 100755 index 00000000..b76beed2 --- /dev/null +++ b/.claude/scripts/check-deps-supply.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash +# check-deps-supply.sh — placeholder: stub check emitting empty findings. +# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.deps-supply +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +REPO_ROOT="${REPO_ROOT:-.}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# DEP-04: internal artifactory index URL +if echo "$diff_content" | grep -qE '\+.*(index-url|extra-index-url).*int\.repositories\.cloud\.sap'; then + emit_finding "DEP-04" "BLOCK" "config" 1 \ + "Internal artifactory --index-url reference in public artifact" "" >> "$findings" +fi +# Helper: `grep -c` returns "0" + exit 1 on zero matches → || echo 0 emits +# "0\n0" and breaks -eq. Use wc -l instead. +count_lines() { echo "$1" | { grep -E "$2" 2>/dev/null || true; } | wc -l | tr -d ' '; } + +# DEP-03: pyproject.toml deps changed but uv.lock not +if [ "$LANGUAGE" = "python" ]; then + pyproject_deps_changed=$(count_lines "$diff_content" '^\+.*"[a-z][a-z0-9_-]*[><=~!]+') + uv_lock_changed=$(count_lines "$diff_content" '^\+\+\+ b/uv\.lock') + if [ "$pyproject_deps_changed" -gt 0 ] && [ "$uv_lock_changed" -eq 0 ]; then + # only fire if pyproject.toml dep table (not [project] version) changed + if echo "$diff_content" | grep -qE '^\+.*\[project\.(dependencies|optional-dependencies)\]|^\+[[:space:]]+"[a-z][a-z0-9_-]*[><=~!].*"'; then + emit_finding "DEP-03" "FLAG" "pyproject.toml" 1 \ + "pyproject.toml dependencies changed but uv.lock not updated" "" >> "$findings" + fi + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "deps-supply" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-disclosure.sh b/.claude/scripts/check-disclosure.sh new file mode 100755 index 00000000..ce9cefd2 --- /dev/null +++ b/.claude/scripts/check-disclosure.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# check-disclosure.sh — open-source disclosure hygiene. +# Detects SAP-internal URLs, ORD IDs, internal Jira, unfilled PR body templates. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/json-emit.sh +source "$SCRIPT_DIR/lib/json-emit.sh" +# shellcheck source=lib/hunk-filter.sh +source "$SCRIPT_DIR/lib/hunk-filter.sh" +# shellcheck source=lib/skill-self-skip.sh +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +PROFILE="${DISCLOSURE_PROFILE:-public}" # "public" or "internal" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +PR_BODY_FILE="${PR_BODY_FILE:-}" + +STARTED=$(now_iso) +findings=$(mktemp) +trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# Determine severity based on profile +sev_public() { if [ "$PROFILE" = "public" ]; then echo "BLOCK"; else echo "FLAG"; fi; } +sev_internal_ok() { if [ "$PROFILE" = "public" ]; then echo "BLOCK"; else echo "PASS"; fi; } + +# Scan added lines only. +# FP-N-01: pre-filter in awk. All DIS-* rules key off SAP-internal hostnames +# or --index-url. Only lines containing 'sap' (case-insensitive) or +# 'index-url' can match — everything else is skipped before the shell loop. +echo "$diff_content" | awk ' + BEGIN { file=""; line=0 } + /^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next } + /^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next } + /^\+/ && !/^\+\+\+/ { + c = substr($0, 2) + if (c ~ /[Ss][Aa][Pp]|index-url/) { print file "\t" line "\t" c } + line++ + next + } + /^ / { line++; next } +' | while IFS=$'\t' read -r file line_num content; do + [ -z "$file" ] && continue + # Self-review protection: skip skill files + if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi + + # DIS-02: Internal Jira URL (checked first — more specific) + dis02_fired=false + if echo "$content" | grep -qEi 'jira\.tools\.sap|jira\.wdf\.sap\.corp'; then + emit_finding_if_touched "DIS-02" "$(sev_public)" "$file" "$line_num" "Internal Jira URL — remove or move to internal-only docs" "" >> "$findings" + dis02_fired=true + fi + # DIS-01: SAP-internal hostnames (skip if DIS-02 already covers the same line) + if [ "$dis02_fired" = "false" ] && echo "$content" | grep -qEi '(int\.repositories\.cloud\.sap|\.tools\.sap|\.wdf\.sap\.corp|\.mo\.sap\.corp)'; then + emit_finding_if_touched "DIS-01" "$(sev_public)" "$file" "$line_num" "SAP-internal hostname detected in code" "" >> "$findings" + fi + # DIS-06: Internal artifactory index-url + if echo "$content" | grep -qEi '\-\-index-url[[:space:]]+https?://int\.repositories\.cloud\.sap'; then + emit_finding_if_touched "DIS-06" "BLOCK" "$file" "$line_num" "Internal artifactory --index-url — must not appear in public/shared configs" "" >> "$findings" + fi +done + +# DIS-07/08: PR body +if [ -n "$PR_BODY_FILE" ] && [ -f "$PR_BODY_FILE" ]; then + body=$(cat "$PR_BODY_FILE") + # DIS-07: unfilled Closes # placeholder + if echo "$body" | grep -qE 'Closes #'; then + emit_finding "DIS-07" "BLOCK" "PR_BODY" 1 "PR body contains unfilled 'Closes #' placeholder" "" >> "$findings" + fi + # DIS-08 (SHADOW): internal URLs / Jira in PR body + if echo "$body" | grep -qEi '\.tools\.sap|\.wdf\.sap\.corp|jira\.tools\.sap'; then + emit_finding "DIS-08" "FLAG" "PR_BODY" 1 "PR body references SAP-internal URLs/Jira — remove from public-visible artifacts" "" >> "$findings" + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "disclosure" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-docs.sh b/.claude/scripts/check-docs.sh new file mode 100755 index 00000000..556d37fe --- /dev/null +++ b/.claude/scripts/check-docs.sh @@ -0,0 +1,114 @@ +#!/usr/bin/env bash +# check-docs.sh — documentation completeness including BTP deps and regional availability. +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" + +LANGUAGE="${LANGUAGE:-python}" +REPO_ROOT="${REPO_ROOT:-.}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# Detect new module directories (module has new files at top-level) +# FP-M-01: multi-module Maven nests sources under /src/main/java. +# Match the module segment after .../com/sap/cloud/sdk/ at any prefix depth. +if [ "$LANGUAGE" = "python" ]; then + new_modules=$(echo "$diff_content" | { grep -oE '^\+\+\+ b/src/sap_cloud_sdk/[a-z_]+/[^/]+\.py' 2>/dev/null || true; } | sed 's|^+++ b/src/sap_cloud_sdk/||; s|/[^/]*\.py$||' | sort -u) +else + new_modules=$(echo "$diff_content" | { grep -oE '^\+\+\+ b/.*src/main/java/com/sap/cloud/sdk/[a-z_]+/[^/]+\.java' 2>/dev/null || true; } | sed -E 's|^.*com/sap/cloud/sdk/||; s|/[^/]*\.java$||' | sort -u) +fi + +while IFS= read -r mod; do + [ -z "$mod" ] && continue + [ "$mod" = "core" ] && continue + + if [ "$LANGUAGE" = "python" ]; then + user_guide="$REPO_ROOT/src/sap_cloud_sdk/$mod/user-guide.md" + mod_dir="$REPO_ROOT/src/sap_cloud_sdk/$mod" + else + user_guide="$REPO_ROOT/src/main/java/com/sap/cloud/sdk/$mod/user-guide.md" + mod_dir="$REPO_ROOT/src/main/java/com/sap/cloud/sdk/$mod" + fi + + # DC-01: user-guide.md exists + if [ ! -f "$user_guide" ]; then + # Only fire if module dir exists (module is new-ish) + if [ -d "$mod_dir" ]; then + emit_finding "DC-01" "BLOCK" "src/.../$mod/user-guide.md" 1 \ + "Module '$mod' missing user-guide.md" \ + "Create $user_guide with sections: ## Installation, ## Quick Start, ## Configuration" >> "$findings" + fi + continue + fi + + # DC-02: required sections + # Report the repo-relative path (strip $REPO_ROOT/ prefix) so findings don't + # leak the reviewer's local absolute path into the PR comment. + guide_rel="${user_guide#"$REPO_ROOT"/}" + guide_content=$(cat "$user_guide" 2>/dev/null || echo "") + if ! echo "$guide_content" | grep -qE '^##[[:space:]]+(Installation|Import)'; then + emit_finding "DC-02" "FLAG" "$guide_rel" 1 "user-guide.md missing ## Installation section" "" >> "$findings" + fi + if ! echo "$guide_content" | grep -qE '^##[[:space:]]+Quick Start'; then + emit_finding "DC-02" "BLOCK" "$guide_rel" 1 "user-guide.md missing ## Quick Start section" "" >> "$findings" + fi + + # DC-11..DC-14 (BTP deps + regional) + # Detect module imports/usages. Use word boundaries and prefer explicit + # SAP-namespaced references to avoid false positives on DocumentFragment, + # AWSRegion, region_id inside docstrings, etc. + if [ "$LANGUAGE" = "python" ]; then + has_dest=$(grep -rq "from sap_cloud_sdk\.destination" "$mod_dir" 2>/dev/null && echo yes || echo no) + # Fragment/Certificate: require the SDK-provided client class specifically, + # or an import from the SDK's fragments/certificates subpackage. + has_frag=$(grep -rqE "\bFragmentClient\b|from[[:space:]]+sap_cloud_sdk\.fragments" "$mod_dir" 2>/dev/null && echo yes || echo no) + has_cert=$(grep -rqE "\bCertificateClient\b|from[[:space:]]+sap_cloud_sdk\.certificates" "$mod_dir" 2>/dev/null && echo yes || echo no) + # Region: constant naming or SDK-provided helper only — plain 'region_id' + # in docstrings should not fire. + has_region=$(grep -rqE "\bSUPPORTED_REGIONS\b|\bSupportedRegion\b|\bavailable_regions\b|from[[:space:]]+sap_cloud_sdk\.regions" "$mod_dir" 2>/dev/null && echo yes || echo no) + else + has_dest=$(grep -rq "com\.sap\.cloud\.sdk\.destination" "$mod_dir" 2>/dev/null && echo yes || echo no) + has_frag=$(grep -rqE "\bFragmentClient\b|com\.sap\.cloud\.sdk\.fragments" "$mod_dir" 2>/dev/null && echo yes || echo no) + has_cert=$(grep -rqE "\bCertificateClient\b|com\.sap\.cloud\.sdk\.certificates" "$mod_dir" 2>/dev/null && echo yes || echo no) + # Java word-boundary: require SAP SDK Region type; avoid AWSRegion etc. + has_region=$(grep -rqE "\bSupportedRegion\b|com\.sap\.cloud\.sdk\.regions|\bREGIONAL_AVAILABILITY\b" "$mod_dir" 2>/dev/null && echo yes || echo no) + fi + + # DC-11: destination dep must be documented + if [ "$has_dest" = "yes" ]; then + if ! echo "$guide_content" | grep -qEi 'destination service|## Dependencies|## Prerequisites'; then + emit_finding "DC-11" "BLOCK" "$guide_rel" 1 \ + "Module imports destination service — must document in ## Dependencies section" \ + "Add: ## Dependencies\\n- SAP BTP Destination Service instance" >> "$findings" + fi + fi + # DC-12: fragments + if [ "$has_frag" = "yes" ]; then + if ! echo "$guide_content" | grep -qEi 'fragments'; then + emit_finding "DC-12" "BLOCK" "$guide_rel" 1 \ + "Module uses Fragments — must document Fragments prerequisite" "" >> "$findings" + fi + fi + # DC-13: certificates + if [ "$has_cert" = "yes" ]; then + if ! echo "$guide_content" | grep -qEi 'certificate'; then + emit_finding "DC-13" "BLOCK" "$guide_rel" 1 \ + "Module uses Certificates — must document Certificate prerequisite" "" >> "$findings" + fi + fi + # DC-14: regional + if [ "$has_region" = "yes" ]; then + if ! echo "$guide_content" | grep -qEi 'Regional Availability|## Limitations|available in|supported region'; then + emit_finding "DC-14" "BLOCK" "$guide_rel" 1 \ + "Module has region-specific constants — must document ## Regional Availability" "" >> "$findings" + fi + fi + +done <<< "$new_modules" + +status=$(status_from_findings < "$findings") +emit_report "docs" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-errors-logging.sh b/.claude/scripts/check-errors-logging.sh new file mode 100755 index 00000000..cedb03f4 --- /dev/null +++ b/.claude/scripts/check-errors-logging.sh @@ -0,0 +1,60 @@ +#!/usr/bin/env bash +# check-errors-logging.sh — exception chaining, log level, sensitive-info in messages. +# Uses AST for chaining/swallow checks. +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" +source "$SCRIPT_DIR/lib/hunk-filter.sh" +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +REPO_ROOT="${REPO_ROOT:-.}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +if [ "$LANGUAGE" = "python" ]; then + changed=$(echo "$diff_content" | grep -oE '^\+\+\+ b/src/.*\.py' | sed 's|^+++ b/||' | sort -u) + if [ -n "$changed" ]; then + # AST-based chaining / swallow — only reports FLAGs when body ends non-Raise + # FP-A-01: filter AST hits through hunk attribution + raw_el=$(mktemp); trap 'rm -f "$raw_el"' EXIT + # shellcheck disable=SC2086 + python3 "$SCRIPT_DIR/lib/ast_python_checks.py" el-01 $changed 2>/dev/null > "$raw_el" || true + # shellcheck disable=SC2086 + python3 "$SCRIPT_DIR/lib/ast_python_checks.py" el-02 $changed 2>/dev/null >> "$raw_el" || true + while IFS= read -r finding; do + [ -z "$finding" ] && continue + f=$(echo "$finding" | python3 -c "import json,sys;o=json.loads(sys.stdin.read());print(o.get('file',''))") + ln=$(echo "$finding" | python3 -c "import json,sys;o=json.loads(sys.stdin.read());print(o.get('line',0))") + [ -z "$f" ] && continue + if is_line_touched "$f" "$ln"; then + echo "$finding" >> "$findings" + fi + done < "$raw_el" + rm -f "$raw_el" + fi + + # EL-04: secret-like variable name in raise args (grep-based, added lines only) + echo "$diff_content" | awk ' + BEGIN { file=""; line=0 } + /^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next } + /^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next } + /^\+/ && !/^\+\+\+/ { print file "\t" line "\t" substr($0, 2); line++; next } + /^ / { line++; next } + ' | while IFS=$'\t' read -r file line_num content; do + [ -z "$file" ] && continue + if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi + # match raise ...({...token|secret|password|api_key|client_secret...}) + if echo "$content" | grep -qE 'raise[[:space:]].*[fF]?"[^"]*\{[^}]*(token|secret|password|api_key|client_secret)[^}]*\}'; then + emit_finding_if_touched "EL-04" "BLOCK" "$file" "$line_num" \ + "Exception message includes secret-like variable — leaks sensitive data" "" >> "$findings" + fi + done +fi + +status=$(status_from_findings < "$findings") +emit_report "errors-logging" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-hardcode.sh b/.claude/scripts/check-hardcode.sh new file mode 100755 index 00000000..599d9489 --- /dev/null +++ b/.claude/scripts/check-hardcode.sh @@ -0,0 +1,127 @@ +#!/usr/bin/env bash +# check-hardcode.sh — no hardcoded URLs, credentials, or magic values in impl code. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/json-emit.sh +source "$SCRIPT_DIR/lib/json-emit.sh" +# shellcheck source=lib/hunk-filter.sh +source "$SCRIPT_DIR/lib/hunk-filter.sh" +# shellcheck source=lib/skill-self-skip.sh +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" + +STARTED=$(now_iso) +findings=$(mktemp) +trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# Determine ignore patterns per language +# FP-B-01: HC-01 must not fire on XML/YAML/POM files where URL-like strings +# are namespace declarations (http://maven.apache.org/POM/4.0.0 etc.). +# FP-H-01: lockfiles are generated + full of package URLs; never scan them. +# FP-I-01: .env.example files carry placeholder URLs (your-…-here); templates. +LOCKFILE_PATTERNS='.*\.lock$|(.*/)?uv\.lock$|(.*/)?poetry\.lock$|(.*/)?Pipfile\.lock$|(.*/)?package-lock\.json$|(.*/)?yarn\.lock$|(.*/)?Cargo\.lock$|(.*/)?Gemfile\.lock$' +# .env, .env.X, .env_X, .env-X — any file whose basename starts with .env +ENV_EXAMPLE_PATTERNS='(.*/)?\.env(\..*|_.*|-.*)?$' +if [ "$LANGUAGE" = "python" ]; then + ignore_files="^(tests?/|.*/tests?/|mocks?/|docs?/|.*/spec/|.*/constants\.py|.*/user-guide\.md|README\.md|.*\.md$|.*\.ya?ml$|.*\.xml$|.*\.properties$|.*pom\.xml$|${LOCKFILE_PATTERNS}|${ENV_EXAMPLE_PATTERNS})" +else + # FP-O-01: multi-module Maven puts tests at /src/test/java, not + # src/test/. Match src/test/ at any depth. Same for mocks/constants. + ignore_files="^(src/test/|.*/src/test/|mocks?/|docs?/|.*Constants\.java|.*/constants/|.*/user-guide\.md|README\.md|.*\.md$|.*\.ya?ml$|.*\.xml$|.*\.properties$|.*pom\.xml$|${LOCKFILE_PATTERNS}|${ENV_EXAMPLE_PATTERNS})" +fi + +# FP-N-01: performance. A per-line shell loop over the full added-line set +# forks 5-6 subprocesses per line; on a 13k-line diff that is ~80k forks and +# blows past the orchestrate timeout. Pre-filter in awk so ONLY lines that +# could match some HC-* rule reach the shell loop. The awk stage also does +# the file-level ignore so we never even emit lines from ignored files. +# +# Interesting-line heuristic (broad — real rule regexes still run in-loop): +# - contains http:// or https:// (HC-01) +# - contains "Authorization" or "Bearer" (HC-02) +# - contains os.environ[ or System.getenv( (HC-04) +# - contains timeout/retries/max_retries = (HC-06) +echo "$diff_content" | awk -v ignore="$ignore_files" ' + BEGIN { file=""; line=0 } + /^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next } + /^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next } + /^\+/ && !/^\+\+\+/ { + c = substr($0, 2) + # File-level ignore (awk regex; mirrors the shell ignore_files pattern) + if (file ~ ignore) { line++; next } + # Interesting-line pre-filter + if (c ~ /https?:\/\// || \ + c ~ /Authorization|Bearer/ || \ + c ~ /os\.environ\[|System\.getenv\(/ || \ + c ~ /(timeout|retries|max_retries)[ \t]*=[ \t]*[0-9]/) { + print file "\t" line "\t" c + } + line++ + next + } + /^ / { line++; next } +' | while IFS=$'\t' read -r file line_num content; do + [ -z "$file" ] && continue + if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi + + # FP-R-01: skip URLs that live inside a Python docstring (documentation + # examples, not runtime code). Compute the docstring line set once per file. + if [ "$LANGUAGE" = "python" ] && echo "$file" | grep -q '\.py$'; then + if [ "${_docstring_file:-}" != "$file" ]; then + _docstring_file="$file" + _docstring_lines="" + if [ -f "$REPO_ROOT/$file" ]; then + _docstring_lines=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" docstring-lines "$REPO_ROOT/$file" 2>/dev/null || echo "") + fi + fi + if [ -n "$_docstring_lines" ] && echo "$_docstring_lines" | grep -qx "$line_num"; then + continue + fi + fi + + # HC-01: hardcoded URL. Extract each URL and check individually so a line + # with both example.com (allowed) and api.com (real) still fires on the real one. + # Use word boundary via (^|[^A-Za-z0-9]) so we don't match tokens inside identifiers. + while IFS= read -r url; do + [ -z "$url" ] && continue + # allow-list: only IANA-reserved test/example TLDs and localhost + # `.example` must be the terminal label (RFC 2606) — anchor at path/port/end. + # FP-B-01: also allowlist standard XML/POM/W3C namespace URLs which appear + # as identifiers in build files, not as network endpoints. + if echo "$url" | grep -qE '^https?://(localhost|127\.0\.0\.1|example\.(com|org|net)|[^/]+\.example(/|:|$)|reserved\.)'; then + continue + fi + if echo "$url" | grep -qE '^https?://(maven\.apache\.org/POM/|www\.w3\.org/[0-9]+/XMLSchema|schemas\.xmlsoap\.org/)'; then + continue + fi + emit_finding_if_touched "HC-01" "BLOCK" "$file" "$line_num" "Hardcoded URL '$url' in implementation — externalize to config" "" >> "$findings" + break # only one finding per line to avoid duplicate reports + done < <(echo "$content" | grep -oE 'https?://[A-Za-z0-9][A-Za-z0-9._~:/?#@!$&*+,;=%-]*' || true) + # HC-02: Authorization Bearer + if echo "$content" | grep -qE 'Authorization[[:space:]]*:[[:space:]]*Bearer[[:space:]]+[A-Za-z0-9]'; then + emit_finding_if_touched "HC-02" "BLOCK" "$file" "$line_num" "Hardcoded Authorization header value" "" >> "$findings" + fi + # HC-04: direct os.environ / System.getenv + if [ "$LANGUAGE" = "python" ]; then + if echo "$content" | grep -qE 'os\.environ\["[A-Z]'; then + emit_finding_if_touched "HC-04" "FLAG" "$file" "$line_num" "Direct os.environ access — prefer secret_resolver / config layer" "" >> "$findings" + fi + else + if echo "$content" | grep -qE 'System\.getenv\('; then + emit_finding_if_touched "HC-04" "FLAG" "$file" "$line_num" "Direct System.getenv() — prefer SecretResolver" "" >> "$findings" + fi + fi + # HC-06: hardcoded timeout numeric literal + # Use word boundary via (^|[^A-Za-z0-9_]) so 'default_timeout' or 'my_timeout' don't match + if echo "$content" | grep -qiE '(^|[^A-Za-z0-9_])(timeout|retries|max_retries)[[:space:]]*=[[:space:]]*[0-9]+'; then + emit_finding_if_touched "HC-06" "FLAG" "$file" "$line_num" "Hardcoded timeout/retry number — externalize to config" "" >> "$findings" + fi +done + +status=$(status_from_findings < "$findings") +emit_report "hardcode" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-http-hygiene.sh b/.claude/scripts/check-http-hygiene.sh new file mode 100755 index 00000000..255c7298 --- /dev/null +++ b/.claude/scripts/check-http-hygiene.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash +# check-http-hygiene.sh — placeholder: stub check emitting empty findings. +# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.http-hygiene +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" +source "$SCRIPT_DIR/lib/hunk-filter.sh" +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +REPO_ROOT="${REPO_ROOT:-.}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# HTTP-PY-01: Session per invocation (only fire on newly added lines) +# FP-N-01: pre-filter — only lines that create a session/client can match, +# and skip test/mock/doc/example/markdown files right in awk. +echo "$diff_content" | awk ' + BEGIN { file=""; line=0; skip=0 } + /^diff --git a\// { + file=$4; sub(/^b\//, "", file); line=0 + # File-level skip: tests/mocks/docs/examples (any depth) + md/rst/txt + skip = (file ~ /(^|\/)(tests?|mocks?|docs?|examples?)\// || file ~ /\.(md|rst|txt)$/) ? 1 : 0 + next + } + /^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next } + /^\+/ && !/^\+\+\+/ { + c = substr($0, 2) + if (!skip && c ~ /(requests|httpx)\.(Session|AsyncClient)\(\)/) { + print file "\t" line "\t" c + } + line++ + next + } + /^ / { line++; next } +' | while IFS=$'\t' read -r file line_num content; do + [ -z "$file" ] && continue + if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi + if echo "$content" | grep -qE '(requests|httpx)\.(Session|AsyncClient)\(\)'; then + # FP-S-01: a Session created in __init__ is the RECOMMENDED pattern, not a + # finding. Use AST to confirm this line is a per-invocation session (i.e. + # created outside __init__). Only fire if the line is in that set. When the + # file isn't on disk (no REPO_ROOT context), fall back to firing. + if [ "$LANGUAGE" = "python" ] && echo "$file" | grep -q '\.py$' && [ -f "$REPO_ROOT/$file" ]; then + if [ "${_http_file:-}" != "$file" ]; then + _http_file="$file" + _http_lines=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" http-session-lines "$REPO_ROOT/$file" 2>/dev/null || echo "") + fi + if ! echo "$_http_lines" | grep -qx "$line_num"; then + continue # session is in __init__ (or class/module level) → correct pattern + fi + fi + emit_finding_if_touched "HTTP-01" "FLAG" "$file" "$line_num" \ + "HTTP session created per invocation — prefer single instance in __init__" "" >> "$findings" + fi +done + +status=$(status_from_findings < "$findings") +emit_report "http-hygiene" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-license-spdx.sh b/.claude/scripts/check-license-spdx.sh new file mode 100755 index 00000000..c213fbf5 --- /dev/null +++ b/.claude/scripts/check-license-spdx.sh @@ -0,0 +1,151 @@ +#!/usr/bin/env bash +# check-license-spdx.sh — verify new source files have SPDX headers. +# REUSE.toml aggregate → rule PASSES (baseline exemption). +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/json-emit.sh +source "$SCRIPT_DIR/lib/json-emit.sh" +# shellcheck source=lib/predicates.sh +source "$SCRIPT_DIR/lib/predicates.sh" + +LANGUAGE="${LANGUAGE:-python}" +REPO_ROOT="${REPO_ROOT:-.}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" + +STARTED=$(now_iso) +findings=$(mktemp) +trap 'rm -f "$findings"' EXIT + +# Predicate: REUSE.toml aggregate present? +reuse_present=$(reuse_toml_aggregate_present "$REPO_ROOT") + +# FP-J-01: if the repo itself doesn't consistently have SPDX headers, don't +# penalize new files for a missing header that no existing file has. Sample +# up to 20 existing source files of the target language. If < 20% carry an +# SPDX-License-Identifier header, we treat LIC-01/02 as SHADOW (report but +# do not block). +repo_has_spdx="unknown" +if [ "$reuse_present" != "true" ]; then + if [ "$LANGUAGE" = "python" ]; then + src_root="$REPO_ROOT/src" + ext="py" + else + # Multi-module Maven: sources live under /src/main/java, so scan + # the whole repo for .java rather than a fixed src/main/java root. + src_root="$REPO_ROOT" + ext="java" + fi + if [ -d "$src_root" ]; then + total=0; with_spdx=0 + # Sample up to 20 files. Read them into an array first so no pipe stays + # open when we stop early (avoids SIGPIPE / exit 141 under pipefail). + sample_files=$(find "$src_root" -type f -name "*.${ext}" 2>/dev/null | head -20 || true) + while IFS= read -r f; do + [ -z "$f" ] && continue + total=$((total+1)) + if head -10 "$f" 2>/dev/null | grep -q "SPDX-License-Identifier"; then + with_spdx=$((with_spdx+1)) + fi + done <<< "$sample_files" + if [ "$total" -gt 0 ]; then + # Percent threshold: <20% adoption means the repo hasn't converged yet + if [ $((with_spdx * 100 / total)) -lt 20 ]; then + repo_has_spdx="no-consistent-adoption" + else + repo_has_spdx="yes" + fi + fi + fi +fi + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +if [ "$reuse_present" = "true" ]; then + # Baseline exemption: rule OFF for this repo. Emit a valid report matching + # the JSON contract in 00-COMMON.md §3 (aggregators expect these fields). + jq -n --arg check "license-spdx" --arg lang "$LANGUAGE" --arg started "$STARTED" \ + '{ + check: $check, + version: "1.0.0", + language: $lang, + status: "PASS", + started_at: $started, + duration_ms: 0, + modules_analysed: [], + findings: [], + summary: { + block_count: 0, + flag_count: 0, + suppressed_by_baseline: 0, + pass_criteria_met: ["REUSE.toml aggregate present — LIC-01/02 exempted"], + pass_criteria_failed: [] + } + }' + exit 0 +fi + +# Find newly added files (starts with "diff --git a/... b/..." followed by "new file mode") +# We match on "+++ b/" lines that appear right after "new file mode" +added_files=$(echo "$diff_content" | awk ' + /^diff --git/ { in_block=1; is_new=0; path=""; next } + in_block && /^new file mode/ { is_new=1; next } + in_block && /^\+\+\+ b\// { if (is_new) print substr($0, 7); in_block=0 } +') + +# REUSE-IgnoreStart +if [ "$LANGUAGE" = "python" ]; then + ext_match='\.py$' + spdx_line='# SPDX-License-Identifier: Apache-2.0' + cprt_line='# SPDX-FileCopyrightText:' +else + ext_match='\.java$' + spdx_line='// SPDX-License-Identifier: Apache-2.0' + cprt_line='// SPDX-FileCopyrightText:' +fi +# REUSE-IgnoreEnd + +# FP-J-01: if the repo hasn't converged on SPDX headers, downgrade LIC-01/02 +# emissions to a single summary FLAG (or skip entirely) rather than BLOCKing +# each new file for a debt the repo itself carries. +if [ "$repo_has_spdx" = "no-consistent-adoption" ]; then + jq -n --arg check "license-spdx" --arg lang "$LANGUAGE" --arg started "$STARTED" \ + '{ + check: $check, + version: "1.0.0", + language: $lang, + status: "PASS", + started_at: $started, + duration_ms: 0, + modules_analysed: [], + findings: [], + summary: { + block_count: 0, + flag_count: 0, + suppressed_by_baseline: 0, + pass_criteria_met: ["Repo has no consistent SPDX adoption (<20% of existing files) — LIC-01/02 held until repo-wide migration"], + pass_criteria_failed: [] + } + }' + exit 0 +fi + +while IFS= read -r f; do + [ -z "$f" ] && continue + if ! echo "$f" | grep -qE "$ext_match"; then continue; fi + # Read first 10 lines from the diff for that file to check headers + header=$(echo "$diff_content" | awk -v file="$f" ' + $0 == "+++ b/" file { flag=1; count=0; next } + flag && /^\+/ && !/^\+\+\+/ { print substr($0, 2); count++; if (count>=10) exit } + flag && /^diff --git/ { exit } + ') + if ! echo "$header" | grep -qF "$spdx_line"; then + emit_finding "LIC-01" "BLOCK" "$f" 1 "New source file missing SPDX-License-Identifier header" "" >> "$findings" + fi + if ! echo "$header" | grep -qF "$cprt_line"; then + emit_finding "LIC-02" "BLOCK" "$f" 1 "New source file missing SPDX-FileCopyrightText header" "" >> "$findings" + fi +done <<< "$added_files" + +status=$(status_from_findings < "$findings") +emit_report "license-spdx" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-patterns.sh b/.claude/scripts/check-patterns.sh new file mode 100755 index 00000000..1e6a9165 --- /dev/null +++ b/.claude/scripts/check-patterns.sh @@ -0,0 +1,118 @@ +#!/usr/bin/env bash +# check-patterns.sh — idiomatic patterns (factory, exceptions, py.typed). +# Uses module_shape predicate to skip non-client modules. +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" +source "$SCRIPT_DIR/lib/predicates.sh" +source "$SCRIPT_DIR/lib/hunk-filter.sh" +source "$SCRIPT_DIR/lib/baseline.sh" +source "$SCRIPT_DIR/lib/peer-consistency.sh" + +LANGUAGE="${LANGUAGE:-python}" +REPO_ROOT="${REPO_ROOT:-.}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# Detect module dirs touched +if [ "$LANGUAGE" = "python" ]; then + modules=$(echo "$diff_content" | grep -oE 'src/sap_cloud_sdk/[a-z_]+/' | sed 's|src/sap_cloud_sdk/||; s|/$||' | grep -v '^core$' | sort -u) +else + modules=$(echo "$diff_content" | grep -oE 'src/main/java/com/sap/cloud/sdk/[a-z_]+/' | sed 's|src/main/java/com/sap/cloud/sdk/||; s|/$||' | grep -v '^core$' | sort -u) +fi + +while IFS= read -r mod; do + [ -z "$mod" ] && continue + + if [ "$LANGUAGE" = "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 + + [ -d "$mod_dir" ] || continue + + # PY-PT-01 / JV-PT-01: peer-consistency check (FP-G-01). + # Previously: BLOCK if client module missing create_client()/ClientFactory. + # Empirically wrong — only `destination` follows that shape. Now: FLAG only + # when the module diverges from an element that ≥80% of peers adopt. + # Tier: FLAG (never BLOCK). + shape=$(module_shape "$mod_dir") + if [ "$shape" = "client" ]; then + for element in factory client config user-guide.md exceptions py.typed; do + # py.typed only applies to Python + if [ "$element" = "py.typed" ] && [ "$LANGUAGE" != "python" ]; then continue; fi + # exceptions is checked separately below with AST for accuracy + if [ "$element" = "exceptions" ]; then continue; fi + if should_flag_peer_divergence "$mod_dir" "$element" "$LANGUAGE" "0.80"; then + if [ "$LANGUAGE" = "python" ]; then + rule_id="PY-PT-01" + rel="src/sap_cloud_sdk/$mod/" + else + rule_id="JV-PT-01" + rel="src/main/java/com/sap/cloud/sdk/$mod/" + fi + emit_finding "$rule_id" "FLAG" "$rel" 1 \ + "Module '$mod' diverges from peer convention: missing '$element' (≥80% of peer modules have it)" \ + "Consider adding $element for consistency with sibling modules" >> "$findings" + fi + done + fi + + # PY-PT-03: py.typed marker + if [ "$LANGUAGE" = "python" ]; then + if [ ! -f "$mod_dir/py.typed" ] && [ ! -f "$REPO_ROOT/src/sap_cloud_sdk/py.typed" ]; then + emit_finding "PY-PT-03" "FLAG" "src/sap_cloud_sdk/$mod/py.typed" 1 \ + "Module missing py.typed marker (PEP 561)" "" >> "$findings" + fi + fi + + # PY-PT-04 / JV-PT-05: module-specific exceptions + # FP-C-02: pass if module has ANY class subclassing Exception (or *Error/*Exception), + # regardless of whether it lives in exceptions.py or __init__.py or elsewhere. + if [ "$LANGUAGE" = "python" ]; then + if ! python3 "$SCRIPT_DIR/lib/ast_python_checks.py" pt-04 "$mod_dir" 2>/dev/null; then + emit_finding "PY-PT-04" "FLAG" "src/sap_cloud_sdk/$mod/exceptions.py" 1 \ + "Module lacks Exception subclasses — define a module-specific exception hierarchy (in exceptions.py or __init__.py)" "" >> "$findings" + fi + else + if [ ! -d "$mod_dir/exceptions" ]; then + emit_finding "JV-PT-05" "FLAG" "src/main/java/com/sap/cloud/sdk/$mod/exceptions/" 1 \ + "Module lacks exceptions/ package" "" >> "$findings" + fi + fi + +done <<< "$modules" + +# PY-PT-08 via AST on changed Python files +# FP-A-01: filter by ADDED_LINES_FILE (only fire on functions declared on lines the PR touched) +# FP-E-01: consult line-level baseline for pre-existing PT-08 debt +if [ "$LANGUAGE" = "python" ]; then + changed_py=$(echo "$diff_content" | grep -oE '^\+\+\+ b/src/sap_cloud_sdk/.*\.py' | sed 's|^+++ b/||' | grep -v '__pycache__' | sort -u) + if [ -n "$changed_py" ]; then + raw_pt08=$(mktemp); trap 'rm -f "$raw_pt08"' EXIT + # shellcheck disable=SC2086 + python3 "$SCRIPT_DIR/lib/ast_python_checks.py" pt-08 $changed_py 2>/dev/null > "$raw_pt08" || true + # Filter: keep only findings on lines touched by this PR AND not in baseline + while IFS= read -r finding; do + [ -z "$finding" ] && continue + f=$(echo "$finding" | python3 -c "import json,sys;o=json.loads(sys.stdin.read());print(o.get('file',''))") + ln=$(echo "$finding" | python3 -c "import json,sys;o=json.loads(sys.stdin.read());print(o.get('line',0))") + [ -z "$f" ] && continue + # Baseline check first (bypasses hunk filter — always suppressed) + if is_in_line_baseline "PY-PT-08" "$f" "$ln"; then continue; fi + # Hunk filter + if is_line_touched "$f" "$ln"; then + echo "$finding" >> "$findings" + fi + done < "$raw_pt08" + rm -f "$raw_pt08" + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "patterns" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-pr-size.sh b/.claude/scripts/check-pr-size.sh new file mode 100755 index 00000000..4d2e9a5b --- /dev/null +++ b/.claude/scripts/check-pr-size.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# check-pr-size.sh — advisory on large PRs (all FLAG tier, initially SHADOW). +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +# Resolve BASE_SHA from the PR base ref rather than HEAD~10. +if [ -z "${BASE_SHA:-}" ]; then + base_ref="${GITHUB_BASE_REF:-main}" + if git rev-parse --verify "origin/${base_ref}" >/dev/null 2>&1; then + BASE_SHA=$(git merge-base HEAD "origin/${base_ref}" 2>/dev/null || echo "HEAD~10") + elif git rev-parse --verify "$base_ref" >/dev/null 2>&1; then + BASE_SHA=$(git merge-base HEAD "$base_ref" 2>/dev/null || echo "HEAD~10") + else + BASE_SHA="HEAD~10" + fi +fi +HEAD_SHA="${HEAD_SHA:-HEAD}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# Helper: `grep -c` returns "0" AND exit 1 on no match. Under `set -e` / +# pipefail the || echo 0 idiom concatenates both, producing "0\n0" and +# breaking arithmetic. Route through wc -l instead. +count_lines() { echo "$1" | { grep -E "$2" 2>/dev/null || true; } | wc -l | tr -d ' '; } + +# PR-SIZE-01: additions > 800 +additions=$(count_lines "$diff_content" '^\+[^+]') +if [ "$additions" -gt 800 ]; then + emit_finding "PR-SIZE-01" "FLAG" "." 1 \ + "PR has $additions additions (>800) — consider splitting into stacked PRs for easier review" \ + "See docs/CONTRIBUTING.md § Incremental Delivery for stacked-PR workflow" >> "$findings" +fi + +# PR-SIZE-02: > 15 files +files_touched=$(count_lines "$diff_content" '^diff --git') +if [ "$files_touched" -gt 15 ]; then + emit_finding "PR-SIZE-02" "FLAG" "." 1 \ + "PR touches $files_touched files (>15) — consider splitting by concern" "" >> "$findings" +fi + +# PR-SIZE-03: > 3 modules +if [ "$LANGUAGE" = "python" ]; then + mods_count=$(echo "$diff_content" | { grep -oE 'src/sap_cloud_sdk/[a-z_]+/' 2>/dev/null || true; } | sed 's|src/sap_cloud_sdk/||; s|/$||' | sort -u | wc -l | tr -d ' ') +else + mods_count=$(echo "$diff_content" | { grep -oE 'src/main/java/com/sap/cloud/sdk/[a-z_]+/' 2>/dev/null || true; } | sed 's|src/main/java/com/sap/cloud/sdk/||; s|/$||' | sort -u | wc -l | tr -d ' ') +fi +if [ "$mods_count" -gt 3 ]; then + emit_finding "PR-SIZE-03" "FLAG" "." 1 \ + "PR modifies $mods_count modules (>3) — consider one PR per module" "" >> "$findings" +fi + +# PR-SIZE-05: > 30 commits (exclude merge commits by looking at parents) +commit_count=$({ git log "${BASE_SHA}..${HEAD_SHA}" --no-merges --oneline 2>/dev/null || true; } | wc -l | tr -d ' ') +if [ "$commit_count" -gt 30 ]; then + emit_finding "PR-SIZE-05" "FLAG" "." 1 \ + "PR has $commit_count commits (>30) — consider squashing or splitting" "" >> "$findings" +fi + +status=$(status_from_findings < "$findings") +emit_report "pr-size" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-quality-gate-parity.sh b/.claude/scripts/check-quality-gate-parity.sh new file mode 100755 index 00000000..05012231 --- /dev/null +++ b/.claude/scripts/check-quality-gate-parity.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash +# check-quality-gate-parity.sh — placeholder: stub check emitting empty findings. +# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.quality-gate-parity +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +REPO_ROOT="${REPO_ROOT:-.}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# QG-01: workflow with `ruff check` but no `ruff format --check` or `ty check` +if echo "$diff_content" | grep -qE '^\+\+\+ b/\.github/workflows/'; then + wf_content=$(echo "$diff_content" | awk '/^\+\+\+ b\/\.github\/workflows\// { flag=1; next } /^diff --git/ { flag=0 } flag && /^\+/ && !/^\+\+\+/ { print }') + if echo "$wf_content" | grep -q 'ruff check'; then + if ! echo "$wf_content" | grep -q 'ruff format'; then + emit_finding "QG-01" "FLAG" ".github/workflows/" 1 \ + "Workflow runs 'ruff check' but not 'ruff format --check' — mismatch with dev gate" "" >> "$findings" + fi + if ! echo "$wf_content" | grep -q 'ty check'; then + emit_finding "QG-02" "FLAG" ".github/workflows/" 1 \ + "Workflow runs 'ruff check' but not 'ty check' — incomplete type gate" "" >> "$findings" + fi + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "quality-gate-parity" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-secrets.sh b/.claude/scripts/check-secrets.sh new file mode 100755 index 00000000..3b12706a --- /dev/null +++ b/.claude/scripts/check-secrets.sh @@ -0,0 +1,115 @@ +#!/usr/bin/env bash +# check-secrets.sh — detect secrets (AWS keys, JWTs, GitHub tokens, private keys, etc.) in added lines. +# All SEC-* rules are BLOCK_LOCKED (cannot be suppressed). +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/json-emit.sh +source "$SCRIPT_DIR/lib/json-emit.sh" +# shellcheck source=lib/hunk-filter.sh +source "$SCRIPT_DIR/lib/hunk-filter.sh" +# shellcheck source=lib/skill-self-skip.sh +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" + +STARTED=$(now_iso) + +# Read diff (either from env-provided file or stdin) — extract added lines with file+line info +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +if [ -z "$diff_content" ]; then + emit_report "secrets" "$LANGUAGE" "PASS" "$STARTED" <<< "" + exit 0 +fi + +findings=$(mktemp) +trap 'rm -f "$findings"' EXIT + +# Parse diff and scan each added line. +# FP-N-01: pre-filter in awk so only lines that could contain a secret reach +# the shell loop. The heuristic is a BROAD superset of every SEC-* anchor — +# the precise per-rule regexes still run in the loop. Secrets are +# BLOCK_LOCKED, so the pre-filter is intentionally over-inclusive (favor +# false-inclusion over ever missing a real secret). +echo "$diff_content" | awk ' + BEGIN { file=""; line=0 } + /^diff --git a\// { + file=$4 + sub(/^b\//, "", file) + line=0 + next + } + /^@@/ { + if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0 + next + } + /^\+/ && !/^\+\+\+/ { + c = substr($0, 2) + # Broad superset of SEC-01..08 anchors: + # AKIA / AIza / gh?_ / sk- / xox / eyJ / PRIVATE KEY / clientsecret + if (c ~ /AKIA|AIza|gh[pousr]_|sk-|xox[baprs]-|eyJ|PRIVATE KEY|[Cc]lient[_]?[Ss]ecret/) { + print file "\t" line "\t" c + } + line++ + next + } + /^ / { line++; next } +' | while IFS=$'\t' read -r file line_num content; do + [ -z "$file" ] && continue + # Self-review protection: skip skill files + if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi + + # SEC-01: AWS Access Key + if echo "$content" | grep -qE 'AKIA[0-9A-Z]{16}'; then + emit_finding "SEC-01" "BLOCK" "$file" "$line_num" "AWS Access Key detected — remove immediately and rotate the key" "" >> "$findings" + fi + # SEC-02: Google API Key + if echo "$content" | grep -qE 'AIza[0-9A-Za-z_-]{35}'; then + emit_finding "SEC-02" "BLOCK" "$file" "$line_num" "Google API Key detected — remove and rotate" "" >> "$findings" + fi + # SEC-03: GitHub PAT + if echo "$content" | grep -qE 'gh[pousr]_[A-Za-z0-9_]{36,}'; then + emit_finding "SEC-03" "BLOCK" "$file" "$line_num" "GitHub PAT detected — remove and rotate" "" >> "$findings" + fi + # SEC-04: OpenAI / Anthropic API key (both use sk-… prefix; Anthropic allows dashes) + if echo "$content" | grep -qE 'sk-[A-Za-z0-9_-]{20,}'; then + emit_finding "SEC-04" "BLOCK" "$file" "$line_num" "AI provider API key detected (sk-…) — remove and rotate" "" >> "$findings" + fi + # SEC-05: Slack bot token + if echo "$content" | grep -qE 'xox[baprs]-[A-Za-z0-9-]+'; then + emit_finding "SEC-05" "BLOCK" "$file" "$line_num" "Slack token detected — remove and rotate" "" >> "$findings" + fi + # SEC-06: JWT + if echo "$content" | grep -qE 'eyJ[A-Za-z0-9_-]{10,}\.eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}'; then + emit_finding "SEC-06" "BLOCK" "$file" "$line_num" "JWT token detected — remove and rotate" "" >> "$findings" + fi + # SEC-07: Private key header + if echo "$content" | grep -qE 'BEGIN (RSA |EC |OPENSSH |DSA |ENCRYPTED )?PRIVATE KEY'; then + emit_finding "SEC-07" "BLOCK" "$file" "$line_num" "Private key header detected — remove and rotate" "" >> "$findings" + fi + # SEC-08: BTP client_secret literal (heuristic: assignment with looks-like-secret value) + # Match client_secret= or clientsecret= with quoted values that look like real secrets + if echo "$content" | grep -qE '"clientsecret"[[:space:]]*:[[:space:]]*"[A-Za-z0-9+/=]{20,}"'; then + emit_finding "SEC-08" "BLOCK" "$file" "$line_num" "BTP client_secret literal detected — use secret resolver" "" >> "$findings" + fi +done + +# SEC-10: .env files in diff (not .env.example, .env.test, .env.sample, .env.template) +# Match both top-level .env and subdirectory service/.env +env_lines=$(echo "$diff_content" | grep -E '^\+\+\+ b/(.*/)?\.env(\..+)?$' || true) +while IFS= read -r line; do + [ -z "$line" ] && continue + # extract path from "+++ b/" + path="${line#+++ b/}" + # basename check for allowlist + base="${path##*/}" + case "$base" in + .env.example|.env.test|.env.sample|.env.template) continue ;; + .env|.env.*) emit_finding "SEC-10" "BLOCK" "$path" 1 ".env file committed — never commit .env; use .env.example" "" >> "$findings" ;; + esac +done <<< "$env_lines" + +status=$(status_from_findings < "$findings") +emit_report "secrets" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-telemetry.sh b/.claude/scripts/check-telemetry.sh new file mode 100755 index 00000000..daea17b3 --- /dev/null +++ b/.claude/scripts/check-telemetry.sh @@ -0,0 +1,119 @@ +#!/usr/bin/env bash +# check-telemetry.sh — verify telemetry instrumentation. +# Python: @record_metrics on public *Client methods + emission tests. +# Java: Telemetry.executeWithTelemetry(...) wrapping + tests. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/json-emit.sh +source "$SCRIPT_DIR/lib/json-emit.sh" +# shellcheck source=lib/hunk-filter.sh +source "$SCRIPT_DIR/lib/hunk-filter.sh" +# shellcheck source=lib/skill-self-skip.sh +source "$SCRIPT_DIR/lib/skill-self-skip.sh" + +LANGUAGE="${LANGUAGE:-python}" +REPO_ROOT="${REPO_ROOT:-.}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" + +STARTED=$(now_iso) +findings=$(mktemp) +trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# Get list of client files newly added or modified +if [ "$LANGUAGE" = "python" ]; then + client_files=$(echo "$diff_content" | { grep -oE '^\+\+\+ b/src/sap_cloud_sdk/[a-z_]+/.*Client\.py|^\+\+\+ b/src/sap_cloud_sdk/[a-z_]+/client\.py' 2>/dev/null || true; } | sed 's|^+++ b/||' | sort -u) +else + client_files=$(echo "$diff_content" | { grep -oE '^\+\+\+ b/src/main/java/com/sap/cloud/sdk/[a-z_]+/.*Client\.java' 2>/dev/null || true; } | sed 's|^+++ b/||' | sort -u) +fi + +# Detect new decorator additions ONLY inside client files (scope predicate). +# Previously this counted @record_metrics from any added line — including tests, +# examples, and skill files — which produced false PY-TEL-06 findings. +if [ "$LANGUAGE" = "python" ]; then + if [ -n "$client_files" ]; then + # Build an awk-friendly set of client paths + new_decorators=$(echo "$diff_content" | awk -v files="$client_files" ' + BEGIN { + n = split(files, arr, "\n") + for (i=1; i<=n; i++) if (arr[i] != "") set[arr[i]] = 1 + current = "" + } + /^\+\+\+ b\// { current = substr($0, 7); next } + /^\+[[:space:]]*@record_metrics/ { + if (current in set) count++ + next + } + END { print count+0 } + ') + else + new_decorators=0 + fi +else + new_decorators=$(echo "$diff_content" | { grep -E '^\+.*Telemetry\.executeWithTelemetry' 2>/dev/null || true; } | wc -l | tr -d ' ') +fi + +# PY-TEL-02: For each changed client file, run AST check +# FP-A-01: filter by hunk attribution — a client method predates the PR unless +# it lives on a line the PR added. +if [ "$LANGUAGE" = "python" ] && [ -n "$client_files" ]; then + raw_tel=$(mktemp); trap 'rm -f "$raw_tel"' EXIT + # shellcheck disable=SC2086 + python3 "$SCRIPT_DIR/lib/ast_python_checks.py" tel-02 $client_files 2>/dev/null > "$raw_tel" || true + while IFS= read -r finding; do + [ -z "$finding" ] && continue + f=$(echo "$finding" | python3 -c "import json,sys;o=json.loads(sys.stdin.read());print(o.get('file',''))") + ln=$(echo "$finding" | python3 -c "import json,sys;o=json.loads(sys.stdin.read());print(o.get('line',0))") + [ -z "$f" ] && continue + if is_line_touched "$f" "$ln"; then + echo "$finding" >> "$findings" + fi + done < "$raw_tel" + rm -f "$raw_tel" +fi + +# JV-TEL-02: For Java, grep-based check (executeWithTelemetry wrap around methods) +if [ "$LANGUAGE" = "java" ] && [ -n "$client_files" ]; then + while IFS= read -r f; do + [ -z "$f" ] && continue + full_path="$REPO_ROOT/$f" + [ -f "$full_path" ] || continue + # find public methods in the file + while IFS= read -r match; do + line_num="${match%%:*}" + # check the following 20 lines for executeWithTelemetry + end_line=$((line_num + 20)) + body=$(sed -n "${line_num},${end_line}p" "$full_path") + if ! echo "$body" | grep -q "executeWithTelemetry"; then + emit_finding_if_touched "JV-TEL-02" "BLOCK" "$f" "$line_num" \ + "Public method lacks Telemetry.executeWithTelemetry wrap" "" >> "$findings" + fi + done < <(grep -nE '^[[:space:]]*public [A-Za-z<>]+ [a-z][a-zA-Z0-9]+\(' "$full_path" 2>/dev/null | grep -v 'public class\|public interface\|public enum' || true) + done <<< "$client_files" +fi + +# PY-TEL-06 / JV-TEL-05: emission tests required when new decorator/wrapper added +if [ "$new_decorators" -gt 0 ]; then + if [ "$LANGUAGE" = "python" ]; then + # Find test files added/modified in same PR + test_files=$(echo "$diff_content" | { grep -oE '^\+\+\+ b/tests/[a-z_]+/.*/test_.*\.py' 2>/dev/null || true; } | sed 's|^+++ b/||' | sort -u) + has_metric_assert=false + while IFS= read -r tf; do + [ -z "$tf" ] && continue + [ -f "$REPO_ROOT/$tf" ] || continue + if grep -q 'record_request_metric\|record_error_metric' "$REPO_ROOT/$tf"; then + has_metric_assert=true; break + fi + done <<< "$test_files" + if [ "$has_metric_assert" = "false" ]; then + emit_finding "PY-TEL-06" "BLOCK" "tests/" 1 \ + "New @record_metrics added ($new_decorators occurrences) but no test asserts record_request_metric was called" \ + "Add a test that mocks record_request_metric and asserts it was called with the expected Module/Operation" >> "$findings" + fi + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "telemetry" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-testing-depth.sh b/.claude/scripts/check-testing-depth.sh new file mode 100755 index 00000000..ba1a4c98 --- /dev/null +++ b/.claude/scripts/check-testing-depth.sh @@ -0,0 +1,57 @@ +#!/usr/bin/env bash +# check-testing-depth.sh — test names, tests-added checkbox truthfulness, integration test present. +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" + +LANGUAGE="${LANGUAGE:-python}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +PR_BODY_FILE="${PR_BODY_FILE:-}" + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# TD-01: If PR title is fix: AND src/ changed AND no test file changed → FLAG +if [ "$LANGUAGE" = "python" ]; then + fix_commit=$(git log HEAD --format=%s -n 1 2>/dev/null | grep -qE '^fix' && echo yes || echo no) + src_changed=$(echo "$diff_content" | grep -qE 'src/sap_cloud_sdk/' && echo yes || echo no) + test_changed=$(echo "$diff_content" | grep -qE 'tests?/.*/test_' && echo yes || echo no) + if [ "$fix_commit" = "yes" ] && [ "$src_changed" = "yes" ] && [ "$test_changed" = "no" ]; then + emit_finding "TD-01" "FLAG" "tests/" 1 \ + "Bug-fix PR touches src/ but no test files changed" \ + "Add a focused unit test that reproduces the bug and asserts the fix" >> "$findings" + fi + + # TD-10: New module → integration test required + new_modules=$(echo "$diff_content" | awk '/^diff --git/ { flag=0 } /^new file mode/ { flag=1 } flag && /^\+\+\+ b\/src\/sap_cloud_sdk\/[a-z_]+\/[^\/]+\.py/ { print }' | { grep -oE 'src/sap_cloud_sdk/[a-z_]+/' 2>/dev/null || true; } | sed 's|src/sap_cloud_sdk/||; s|/$||' | sort -u) + while IFS= read -r mod; do + # Both conditions should skip the loop iteration. The previous + # A || B && continue form parses as (A || B) && continue, which is + # correct — but the `&& continue` under `set -e` short-circuits the + # loop body's exit status and hides errors. Explicit if is safer. + if [ -z "$mod" ] || [ "$mod" = "core" ]; then + continue + fi + has_integration=$(echo "$diff_content" | grep -qE "tests/$mod/integration/" && echo yes || echo no) + if [ "$has_integration" = "no" ]; then + emit_finding "TD-10" "BLOCK" "tests/$mod/integration/" 1 \ + "New module '$mod' has no integration test" "" >> "$findings" + fi + done <<< "$new_modules" +fi + +# TD-checkbox: PR body says "tests added" but no test files touched +if [ -n "$PR_BODY_FILE" ] && [ -f "$PR_BODY_FILE" ]; then + body=$(cat "$PR_BODY_FILE") + if echo "$body" | grep -qE '\-[[:space:]]*\[[xX]\][[:space:]].*(added|updated).*tests'; then + if ! echo "$diff_content" | grep -qE 'diff --git a/(tests?/|src/test/)'; then + emit_finding "TD-checkbox" "FLAG" "PR_BODY" 1 \ + "PR body ticks 'added tests' checkbox but no test files changed" "" >> "$findings" + fi + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "testing-depth" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/check-versioning.sh b/.claude/scripts/check-versioning.sh new file mode 100755 index 00000000..1edfd107 --- /dev/null +++ b/.claude/scripts/check-versioning.sh @@ -0,0 +1,121 @@ +#!/usr/bin/env bash +# check-versioning.sh — SemVer bump + BREAKING family (BREAKING-01..04). +set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/lib/json-emit.sh" +source "$SCRIPT_DIR/lib/predicates.sh" + +LANGUAGE="${LANGUAGE:-python}" +REPO_ROOT="${REPO_ROOT:-.}" +DIFF_FILE="${DIFF_FILE:-/dev/stdin}" +PR_BODY_FILE="${PR_BODY_FILE:-}" +# Resolve BASE_SHA from the PR base ref rather than HEAD~10 (which fails on +# short branches). Only fall back to HEAD~10 when we can't reach a base ref. +if [ -z "${BASE_SHA:-}" ]; then + base_ref="${GITHUB_BASE_REF:-main}" + if git rev-parse --verify "origin/${base_ref}" >/dev/null 2>&1; then + BASE_SHA=$(git merge-base HEAD "origin/${base_ref}" 2>/dev/null || echo "HEAD~10") + elif git rev-parse --verify "$base_ref" >/dev/null 2>&1; then + BASE_SHA=$(git merge-base HEAD "$base_ref" 2>/dev/null || echo "HEAD~10") + else + BASE_SHA="HEAD~10" + fi +fi +HEAD_SHA="${HEAD_SHA:-HEAD}" +BREAKING_JSON="${BREAKING_JSON:-}" # pre-computed via breaking-detector.py + +STARTED=$(now_iso) +findings=$(mktemp); trap 'rm -f "$findings"' EXIT + +diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "") + +# Detect version-bump line change +if [ "$LANGUAGE" = "python" ]; then + version_bumped=$(echo "$diff_content" | grep -E '^\+version[[:space:]]*=' | head -1 || true) + version_removed=$(echo "$diff_content" | grep -E '^-version[[:space:]]*=' | head -1 || true) +else + version_bumped=$(echo "$diff_content" | grep -E '^\+[[:space:]]*' | head -1 || true) + version_removed=$(echo "$diff_content" | grep -E '^-[[:space:]]*' | head -1 || true) +fi + +# src/ changes present? +if [ "$LANGUAGE" = "python" ]; then + src_changed=$(echo "$diff_content" | grep -qE 'diff --git a/src/sap_cloud_sdk/' && echo yes || echo no) +else + src_changed=$(echo "$diff_content" | grep -qE 'diff --git a/src/main/java/' && echo yes || echo no) +fi + +# commit types (BASE_SHA already resolved above; do not fall back to HEAD~10) +commit_types=$(has_commit_type "$BASE_SHA" "$HEAD_SHA" "feat,feat!,fix,fix!" 2>/dev/null || echo "false") + +# BREAKING detector output +breaking_detected="false" +if [ -n "$BREAKING_JSON" ] && [ -f "$BREAKING_JSON" ]; then + breaking_detected=$(jq -r '.breaking_detected' "$BREAKING_JSON" 2>/dev/null || echo "false") +fi + +is_feat=$(has_commit_type "$BASE_SHA" "$HEAD_SHA" "feat,feat!" 2>/dev/null || echo "false") + +# VER-01: src/ change without bump — only fires on feat OR breaking +if [ "$src_changed" = "yes" ] && [ -z "$version_bumped" ]; then + if [ "$is_feat" = "true" ] || [ "$breaking_detected" = "true" ]; then + emit_finding "VER-01" "BLOCK" "pyproject.toml" 1 \ + "Feature or breaking change detected but version not bumped" \ + "Bump MINOR version for feat/API change; MAJOR for breaking" >> "$findings" + fi +fi + +# BREAKING-01: if breaking, check PR body has proper declarations +if [ "$breaking_detected" = "true" ]; then + # collect requirements + has_bang=$(has_commit_type "$BASE_SHA" "$HEAD_SHA" "feat!,fix!" 2>/dev/null || echo "false") + has_bump=$([ -n "$version_bumped" ] && echo "true" || echo "false") + + pr_body="" + if [ -n "$PR_BODY_FILE" ] && [ -f "$PR_BODY_FILE" ]; then + pr_body=$(cat "$PR_BODY_FILE") + fi + has_breaking_section="false" + if echo "$pr_body" | grep -qE '^##[[:space:]]+Breaking Changes' ; then + # section must have non-empty content (not "N/A" or "None" alone) + # Use awk to extract from "## Breaking Changes" to next "## " heading, drop the header line itself + section=$(echo "$pr_body" | awk ' + /^##[[:space:]]+Breaking Changes/ { flag=1; next } + flag && /^##[[:space:]]+[A-Z]/ { exit } + flag { print } + ' | tr -d '[:space:]') + if [ -n "$section" ] && ! echo "$section" | grep -qEi '^(N/A|None|none|--)*$'; then + has_breaking_section="true" + fi + fi + # Checkbox: accept -, *, +, or bullet with either ticked casing + has_checkbox=$(echo "$pr_body" | grep -qE '^[[:space:]]*[-*+][[:space:]]*\[[xX]\][[:space:]]+([Bb]reaking change|BREAKING|Contains breaking)' && echo "true" || echo "false") + + # if ANY of the 4 requirements is missing → BLOCK + missing="" + [ "$has_bang" != "true" ] && missing="$missing commit-!:-prefix" + [ "$has_bump" != "true" ] && missing="$missing version-bump" + [ "$has_breaking_section" != "true" ] && missing="$missing PR-body-Breaking-Changes-section" + [ "$has_checkbox" != "true" ] && missing="$missing PR-body-checkbox" + + if [ -n "$missing" ]; then + emit_finding "BREAKING-01" "BLOCK" "PR_METADATA" 1 \ + "Breaking change detected — missing declarations:$missing" \ + "Add all 4: (a) feat!:/fix!: commit prefix (b) ## Breaking Changes section (c) checkbox ticked (d) version bump" >> "$findings" + + # BREAKING-02: partial declaration (some declared, others not) + declared_count=0 + [ "$has_bang" = "true" ] && declared_count=$((declared_count+1)) + [ "$has_bump" = "true" ] && declared_count=$((declared_count+1)) + [ "$has_breaking_section" = "true" ] && declared_count=$((declared_count+1)) + [ "$has_checkbox" = "true" ] && declared_count=$((declared_count+1)) + if [ "$declared_count" -gt 0 ] && [ "$declared_count" -lt 4 ]; then + emit_finding "BREAKING-02" "BLOCK" "PR_METADATA" 1 \ + "Breaking-change metadata is inconsistent ($declared_count/4 declared)" \ + "All 4 declarations must agree — reconcile or fully retract" >> "$findings" + fi + fi +fi + +status=$(status_from_findings < "$findings") +emit_report "versioning" "$LANGUAGE" "$status" "$STARTED" < "$findings" diff --git a/.claude/scripts/lib/ast_python_checks.py b/.claude/scripts/lib/ast_python_checks.py index 7e290c42..42b689f3 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 [ ...] + python3 ast-python-checks.py [ ...] Where is one of: el-01, el-02 exception chaining / swallow @@ -18,6 +18,7 @@ import ast import json +import os import sys from pathlib import Path @@ -212,31 +213,122 @@ def check_pt_08(path: str, tree: ast.Module) -> None: def check_con_01( path: str, tree: ast.Module, threshold: int = 3, min_len: int = 4 ) -> None: + # FP-D-01: skip generated/model files where schema keys legitimately repeat. + GENERATED_PATTERNS = ("_models.py", "_generated.py") + GENERATED_API_SUFFIX = "_api.py" + fname = Path(path).name + if fname.endswith(GENERATED_PATTERNS): + return + # `__api.py` (starts with underscore, ends with _api.py) → generated OpenAPI client + if fname.startswith("_") and fname.endswith(GENERATED_API_SUFFIX): + return # 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") + # FP-D-01: minimum length 4 (was 4; enforce hard floor of 3 per plan) + effective_min_len = max(min_len, 3) + # FP-D-01: per-file cap + MAX_PER_FILE = 3 + + # FP-K-01: load the PR's added-lines set (if orchestrate provided it) so + # we only count string occurrences that the PR actually introduces. Prevents + # penalizing an unrelated edit for pre-existing repetitions. + added_lines_for_file: set[int] | None = None + added_lines_file = os.environ.get("ADDED_LINES_FILE", "") + if added_lines_file and Path(added_lines_file).is_file(): + added_lines_for_file = set() + try: + with open(added_lines_file, encoding="utf-8") as fh: + for entry in fh: + entry = entry.strip() + if not entry or ":" not in entry: + continue + p, _, ln = entry.rpartition(":") + if p == path: + try: + added_lines_for_file.add(int(ln)) + except ValueError: + continue + except OSError: + added_lines_for_file = None + # If the caller didn't provide a diff scope, fall back to legacy behavior + # (count all occurrences). Bats tests exercise the check without an + # orchestrate wrapper, so we keep backward-compat. + 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: + if len(v) < effective_min_len: continue if v.startswith(URL_PREFIXES): continue if v.startswith(SKIP_PREFIXES): continue counts.setdefault(v, []).append(node.lineno) + emitted = 0 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}", - ) + if emitted >= MAX_PER_FILE: + break + if len(lines) < threshold: + continue + # FP-K-01: require at least one occurrence to live on a PR-added line. + # Without this, an unrelated edit is credited for the repetition that + # already existed. If we have no diff scope, allow the legacy path. + if added_lines_for_file is not None: + added_occurrences = [ln for ln in lines if ln in added_lines_for_file] + if not added_occurrences: + continue + anchor_line = added_occurrences[0] + else: + anchor_line = lines[0] + emit( + "PY-CON-01", + "FLAG", + path, + anchor_line, + f"String literal {literal!r} appears {len(lines)}× — extract module-level constant", + suggestion=f"e.g., _CONSTANT_NAME = {literal!r}", + ) + emitted += 1 + + +# ---------- PY-PT-04: module has any Exception subclass (AST-based) ---------- + + +def check_pt_04(module_dir: str) -> bool: + """FP-C-02: return True if any .py file in the module directory (recursive) + defines a class subclassing Exception (directly or via a name ending in + 'Error' / 'Exception'). Callers can use this as a soft check before + emitting PY-PT-04. + """ + p = Path(module_dir) + if not p.is_dir(): + return False + for py in p.rglob("*.py"): + if "__pycache__" in py.parts: + continue + tree = parse_file(str(py)) + if tree is None: + continue + for node in ast.walk(tree): + if not isinstance(node, ast.ClassDef): + continue + for base in node.bases: + base_name = None + if isinstance(base, ast.Name): + base_name = base.id + elif isinstance(base, ast.Attribute): + base_name = base.attr + if not base_name: + continue + if base_name == "Exception" or base_name == "BaseException": + return True + # accept anything ending in Error/Exception as an exception hierarchy + if base_name.endswith("Error") or base_name.endswith("Exception"): + return True + return False # ---------- PY-PT-01: create_client factory exists ---------- @@ -285,13 +377,17 @@ def extract_all(tree: ast.Module) -> list[str]: 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 +) -> dict[str, dict[str, tuple]]: + """Return {ClassName: {method_name: signature_tuple}}. + + signature_tuple = ( + positional_args, # names of positional args (self/cls excluded) + kwonly_args, # names of keyword-only args + vararg_kwarg_flags, # ["*args", "**kwargs"] if present + return_ann, # repr of return annotation, or None + num_pos_defaults, # count of positional args with a default value + num_kwonly_required, # count of keyword-only args WITHOUT a default + ) """ result: dict[str, dict] = {} for node in ast.walk(tree): @@ -313,7 +409,22 @@ def extract_public_class_methods( 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) + # Count of positional args that have a default value. Needed by + # the breaking-change detector to tell an additive change (new + # optional trailing arg) from a breaking one (new required arg). + num_pos_defaults = len(item.args.defaults) + # kwonly defaults: kw_defaults has None entries for required kwonly + num_kwonly_required = sum( + 1 for d in item.args.kw_defaults if d is None + ) + methods[item.name] = ( + pos_args, + kwonly, + varflags, + ret, + num_pos_defaults, + num_kwonly_required, + ) if methods: result[node.name] = methods return result @@ -377,13 +488,77 @@ 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 [ ...]", + "Usage: ast-python-checks.py [ ...]", file=sys.stderr, ) return 2 check_name = argv[1] files = argv[2:] + # FP-C-02: pt-04 has a different signature (module dir, not files) and + # returns a boolean via exit code (0 = has exceptions, 1 = none found). + if check_name == "pt-04": + module_dir = files[0] + return 0 if check_pt_04(module_dir) else 1 + + # FP-R-01: docstring-lines prints "1" for every line number that falls + # inside a module/class/function docstring, so line-based checks (e.g. + # check-hardcode HC-01) can skip URLs that live in documentation examples. + if check_name == "docstring-lines": + for f in files: + tree = parse_file(f) + if tree is None: + continue + for node in ast.walk(tree): + if not isinstance( + node, (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef) + ): + continue + doc = ast.get_docstring(node, clean=False) + if not doc: + continue + # The docstring is the first statement's Constant node. + body = getattr(node, "body", []) + if not body: + continue + first = body[0] + if ( + isinstance(first, ast.Expr) + and isinstance(first.value, ast.Constant) + and isinstance(first.value.value, str) + ): + start = first.value.lineno + end = getattr(first.value, "end_lineno", start) + for ln in range(start, end + 1): + print(ln) + return 0 + + # FP-S-01: http-session-lines prints the line number of every + # requests.Session()/httpx.Client()/AsyncClient() call that is NOT inside + # __init__ (or a module/class-level assignment). A session created in + # __init__ is the RECOMMENDED pattern, so HTTP-01 must not fire on it. + # Only lines printed here are per-invocation sessions worth flagging. + if check_name == "http-session-lines": + SESSION_CTORS = {"Session", "Client", "AsyncClient"} + for f in files: + tree = parse_file(f) + if tree is None: + continue + # Map every function def to whether it is __init__. + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if node.name == "__init__": + continue # sessions here are the correct pattern + for sub in ast.walk(node): + if ( + isinstance(sub, ast.Call) + and isinstance(sub.func, ast.Attribute) + and sub.func.attr in SESSION_CTORS + ): + print(sub.lineno) + return 0 + if check_name not in CHECKS: print(f"ERROR: unknown check {check_name}", file=sys.stderr) return 2 diff --git a/.claude/scripts/lib/breaking-detector.py b/.claude/scripts/lib/breaking-detector.py index 47139806..4648a63c 100755 --- a/.claude/scripts/lib/breaking-detector.py +++ b/.claude/scripts/lib/breaking-detector.py @@ -21,6 +21,7 @@ import ast import json +import os import subprocess import sys from pathlib import Path @@ -92,13 +93,124 @@ def files_changed(base: str, head: str) -> list[str]: text=True, check=False, ) - return [f for f in out.stdout.strip().split("\n") if f] + files = [f for f in out.stdout.strip().split("\n") if f] + # FP-Q-01: when the PR branch is behind main, `base..head` also surfaces + # files that MAIN changed after the merge-base — the PR never touched them. + # Comparing base-vs-head on those makes main's additions look like PR + # deletions. Restrict to files the PR actually modified, taken from the + # PR's own diff (ADDED_LINES_FILE lists "path:line" for every added line). + added_lines_file = os.environ.get("ADDED_LINES_FILE", "") + if added_lines_file and os.path.isfile(added_lines_file): + pr_files: set[str] = set() + try: + with open(added_lines_file, encoding="utf-8") as fh: + for entry in fh: + entry = entry.strip() + if ":" in entry: + pr_files.add(entry.rpartition(":")[0]) + except OSError: + pr_files = set() + if pr_files: + files = [f for f in files if f in pr_files] + return files + + +def _is_breaking_sig_change(base_sig: tuple, head_sig: tuple) -> bool: + """Return True only if the signature change breaks existing callers. + + Signature tuple layout (see ast_python_checks.extract_public_class_methods): + (pos_args, kwonly_args, var_flags, return_ann, + num_pos_defaults, num_kwonly_required) + + ADDITIVE (not breaking): + - Appending new positional args that all have defaults. + - Adding keyword-only args that have defaults. + - (var_flags unchanged, return unchanged, no existing arg renamed/removed/reordered) + + BREAKING: + - Removing/renaming/reordering any existing positional arg. + - Adding a positional arg WITHOUT a default (new required arg). + - Adding a required keyword-only arg. + - Removing *args/**kwargs. + - Changing the return annotation. + """ + base_pos, base_kw, base_var, base_ret = base_sig[0], base_sig[1], base_sig[2], base_sig[3] + head_pos, head_kw, head_var, head_ret = head_sig[0], head_sig[1], head_sig[2], head_sig[3] + # Older signature tuples (4-element) had no default counts — treat any + # change as breaking to stay conservative for legacy callers. + if len(base_sig) < 6 or len(head_sig) < 6: + return base_sig != head_sig + head_pos_defaults = head_sig[4] + head_kwonly_required = head_sig[5] + base_kwonly_required = base_sig[5] + + # Return type change → breaking. + if base_ret != head_ret: + return True + # Removing *args/**kwargs → breaking (callers may rely on them). + if set(base_var) - set(head_var): + return True + # Existing positional args must be preserved as a prefix, in order. + if head_pos[: len(base_pos)] != base_pos: + return True # a prefix arg was removed, renamed, or reordered + # Any NEW positional args (beyond the base prefix) must all have defaults. + num_new_pos = len(head_pos) - len(base_pos) + if num_new_pos > 0: + # head_pos_defaults counts trailing defaulted positionals. For the new + # args to be additive, there must be at least num_new_pos defaults. + if head_pos_defaults < num_new_pos: + return True # added a required positional arg → breaking + elif num_new_pos < 0: + return True # positional args were removed + # Keyword-only: any newly-required kwonly arg is breaking. + if head_kwonly_required > base_kwonly_required: + return True + # Removing an existing kwonly arg is breaking. + if set(base_kw) - set(head_kw): + return True + # Everything else is additive or a no-op. + return False + + +def _load_pr_removed_text() -> str | None: + """Concatenate all '-' (removed) lines from the PR's own diff. + + FP-Q-01 (enum/field/method variant): AST comparison of the full base vs + head file treats symbols that MAIN added (but the behind-main PR lacks) as + PR deletions. A real deletion by THIS PR must appear on a '-' line in the + PR's diff. We load that removed-text once and gate every deletion finding + on the symbol name appearing there. Returns None when no diff is available + (then we fall back to the AST-only behavior, i.e. no gating). + """ + diff_file = os.environ.get("DIFF_FILE", "") + if not diff_file or not os.path.isfile(diff_file): + return None + removed: list[str] = [] + try: + with open(diff_file, encoding="utf-8", errors="replace") as fh: + for ln in fh: + # Real removed lines start with '-' but not '---' (file header) + if ln.startswith("-") and not ln.startswith("---"): + removed.append(ln[1:]) + except OSError: + return None + return "".join(removed) def detect(base: str, head: str) -> dict: details: list[dict] = [] kinds: set[str] = set() + # FP-Q-01: text of all lines this PR removed (None if no diff available). + pr_removed_text = _load_pr_removed_text() + + def pr_actually_removed(symbol_leaf: str) -> bool: + """True if the PR's diff removed a line mentioning this symbol leaf. + When we have no diff context, don't gate (return True).""" + if pr_removed_text is None: + return True + return symbol_leaf in pr_removed_text + changed = files_changed(base, head) for file in changed: @@ -133,6 +245,8 @@ def detect(base: str, head: str) -> dict: head_all = set(extract_all(head_tree)) removed = base_all - head_all for name in removed: + if not pr_actually_removed(name): + continue details.append( { "kind": "api_removal_detected", @@ -150,6 +264,10 @@ def detect(base: str, head: str) -> dict: head_methods = head_classes.get(cls_name, {}) for mname, base_sig in base_methods.items(): if mname not in head_methods: + # FP-Q-01: only a real deletion if the PR removed a line + # mentioning this method (guards against behind-main branches). + if not pr_actually_removed(mname): + continue details.append( { "kind": "method_deletion_on_public_class", @@ -161,9 +279,9 @@ def detect(base: str, head: str) -> dict: 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: + # Compare signatures, but only flag CALLER-BREAKING changes. + # Adding a trailing optional arg is additive, not breaking. + if _is_breaking_sig_change(base_sig, head_sig): details.append( { "kind": "public_method_signature_change", @@ -193,6 +311,8 @@ def detect(base: str, head: str) -> dict: head_fields = set(head_dcs.get(dc_name, [])) for field in base_fields: if field not in head_fields: + if not pr_actually_removed(field): + continue details.append( { "kind": "dataclass_field_deletion_on_public_model", @@ -210,6 +330,8 @@ def detect(base: str, head: str) -> dict: head_values = set(head_enums.get(enum_name, [])) for val in base_values: if val not in head_values: + if not pr_actually_removed(val): + continue details.append( { "kind": "enum_value_deletion_on_public_enum", diff --git a/.claude/scripts/lib/github-api.sh b/.claude/scripts/lib/github-api.sh index 3c4af30a..acea967c 100755 --- a/.claude/scripts/lib/github-api.sh +++ b/.claude/scripts/lib/github-api.sh @@ -36,6 +36,13 @@ detect_hostname() { esac } +# gh_api — wrapper that pins --hostname so `gh_api repos/OWNER/REPO/…` hits the +# correct GitHub instance. Without this, gh defaults to github.com and every +# call against the internal github.tools.sap repo 404s. +gh_api() { + gh api --hostname "$(detect_hostname)" "$@" +} + # detect_owner_repo → prints "owner/repo" from git remote # Handles all remote formats: # https://github.com/OWNER/REPO.git @@ -79,7 +86,7 @@ get_pr_head_sha() { 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 | \ + 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 } @@ -87,23 +94,28 @@ list_bot_review_comments() { 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 | \ + 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 — idempotency: remove prior INLINE review +# comments before posting fresh ones. The summary comment is NOT deleted here; +# post_summary_comment updates it in place (avoids a delete→recreate race that +# produced duplicate summaries). 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 + # review-comments (inline). Loop with a bounded retry: a single pass can miss + # comments if pagination races with deletion, which is how duplicate/stale + # comments accumulate. Re-list until none remain (max 5 passes). + local pass ids + for pass in 1 2 3 4 5; do + ids=$(list_bot_review_comments "$pr" | awk -F'\t' '{print $1}' | grep -E '^[0-9]+$' || true) + [ -z "$ids" ] && break + while read -r id; do + [ -n "$id" ] && gh_api -X DELETE "repos/${owner_repo}/pulls/comments/${id}" > /dev/null 2>&1 || true + done <<< "$ids" done } @@ -117,36 +129,53 @@ is_fork_pr() { } # post_inline_comment +# On success: echoes the created comment's html_url (for linking from summary). +# On 422 (line not in diff): silent, echoes nothing (caller lists it in summary). # 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" \ + local resp + # Capture the JSON response; on success it contains .html_url. + resp=$(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 + -F side="RIGHT" 2>/dev/null || true) + local url + url=$(echo "$resp" | jq -r '.html_url // empty' 2>/dev/null || echo "") + if [ -n "$url" ]; then + echo "$url" fi + # If the post failed (no url), the caller still lists the finding in the + # summary, so nothing is lost. We intentionally do NOT create a duplicate + # issue-comment fallback here — the summary already carries every finding. } # post_summary_comment +# Update-in-place: if a prior summary comment exists, PATCH it instead of +# creating a new one. This makes the summary idempotent even under concurrent +# runs (which is how duplicate summaries appeared). Only creates a new comment +# when none exists. 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 - } + local existing_id + existing_id=$(list_bot_issue_comments "$pr" | grep -E '^[0-9]+$' | head -1 || true) + if [ -n "$existing_id" ]; then + gh_api -X PATCH "repos/${owner_repo}/issues/comments/${existing_id}" -F body="$body" > /dev/null 2>&1 || { + echo "WARN: could not update summary comment (fork PR or missing pull-requests:write scope)" >&2 + } + # Delete any extra duplicates beyond the one we just updated. + list_bot_issue_comments "$pr" | grep -E '^[0-9]+$' | tail -n +2 | while read -r dup; do + [ -n "$dup" ] && gh_api -X DELETE "repos/${owner_repo}/issues/comments/${dup}" > /dev/null 2>&1 || true + done + else + 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 + } + fi } # post_or_update_check_run <summary> @@ -158,11 +187,11 @@ post_or_update_check_run() { # 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 | \ + 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}" \ + gh_api -X PATCH "repos/${owner_repo}/check-runs/${existing}" \ -F status="completed" \ -F conclusion="$conclusion" \ -F "output[title]=$title" \ @@ -170,7 +199,7 @@ post_or_update_check_run() { echo "WARN: could not update check-run (fork PR or missing checks:write scope)" >&2 } else - gh api "repos/${owner_repo}/check-runs" \ + gh_api "repos/${owner_repo}/check-runs" \ -F name="$check_name" \ -F head_sha="$sha" \ -F external_id="$external_id" \ @@ -193,7 +222,7 @@ apply_label() { "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 + 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 diff --git a/.claude/scripts/orchestrate.sh b/.claude/scripts/orchestrate.sh new file mode 100755 index 00000000..1cd8aa8f --- /dev/null +++ b/.claude/scripts/orchestrate.sh @@ -0,0 +1,277 @@ +#!/usr/bin/env bash +# orchestrate.sh — main entry point. Runs all 20 checks and posts results. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +LIB="$SCRIPT_DIR/lib" + +# shellcheck source=lib/json-emit.sh +source "$LIB/json-emit.sh" +# shellcheck source=lib/github-api.sh +source "$LIB/github-api.sh" + +PR_NUMBER="${1:-}" +DRY_RUN="${DRY_RUN:-false}" +if [ "${2:-}" = "--dry-run" ]; then DRY_RUN=true; fi + +if [ -z "$PR_NUMBER" ]; then + echo "Usage: orchestrate.sh <PR_NUMBER> [--dry-run]" >&2 + exit 2 +fi + +REPO_ROOT="${REPO_ROOT:-$(pwd)}" +export REPO_ROOT +CONFIG_DIR="$REPO_ROOT/.claude/config" +export CONFIG_DIR +if [ -n "${TMPDIR_RUN:-}" ]; then + mkdir -p "$TMPDIR_RUN" +else + TMPDIR_RUN="$(mktemp -d)" +fi +trap '[ -n "${KEEP_TMP:-}" ] || rm -rf "$TMPDIR_RUN"' EXIT + +echo "▶ SDK Module Review — PR #$PR_NUMBER (dry-run=$DRY_RUN)" +echo " Working dir: $TMPDIR_RUN" + +# 1. Preflight — detect language + hostname + fetch PR data +LANGUAGE=$("$LIB/detect-language.sh" "$REPO_ROOT") +export LANGUAGE +echo " Language: $LANGUAGE" + +if [ "$DRY_RUN" != "true" ]; then + # Bash sets HOSTNAME automatically; using our own HOSTNAME shadowed it and + # produced confusing errors under `set -u`. Rename to GH_HOSTNAME. + GH_HOSTNAME=$(detect_hostname) + check_gh_auth "$GH_HOSTNAME" +fi + +# 2. Fetch diff + PR metadata +if [ -f "${DIFF_FILE:-}" ]; then + cp "$DIFF_FILE" "$TMPDIR_RUN/pr.diff" +elif [ "$DRY_RUN" = "true" ] && [ -n "${LOCAL_DIFF:-}" ]; then + cp "$LOCAL_DIFF" "$TMPDIR_RUN/pr.diff" +else + gh pr diff "$PR_NUMBER" > "$TMPDIR_RUN/pr.diff" +fi + +if [ "$DRY_RUN" != "true" ]; then + gh pr view "$PR_NUMBER" --json body -q .body > "$TMPDIR_RUN/pr-body.txt" 2>/dev/null || echo "" > "$TMPDIR_RUN/pr-body.txt" + HEAD_SHA=$(get_pr_head_sha "$PR_NUMBER") + # PR title feeds check-commits (squash-merge subject). Non-fatal if it fails. + PR_TITLE=$(gh pr view "$PR_NUMBER" --json title -q .title 2>/dev/null || echo "") + export PR_TITLE +elif [ -n "${LOCAL_PR_BODY:-}" ] && [ -f "$LOCAL_PR_BODY" ]; then + cp "$LOCAL_PR_BODY" "$TMPDIR_RUN/pr-body.txt" + HEAD_SHA="${HEAD_SHA:-HEAD}" +else + echo "" > "$TMPDIR_RUN/pr-body.txt" + HEAD_SHA="${HEAD_SHA:-HEAD}" +fi + +export DIFF_FILE="$TMPDIR_RUN/pr.diff" +export PR_BODY_FILE="$TMPDIR_RUN/pr-body.txt" +export HEAD_SHA +export BASE_SHA="${BASE_SHA:-$(git merge-base "origin/${GITHUB_BASE_REF:-main}" HEAD 2>/dev/null || git merge-base origin/main HEAD 2>/dev/null || echo HEAD~10)}" +export BREAKING_JSON="$TMPDIR_RUN/breaking.json" + +# 3. Compute added-lines set (used for hunk attribution) +"$LIB/diff-added-lines.sh" < "$DIFF_FILE" > "$TMPDIR_RUN/added-lines.txt" +export ADDED_LINES_FILE="$TMPDIR_RUN/added-lines.txt" + +# 4. Run breaking-change detector +python3 "$LIB/breaking-detector.py" "$BASE_SHA" "$HEAD_SHA" > "$BREAKING_JSON" 2>/dev/null || echo '{"breaking_detected":false,"kinds":[],"details":[]}' > "$BREAKING_JSON" + +# 5. Detect disclosure profile from remote +if [ "$DRY_RUN" != "true" ]; then + remote_url=$(git remote get-url origin 2>/dev/null || echo "") + if [[ "$remote_url" == *"github.tools.sap"* ]]; then + export DISCLOSURE_PROFILE="internal" + else + export DISCLOSURE_PROFILE="public" + fi +else + export DISCLOSURE_PROFILE="${DISCLOSURE_PROFILE:-public}" +fi + +# 6. Run all 20 checks in parallel +checks=(secrets license-spdx disclosure hardcode telemetry + docs bdd patterns versioning commits + errors-logging testing-depth http-hygiene concurrency + deps-supply deletion-hygiene constants binding-shape + quality-gate-parity pr-size) + +# 6. Run all 20 checks in parallel. Cap each check at 60s so a wedged +# subprocess (e.g. blocked on stdin) can't hang the review indefinitely. +CHECK_TIMEOUT="${CHECK_TIMEOUT:-180}" +# Detect a portable timeout command (BSD/macOS installs gtimeout via coreutils) +if command -v timeout >/dev/null 2>&1; then + TIMEOUT_CMD="timeout" +elif command -v gtimeout >/dev/null 2>&1; then + TIMEOUT_CMD="gtimeout" +else + TIMEOUT_CMD="" +fi + +for check in "${checks[@]}"; do + script="$SCRIPT_DIR/check-${check}.sh" + if [ ! -x "$script" ]; then continue; fi + if [ -n "$TIMEOUT_CMD" ]; then + DIFF_FILE="$DIFF_FILE" PR_BODY_FILE="$PR_BODY_FILE" \ + "$TIMEOUT_CMD" "${CHECK_TIMEOUT}s" "$script" < "$DIFF_FILE" \ + > "$TMPDIR_RUN/report-${check}.json" 2> "$TMPDIR_RUN/${check}.err" & + else + DIFF_FILE="$DIFF_FILE" PR_BODY_FILE="$PR_BODY_FILE" \ + "$script" < "$DIFF_FILE" \ + > "$TMPDIR_RUN/report-${check}.json" 2> "$TMPDIR_RUN/${check}.err" & + fi +done +wait + +# 6.5. Collect suppression tuples from files touched by the diff, then filter each report +touched_files=$(grep -oE '^\+\+\+ b/[^[:space:]]+' "$DIFF_FILE" 2>/dev/null | sed 's|^+++ b/||' | sort -u || true) +supp_file="$TMPDIR_RUN/suppressions.txt" +: > "$supp_file" +if [ -n "$touched_files" ]; then + while IFS= read -r f; do + [ -z "$f" ] && continue + [ -f "$REPO_ROOT/$f" ] || continue + bash "$LIB/suppression.sh" parse_line "$REPO_ROOT/$f" 2>/dev/null | sed "s|^$REPO_ROOT/|| ; s|^\./||" >> "$supp_file" || true + bash "$LIB/suppression.sh" parse_file "$REPO_ROOT/$f" 2>/dev/null | sed "s|^$REPO_ROOT/|| ; s|^\./||" >> "$supp_file" || true + done <<< "$touched_files" +fi + +for check in "${checks[@]}"; do + report="$TMPDIR_RUN/report-${check}.json" + [ -f "$report" ] || continue + filtered=$(bash "$LIB/apply-suppression.sh" apply "$report" "$supp_file" 2>/dev/null || cat "$report") + echo "$filtered" > "$report" +done + +# 7. Aggregate reports (applies tier gating per rules.yaml) +RULES_YAML="$REPO_ROOT/.claude/config/rules.yaml" \ + "$SCRIPT_DIR/aggregate.sh" "$TMPDIR_RUN" > "$TMPDIR_RUN/summary.json" + +# 8. Post signals (unless dry-run) +if [ "$DRY_RUN" = "true" ]; then + echo "" + echo "▶ DRY-RUN summary:" + jq -r ' + " BLOCK: \(.summary.block_count) FLAG: \(.summary.flag_count) SHADOW: \(.summary.shadow_count)", + "", + "Findings:", + (.findings[] | " [\(.severity)] \(.rule) at \(.file):\(.line) — \(.message)") + ' "$TMPDIR_RUN/summary.json" + echo "" + echo "▶ Full report at: $TMPDIR_RUN/summary.json" + exit "$(jq -r '.summary.block_count | if . > 0 then 1 else 0 end' "$TMPDIR_RUN/summary.json")" +fi + +# 9. Idempotency: delete prior bot artifacts +delete_prior_bot_artifacts "$PR_NUMBER" + +# 10. Post inline comments — for findings anchored to a real added line. +# EVERY finding is also listed in the summary (bug: inline-only findings looked +# "missing" from the Conversation tab). Anchored findings get a link to their +# inline comment; non-anchored ones (PR_BODY, missing files/dirs, commit +# metadata, synthetic "tests/" paths) are listed with a location hint. +all_findings_md=$(mktemp) +non_anchored=$(mktemp) +trap 'rm -f "$all_findings_md" "$non_anchored"' EXIT +inline_count=0 +n_inline=$(jq '.findings | length' "$TMPDIR_RUN/summary.json") +i=0 +while [ "$i" -lt "$n_inline" ]; do + f=$(jq -c ".findings[$i]" "$TMPDIR_RUN/summary.json") + file=$(echo "$f" | jq -r '.file') + line=$(echo "$f" | jq -r '.line') + rule=$(echo "$f" | jq -r '.rule') + sev=$(echo "$f" | jq -r '.severity') + msg=$(echo "$f" | jq -r '.message') + body="<!-- sdk-review:v1 check=$rule id=$rule-$i --> +**[$sev] $rule** + +$msg" + # Anchorable only if file:line is a real entry in the added-line set. + anchorable=false + if [ -f "${ADDED_LINES_FILE:-/nonexistent}" ] && grep -Fxq "${file}:${line}" "$ADDED_LINES_FILE" 2>/dev/null; then + anchorable=true + fi + if [ "$anchorable" = "true" ]; then + comment_url=$(post_inline_comment "$PR_NUMBER" "$HEAD_SHA" "$file" "$line" "$body" || true) + inline_count=$((inline_count + 1)) + if [ -n "$comment_url" ]; then + printf -- '- **[%s] %s** — %s ([inline on `%s:%s`](%s))\n' "$sev" "$rule" "$msg" "$file" "$line" "$comment_url" >> "$all_findings_md" + else + printf -- '- **[%s] %s** — %s _(inline on `%s:%s`)_\n' "$sev" "$rule" "$msg" "$file" "$line" >> "$all_findings_md" + fi + else + loc="$file" + [ "$line" != "0" ] && [ "$line" != "1" ] && loc="$file:$line" + printf -- '- **[%s] %s** — %s _(at `%s`)_\n' "$sev" "$rule" "$msg" "$loc" >> "$non_anchored" + printf -- '- **[%s] %s** — %s _(at `%s`, no code line to anchor)_\n' "$sev" "$rule" "$msg" "$loc" >> "$all_findings_md" + fi + i=$((i + 1)) +done + +# 11. Post summary comment +# The table + non-anchored findings section. Anchored findings appear as +# inline comments; non-anchored ones (PR body, missing files/dirs, commit +# metadata) are listed here so they are never invisible. +summary_table=$(jq -r ' + def status_icon: + if . == "PASS" then "✅ PASS" + elif . == "FLAG" then "⚠️ FLAG" + elif . == "BLOCK" then "❌ BLOCK" + elif . == "SHADOW" then "⏩ SHADOW" + else . end; + "| Check | Status | Findings |\n|-------|--------|----------|\n" + + (.per_check_summary | to_entries | map("| \(.key) | \(.value.status | status_icon) | \(.value.count) |") | join("\n")) +' "$TMPDIR_RUN/summary.json") + +non_anchored_count=0 +[ -s "$non_anchored" ] && non_anchored_count=$(grep -c '^- ' "$non_anchored" 2>/dev/null || echo 0) +total_findings=$(jq '.findings | length' "$TMPDIR_RUN/summary.json") + +# All-findings section — EVERY finding is listed here (anchored ones link to +# their inline comment; metadata ones show a location). This guarantees the +# Conversation tab shows the complete picture; nothing is inline-only-invisible. +all_findings_section="" +if [ -s "$all_findings_md" ]; then + all_findings_section=" + +### Findings ($total_findings) + +$(cat "$all_findings_md")" +fi + +# Reconciliation line so the counts visibly close. +reconcile="_${total_findings} finding(s): ${inline_count} posted as inline comment(s) on the affected lines, $(( total_findings - inline_count )) not tied to a code line (listed above)._" + +summary_body="<!-- sdk-review:v1 kind=summary --> +## SDK Module Review + +${summary_table} +${all_findings_section} + +${reconcile} + +--- +_Generated by sdk-review-skill · v1_" + +post_summary_comment "$PR_NUMBER" "$summary_body" + +# 12. Post check-run +conclusion=$(jq -r 'if .summary.block_count > 0 then "failure" elif .summary.flag_count > 0 then "neutral" else "success" end' "$TMPDIR_RUN/summary.json") +title=$(jq -r "\"SDK Review: \(.summary.block_count) BLOCK, \(.summary.flag_count) FLAG\"" "$TMPDIR_RUN/summary.json") +post_or_update_check_run "$HEAD_SHA" "$conclusion" "$title" "$summary_body" + +# 13. Apply label +case "$conclusion" in + success) label="sdk-review: ✅ passed" ;; + neutral) label="sdk-review: ⚠️ flagged" ;; + failure) label="sdk-review: ❌ blocked" ;; +esac +apply_label "$PR_NUMBER" "$label" + +# 14. Exit +exit "$(jq -r '.summary.block_count | if . > 0 then 1 else 0 end' "$TMPDIR_RUN/summary.json")" diff --git a/.claude/tests/test_fp_remediation.bats b/.claude/tests/test_fp_remediation.bats new file mode 100644 index 00000000..6f321217 --- /dev/null +++ b/.claude/tests/test_fp_remediation.bats @@ -0,0 +1,510 @@ +#!/usr/bin/env bats +# test_fp_remediation.bats — regression tests for the 8 FP-* fixes catalogued +# in docs/plans 09-FP-REMEDIATION.md §2. Each test pins ONE fix so a future +# regression is caught immediately. +# +# Runs standalone (uses ADDED_LINES_FILE to feed the hunk filter). If a check +# script or lib helper isn't present in the current batch, the test is skipped. + +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" +} + +# ------------------------------------------------------------ +# FP-A-01 — hunk attribution enforced +# ------------------------------------------------------------ + +@test "FP-A-01: is_line_touched respects ADDED_LINES_FILE" { + [ -f "$SCRIPT_DIR/lib/hunk-filter.sh" ] || skip "hunk-filter.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/added.txt" <<'EOF' +src/foo.py:5 +src/foo.py:6 +src/foo.py:7 +src/foo.py:8 +src/foo.py:9 +src/foo.py:10 +EOF + # touched + ADDED_LINES_FILE="$tmpd/added.txt" bash "$SCRIPT_DIR/lib/hunk-filter.sh" is_line_touched src/foo.py 7 + # not touched + run env ADDED_LINES_FILE="$tmpd/added.txt" bash "$SCRIPT_DIR/lib/hunk-filter.sh" is_line_touched src/foo.py 100 + [ "$status" -ne 0 ] + rm -rf "$tmpd" +} + +@test "FP-A-01: is_meta_finding treats PR_BODY / COMMIT:* as metadata" { + [ -f "$SCRIPT_DIR/lib/hunk-filter.sh" ] || skip "hunk-filter.sh not in this batch" + bash "$SCRIPT_DIR/lib/hunk-filter.sh" is_meta_finding PR_BODY + bash "$SCRIPT_DIR/lib/hunk-filter.sh" is_meta_finding COMMIT:abc123 + run bash "$SCRIPT_DIR/lib/hunk-filter.sh" is_meta_finding src/foo.py + [ "$status" -ne 0 ] +} + +# ------------------------------------------------------------ +# FP-B-01 — HC-01 ignores POM/XML namespaces +# ------------------------------------------------------------ + +@test "FP-B-01: HC-01 does not fire on pom.xml POM namespace" { + [ -f "$SCRIPT_DIR/check-hardcode.sh" ] || skip "check-hardcode.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/diff" <<'EOF' +diff --git a/pom.xml b/pom.xml +new file mode 100644 +--- /dev/null ++++ b/pom.xml +@@ -0,0 +1,5 @@ ++<?xml version="1.0"?> ++<project xmlns="http://maven.apache.org/POM/4.0.0" ++ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"> ++ <modelVersion>4.0.0</modelVersion> ++</project> +EOF + bash "$SCRIPT_DIR/lib/diff-added-lines.sh" < "$tmpd/diff" > "$tmpd/added.txt" + result=$(LANGUAGE=java ADDED_LINES_FILE="$tmpd/added.txt" DIFF_FILE="$tmpd/diff" bash "$SCRIPT_DIR/check-hardcode.sh") + hc01_count=$(echo "$result" | jq '[.findings[] | select(.rule=="HC-01")] | length') + [ "$hc01_count" = "0" ] + rm -rf "$tmpd" +} + +# ------------------------------------------------------------ +# FP-B-02 — HTTP-01 ignores markdown code fences +# ------------------------------------------------------------ + +@test "FP-B-02: HTTP-01 does not fire on .md files" { + [ -f "$SCRIPT_DIR/check-http-hygiene.sh" ] || skip "check-http-hygiene.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/diff" <<'EOF' +diff --git a/docs/user-guide.md b/docs/user-guide.md +new file mode 100644 +--- /dev/null ++++ b/docs/user-guide.md +@@ -0,0 +1,4 @@ ++# HTTP client usage ++```python ++client = httpx.Client() ++``` +EOF + bash "$SCRIPT_DIR/lib/diff-added-lines.sh" < "$tmpd/diff" > "$tmpd/added.txt" + result=$(ADDED_LINES_FILE="$tmpd/added.txt" DIFF_FILE="$tmpd/diff" bash "$SCRIPT_DIR/check-http-hygiene.sh") + http01_count=$(echo "$result" | jq '[.findings[] | select(.rule=="HTTP-01")] | length') + [ "$http01_count" = "0" ] + rm -rf "$tmpd" +} + +# ------------------------------------------------------------ +# FP-C-01 — BDD-01 accepts any *.feature file, not just <mod>.feature +# ------------------------------------------------------------ + +@test "FP-C-01: BDD-01 accepts scenarios.feature when module <mod>.feature is absent" { + [ -f "$SCRIPT_DIR/check-bdd.sh" ] || skip "check-bdd.sh not in this batch" + tmpd=$(mktemp -d) + # Simulate a new module `foo` with source file and a feature file NOT + # named foo.feature — BDD-01 must PASS (glob-based check). + mkdir -p "$tmpd/src/sap_cloud_sdk/foo" + echo "x = 1" > "$tmpd/src/sap_cloud_sdk/foo/client.py" + mkdir -p "$tmpd/tests/foo/integration" + echo "Feature: something else" > "$tmpd/tests/foo/integration/scenarios.feature" + cat > "$tmpd/diff" <<'EOF' +diff --git a/src/sap_cloud_sdk/foo/client.py b/src/sap_cloud_sdk/foo/client.py +new file mode 100644 +--- /dev/null ++++ b/src/sap_cloud_sdk/foo/client.py +@@ -0,0 +1,1 @@ ++x = 1 +EOF + result=$(REPO_ROOT="$tmpd" DIFF_FILE="$tmpd/diff" LANGUAGE=python bash "$SCRIPT_DIR/check-bdd.sh") + bdd01_count=$(echo "$result" | jq '[.findings[] | select(.rule=="BDD-01")] | length') + [ "$bdd01_count" = "0" ] + rm -rf "$tmpd" +} + +# ------------------------------------------------------------ +# FP-C-02 — PY-PT-04 accepts Exception subclass anywhere in module (AST) +# ------------------------------------------------------------ + +@test "FP-C-02: PY-PT-04 accepts Exception subclass in __init__.py (not exceptions.py)" { + [ -f "$SCRIPT_DIR/lib/ast_python_checks.py" ] || skip "ast_python_checks.py not in this batch" + tmpd=$(mktemp -d) + mkdir -p "$tmpd/foo" + cat > "$tmpd/foo/__init__.py" <<'EOF' +class FooError(Exception): + pass +EOF + # pt-04 helper returns exit 0 if module has any Exception subclass. + python3 "$SCRIPT_DIR/lib/ast_python_checks.py" pt-04 "$tmpd/foo" + # Negative: empty module → exit 1 + mkdir -p "$tmpd/bar" + echo "x = 1" > "$tmpd/bar/__init__.py" + run python3 "$SCRIPT_DIR/lib/ast_python_checks.py" pt-04 "$tmpd/bar" + [ "$status" -ne 0 ] + rm -rf "$tmpd" +} + +# ------------------------------------------------------------ +# FP-D-01 — PY-CON-01 skips generated model files + per-file cap +# ------------------------------------------------------------ + +@test "FP-D-01: PY-CON-01 skips _models.py files entirely" { + [ -f "$SCRIPT_DIR/lib/ast_python_checks.py" ] || skip "ast_python_checks.py not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/_models.py" <<'EOF' +class A: x = "value"; y = "value"; z = "value"; w = "value" +class B: x = "value"; y = "value"; z = "value" +EOF + result=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" con-01 "$tmpd/_models.py" 2>/dev/null) + # Expect zero findings (file skipped) + [ -z "$result" ] + rm -rf "$tmpd" +} + +@test "FP-D-01: PY-CON-01 caps at 3 findings per file" { + [ -f "$SCRIPT_DIR/lib/ast_python_checks.py" ] || skip "ast_python_checks.py not in this batch" + tmpd=$(mktemp -d) + # Six different repeated literals (each ≥3× and ≥3 chars) — should be capped to 3. + cat > "$tmpd/regular.py" <<'EOF' +a = "foo1234"; b = "foo1234"; c = "foo1234" +d = "bar1234"; e = "bar1234"; f = "bar1234" +g = "baz1234"; h = "baz1234"; i = "baz1234" +j = "qux1234"; k = "qux1234"; l = "qux1234" +m = "quux12"; n = "quux12"; o = "quux12" +p = "corge2"; q = "corge2"; r = "corge2" +EOF + count=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" con-01 "$tmpd/regular.py" 2>/dev/null | wc -l | tr -d ' ') + [ "$count" -le 3 ] + rm -rf "$tmpd" +} + +# ------------------------------------------------------------ +# FP-E-01 — PY-PT-08 baseline suppresses pre-existing lines +# ------------------------------------------------------------ + +@test "FP-E-01: is_in_line_baseline matches (rule,file,line) triple" { + [ -f "$SCRIPT_DIR/lib/baseline.sh" ] || skip "baseline.sh not in this batch" + # Skip if this batch's baseline.sh lacks the new function. + grep -q "is_in_line_baseline" "$SCRIPT_DIR/lib/baseline.sh" || skip "baseline.sh lacks is_in_line_baseline" + tmpd=$(mktemp -d) + cat > "$tmpd/baseline.json" <<'EOF' +{ + "line_baseline": [ + {"rule": "PY-PT-08", "file": "src/foo/__init__.py", "line": 81}, + {"rule": "PY-PT-08", "file": "src/foo/__init__.py", "line": 127} + ] +} +EOF + BASELINE_FILE="$tmpd/baseline.json" bash "$SCRIPT_DIR/lib/baseline.sh" is_in_line_baseline PY-PT-08 src/foo/__init__.py 81 + BASELINE_FILE="$tmpd/baseline.json" bash "$SCRIPT_DIR/lib/baseline.sh" is_in_line_baseline PY-PT-08 src/foo/__init__.py 127 + # Not baselined + run env BASELINE_FILE="$tmpd/baseline.json" bash "$SCRIPT_DIR/lib/baseline.sh" is_in_line_baseline PY-PT-08 src/foo/__init__.py 999 + [ "$status" -ne 0 ] + # Prefix-guard: 81 must NOT match 812 + cat > "$tmpd/baseline.json" <<'EOF' +{"line_baseline": [{"rule":"PY-PT-08","file":"src/foo.py","line":81}]} +EOF + run env BASELINE_FILE="$tmpd/baseline.json" bash "$SCRIPT_DIR/lib/baseline.sh" is_in_line_baseline PY-PT-08 src/foo.py 812 + [ "$status" -ne 0 ] + rm -rf "$tmpd" +} + +# ------------------------------------------------------------ +# DEL-01 guardrail — synthetic path is not filtered out +# ------------------------------------------------------------ + +@test "DEL-01: guardrail — synthetic src/ path is treated as metadata by hunk filter" { + [ -f "$SCRIPT_DIR/lib/hunk-filter.sh" ] || skip "hunk-filter.sh not in this batch" + # is_meta_finding returns 0 for special paths. src/ is a real path so we test + # the emit_finding_if_touched path — DEL-01's src/:1 emission bypasses the + # filter because the calling script does NOT wrap it (verified via grep). + ! grep -q "emit_finding_if_touched \"DEL-01\"" "$SCRIPT_DIR/check-deletion-hygiene.sh" + # DEL-01 is called via plain emit_finding — the fix is deliberate. +} + +# ------------------------------------------------------------ +# FP-G-01 — peer-consistency for PT-01 (no more "factory required" law) +# ------------------------------------------------------------ + +@test "FP-G-01: peer_element_fraction returns adopted/total/fraction" { + [ -f "$SCRIPT_DIR/lib/peer-consistency.sh" ] || skip "peer-consistency.sh not in this batch" + tmpd=$(mktemp -d) + mkdir -p "$tmpd/src/sap_cloud_sdk/a" "$tmpd/src/sap_cloud_sdk/b" "$tmpd/src/sap_cloud_sdk/c" + # a and b have user-guide.md; c doesn't + touch "$tmpd/src/sap_cloud_sdk/a/user-guide.md" "$tmpd/src/sap_cloud_sdk/b/user-guide.md" + result=$(bash "$SCRIPT_DIR/lib/peer-consistency.sh" peer_element_fraction "$tmpd" python user-guide.md) + adopted=$(echo "$result" | awk '{print $1}') + total=$(echo "$result" | awk '{print $2}') + [ "$adopted" = "2" ] + [ "$total" = "3" ] + rm -rf "$tmpd" +} + +@test "FP-G-01: PT-01 fires FLAG when new module lacks a >=80% adopted element" { + [ -f "$SCRIPT_DIR/check-patterns.sh" ] || skip "check-patterns.sh not in this batch" + tmpd=$(mktemp -d) + # Peers a, b, c all have user-guide.md (100%). New module `foo` doesn't. + mkdir -p "$tmpd/src/sap_cloud_sdk/"{a,b,c,foo} + for m in a b c; do + touch "$tmpd/src/sap_cloud_sdk/$m/user-guide.md" + touch "$tmpd/src/sap_cloud_sdk/$m/client.py" + echo "def create_${m}_client(): pass" > "$tmpd/src/sap_cloud_sdk/$m/client.py" + done + # foo has client.py but no user-guide.md → should FLAG + echo "class FooClient: pass" > "$tmpd/src/sap_cloud_sdk/foo/client.py" + # Diff creates foo — client shape + cat > "$tmpd/diff" <<'EOF' +diff --git a/src/sap_cloud_sdk/foo/client.py b/src/sap_cloud_sdk/foo/client.py +new file mode 100644 +--- /dev/null ++++ b/src/sap_cloud_sdk/foo/client.py +@@ -0,0 +1,1 @@ ++class FooClient: pass +EOF + result=$(REPO_ROOT="$tmpd" DIFF_FILE="$tmpd/diff" LANGUAGE=python bash "$SCRIPT_DIR/check-patterns.sh" 2>/dev/null) + # PY-PT-01 should fire FLAG (not BLOCK) mentioning user-guide.md + pt01_flag=$(echo "$result" | jq '[.findings[] | select(.rule=="PY-PT-01" and .severity=="FLAG")] | length') + [ "$pt01_flag" -ge 1 ] + pt01_block=$(echo "$result" | jq '[.findings[] | select(.rule=="PY-PT-01" and .severity=="BLOCK")] | length') + [ "$pt01_block" = "0" ] + rm -rf "$tmpd" +} + +@test "FP-G-01: PT-01 does NOT fire when peers <80% adopt the element (factory case)" { + [ -f "$SCRIPT_DIR/check-patterns.sh" ] || skip "check-patterns.sh not in this batch" + tmpd=$(mktemp -d) + # 3 peers: only 1 has create_*_client → 33% adoption of factory element. + mkdir -p "$tmpd/src/sap_cloud_sdk/"{a,b,c,foo} + echo "def create_a_client(): pass" > "$tmpd/src/sap_cloud_sdk/a/client.py" + echo "class BClient: pass" > "$tmpd/src/sap_cloud_sdk/b/client.py" + echo "class CClient: pass" > "$tmpd/src/sap_cloud_sdk/c/client.py" + echo "class FooClient: pass" > "$tmpd/src/sap_cloud_sdk/foo/client.py" + cat > "$tmpd/diff" <<'EOF' +diff --git a/src/sap_cloud_sdk/foo/client.py b/src/sap_cloud_sdk/foo/client.py +new file mode 100644 +--- /dev/null ++++ b/src/sap_cloud_sdk/foo/client.py +@@ -0,0 +1,1 @@ ++class FooClient: pass +EOF + result=$(REPO_ROOT="$tmpd" DIFF_FILE="$tmpd/diff" LANGUAGE=python bash "$SCRIPT_DIR/check-patterns.sh" 2>/dev/null) + # No factory-related PT-01 finding + factory_flag=$(echo "$result" | jq '[.findings[] | select(.rule=="PY-PT-01" and (.message | contains("factory")))] | length') + [ "$factory_flag" = "0" ] + rm -rf "$tmpd" +} + +@test "FP-G-01: PT-01 emits zero findings when module has every universal element" { + [ -f "$SCRIPT_DIR/check-patterns.sh" ] || skip "check-patterns.sh not in this batch" + tmpd=$(mktemp -d) + # 3 peers with user-guide.md; foo also has it and everything else uncommon. + mkdir -p "$tmpd/src/sap_cloud_sdk/"{a,b,c,foo} + for m in a b c foo; do + touch "$tmpd/src/sap_cloud_sdk/$m/user-guide.md" + echo "class ${m^}Client: pass" > "$tmpd/src/sap_cloud_sdk/$m/client.py" + done + cat > "$tmpd/diff" <<'EOF' +diff --git a/src/sap_cloud_sdk/foo/client.py b/src/sap_cloud_sdk/foo/client.py +new file mode 100644 +--- /dev/null ++++ b/src/sap_cloud_sdk/foo/client.py +@@ -0,0 +1,1 @@ ++class FooClient: pass +EOF + result=$(REPO_ROOT="$tmpd" DIFF_FILE="$tmpd/diff" LANGUAGE=python bash "$SCRIPT_DIR/check-patterns.sh" 2>/dev/null) + pt01_count=$(echo "$result" | jq '[.findings[] | select(.rule=="PY-PT-01")] | length') + [ "$pt01_count" = "0" ] + rm -rf "$tmpd" +} + +@test "FP-H-01: HC-01 does not fire on uv.lock" { + [ -f "$SCRIPT_DIR/check-hardcode.sh" ] || skip "check-hardcode.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/diff" <<'DIFF' +diff --git a/uv.lock b/uv.lock +new file mode 100644 +--- /dev/null ++++ b/uv.lock +@@ -0,0 +1,3 @@ ++url = "https://files.pythonhosted.org/packages/aa/bb/foo-1.0.tar.gz" ++url = "https://files.pythonhosted.org/packages/cc/dd/bar-2.0.tar.gz" ++other = "value" +DIFF + result=$(DIFF_FILE="$tmpd/diff" LANGUAGE=python bash "$SCRIPT_DIR/check-hardcode.sh" 2>/dev/null) + hc01_count=$(echo "$result" | jq '[.findings[] | select(.rule=="HC-01")] | length') + [ "$hc01_count" = "0" ] + rm -rf "$tmpd" +} + +@test "FP-H-01: HC-01 does not fire on poetry.lock / package-lock.json" { + [ -f "$SCRIPT_DIR/check-hardcode.sh" ] || skip "check-hardcode.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/diff" <<'DIFF' +diff --git a/poetry.lock b/poetry.lock +new file mode 100644 +--- /dev/null ++++ b/poetry.lock +@@ -0,0 +1,1 @@ ++url = "https://files.pythonhosted.org/packages/foo.tar.gz" +diff --git a/package-lock.json b/package-lock.json +new file mode 100644 +--- /dev/null ++++ b/package-lock.json +@@ -0,0 +1,1 @@ ++"resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz" +DIFF + result=$(DIFF_FILE="$tmpd/diff" LANGUAGE=python bash "$SCRIPT_DIR/check-hardcode.sh" 2>/dev/null) + hc01_count=$(echo "$result" | jq '[.findings[] | select(.rule=="HC-01")] | length') + [ "$hc01_count" = "0" ] + rm -rf "$tmpd" +} + +@test "FP-I-01: HC-01 does not fire on .env.example templates" { + [ -f "$SCRIPT_DIR/check-hardcode.sh" ] || skip "check-hardcode.sh not in this batch" + tmpd=$(mktemp -d) + cat > "$tmpd/diff" <<'DIFF' +diff --git a/.env_integration_tests.example b/.env_integration_tests.example +new file mode 100644 +--- /dev/null ++++ b/.env_integration_tests.example +@@ -0,0 +1,3 @@ ++API_URL=https://your-api-url-here.com ++AUTH_URL=https://your-auth-url.example.com ++OTHER=value +DIFF + result=$(DIFF_FILE="$tmpd/diff" LANGUAGE=python bash "$SCRIPT_DIR/check-hardcode.sh" 2>/dev/null) + hc01_count=$(echo "$result" | jq '[.findings[] | select(.rule=="HC-01")] | length') + [ "$hc01_count" = "0" ] + rm -rf "$tmpd" +} + +@test "FP-J-01: LIC-01/02 skips when repo lacks SPDX in existing files" { + [ -f "$SCRIPT_DIR/check-license-spdx.sh" ] || skip "check-license-spdx.sh not in this batch" + tmpd=$(mktemp -d) + mkdir -p "$tmpd/src/main/java/com/sap" + # Create 5 existing java files, none with SPDX + for i in 1 2 3 4 5; do + echo "package com.sap;" > "$tmpd/src/main/java/com/sap/Foo${i}.java" + echo "public class Foo${i} {}" >> "$tmpd/src/main/java/com/sap/Foo${i}.java" + done + cat > "$tmpd/diff" <<'DIFF' +diff --git a/src/main/java/com/sap/NewClient.java b/src/main/java/com/sap/NewClient.java +new file mode 100644 +--- /dev/null ++++ b/src/main/java/com/sap/NewClient.java +@@ -0,0 +1,2 @@ ++package com.sap; ++public class NewClient {} +DIFF + result=$(REPO_ROOT="$tmpd" DIFF_FILE="$tmpd/diff" LANGUAGE=java bash "$SCRIPT_DIR/check-license-spdx.sh" 2>/dev/null) + lic_count=$(echo "$result" | jq '[.findings[] | select(.rule | startswith("LIC-"))] | length') + [ "$lic_count" = "0" ] + # Should have a pass_criteria_met entry mentioning no consistent adoption + no_adoption=$(echo "$result" | jq '.summary.pass_criteria_met | any(contains("no consistent SPDX adoption"))') + [ "$no_adoption" = "true" ] + rm -rf "$tmpd" +} + +@test "FP-J-01: LIC-01/02 still fires when repo has SPDX in existing files" { + [ -f "$SCRIPT_DIR/check-license-spdx.sh" ] || skip "check-license-spdx.sh not in this batch" + tmpd=$(mktemp -d) + mkdir -p "$tmpd/src/main/java/com/sap" + # Create 5 existing files, all with SPDX + for i in 1 2 3 4 5; do + cat > "$tmpd/src/main/java/com/sap/Foo${i}.java" <<HDR +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: 2026 SAP SE +package com.sap; +public class Foo${i} {} +HDR + done + cat > "$tmpd/diff" <<'DIFF' +diff --git a/src/main/java/com/sap/NewClient.java b/src/main/java/com/sap/NewClient.java +new file mode 100644 +--- /dev/null ++++ b/src/main/java/com/sap/NewClient.java +@@ -0,0 +1,2 @@ ++package com.sap; ++public class NewClient {} +DIFF + result=$(REPO_ROOT="$tmpd" DIFF_FILE="$tmpd/diff" LANGUAGE=java bash "$SCRIPT_DIR/check-license-spdx.sh" 2>/dev/null) + lic01=$(echo "$result" | jq '[.findings[] | select(.rule=="LIC-01")] | length') + [ "$lic01" = "1" ] + rm -rf "$tmpd" +} + +@test "FP-K-01: PY-CON-01 does not count occurrences outside PR added-lines set" { + [ -f "$SCRIPT_DIR/lib/ast_python_checks.py" ] || skip "ast_python_checks.py not present" + tmpd=$(mktemp -d) + + # File with 5 occurrences of a repeated string; PR only touches unrelated lines + cat > "$tmpd/module.py" <<'PY' +"""module.""" +CONST_A = "value-1" + +def a(): + logger.info("customer credentials at path") # line 5 + +def b(): + logger.info("customer credentials at path") # line 8 + +def c(): + logger.info("customer credentials at path") # line 11 + +def d(): + logger.info("customer credentials at path") # line 14 + +def e(): + logger.info("customer credentials at path") # line 17 +PY + + # Added-lines set says PR only touched lines 1 and 2 (unrelated docstring/const) + cat > "$tmpd/added-lines.txt" <<'ADDED' +$tmpd/module.py:1 +$tmpd/module.py:2 +ADDED + # Substitute the actual tmp path into the added-lines file + sed -i '' "s|\$tmpd|$tmpd|g" "$tmpd/added-lines.txt" + + ADDED_LINES_FILE="$tmpd/added-lines.txt" \ + python3 "$SCRIPT_DIR/lib/ast_python_checks.py" con-01 "$tmpd/module.py" > "$tmpd/out.jsonl" + + count=$(wc -l < "$tmpd/out.jsonl" | tr -d ' ') + # Should be 0 because none of the 5 occurrences are on added lines + [ "$count" = "0" ] + rm -rf "$tmpd" +} + +@test "FP-K-01: PY-CON-01 fires when at least one occurrence is on an added line" { + [ -f "$SCRIPT_DIR/lib/ast_python_checks.py" ] || skip "ast_python_checks.py not present" + tmpd=$(mktemp -d) + + cat > "$tmpd/module.py" <<'PY' +"""module.""" + +def a(): + logger.info("customer credentials at path") # line 4 + +def b(): + logger.info("customer credentials at path") # line 7 + +def c(): + logger.info("customer credentials at path") # line 10 +PY + + # PR touches line 4 (one of the occurrences) + cat > "$tmpd/added-lines.txt" <<ADDED +$tmpd/module.py:4 +ADDED + + ADDED_LINES_FILE="$tmpd/added-lines.txt" \ + python3 "$SCRIPT_DIR/lib/ast_python_checks.py" con-01 "$tmpd/module.py" > "$tmpd/out.jsonl" + + count=$(wc -l < "$tmpd/out.jsonl" | tr -d ' ') + [ "$count" = "1" ] + # Anchor line should be the added-line occurrence (4), not the first overall + anchor=$(jq -r '.line' "$tmpd/out.jsonl") + [ "$anchor" = "4" ] + rm -rf "$tmpd" +} diff --git a/.claude/tests/test_signals.bats b/.claude/tests/test_signals.bats new file mode 100644 index 00000000..271b4941 --- /dev/null +++ b/.claude/tests/test_signals.bats @@ -0,0 +1,98 @@ +#!/usr/bin/env bats +# test_signals.bats — regression tests for the PR-signal invariants that broke +# during live posting (see conversation 2026-07-09). Each test pins ONE +# reported problem so it can't silently return: +# +# 1. per_check_summary table count MUST reconcile with posted findings +# (bug-T-01: table showed 6 while only 3 findings posted). +# 2. findings that survive tier-gating carry their originating check name. +# 3. shadow/OFF-tier findings are NOT counted in the table. +# 4. aggregate output is always valid JSON with the required keys. + +setup() { + SCRIPT_DIR="$BATS_TEST_DIRNAME/../.." + AGG="$SCRIPT_DIR/.claude/scripts/aggregate.sh" + RULES="$SCRIPT_DIR/.claude/config/rules.yaml" +} + +# Build a throwaway TMPDIR_RUN with synthetic report-*.json files. +_mk_reports() { + local d="$1" + mkdir -p "$d" + # hardcode: 2 FLAG findings (HC-04) — should post + cat > "$d/report-hardcode.json" <<'JSON' +{"check":"hardcode","version":"1.0.0","language":"java","status":"FLAG","started_at":"t","duration_ms":0,"modules_analysed":[], + "findings":[ + {"rule":"HC-04","severity":"FLAG","file":"a/Foo.java","line":10,"message":"getenv","suggestion":""}, + {"rule":"HC-04","severity":"FLAG","file":"a/Foo.java","line":12,"message":"getenv","suggestion":""} + ], + "summary":{"block_count":0,"flag_count":2}} +JSON + # pr-size: 2 findings but rule is SHADOW tier → must be filtered out of count + cat > "$d/report-pr-size.json" <<'JSON' +{"check":"pr-size","version":"1.0.0","language":"java","status":"FLAG","started_at":"t","duration_ms":0,"modules_analysed":[], + "findings":[ + {"rule":"PR-SIZE-01","severity":"FLAG","file":".","line":1,"message":"big","suggestion":""}, + {"rule":"PR-SIZE-02","severity":"FLAG","file":".","line":1,"message":"many files","suggestion":""} + ], + "summary":{"block_count":0,"flag_count":2}} +JSON + # a clean check with zero findings — must still appear as PASS/0 + cat > "$d/report-secrets.json" <<'JSON' +{"check":"secrets","version":"1.0.0","language":"java","status":"PASS","started_at":"t","duration_ms":0,"modules_analysed":[], + "findings":[],"summary":{"block_count":0,"flag_count":0}} +JSON +} + +@test "signals: aggregate output is valid JSON with required keys" { + [ -f "$AGG" ] || skip "aggregate.sh not in this batch" + tmpd=$(mktemp -d); _mk_reports "$tmpd" + out=$(RULES_YAML="$RULES" bash "$AGG" "$tmpd") + echo "$out" | jq -e '.findings and .per_check_summary and .summary' >/dev/null + rm -rf "$tmpd" +} + +@test "signals: table count reconciles with posted findings (bug-T-01)" { + [ -f "$AGG" ] || skip "aggregate.sh not in this batch" + tmpd=$(mktemp -d); _mk_reports "$tmpd" + out=$(RULES_YAML="$RULES" bash "$AGG" "$tmpd") + total=$(echo "$out" | jq '.findings | length') + table_sum=$(echo "$out" | jq '[.per_check_summary[].count] | add') + # The sum of per-check counts MUST equal the number of posted findings. + [ "$total" = "$table_sum" ] + rm -rf "$tmpd" +} + +@test "signals: SHADOW-tier findings are excluded from the table count" { + [ -f "$AGG" ] || skip "aggregate.sh not in this batch" + [ -f "$RULES" ] || skip "rules.yaml not in this batch" + # Only assert if PR-SIZE-01 is actually SHADOW in rules.yaml (its configured tier). + tier=$(grep -oE 'PR-SIZE-01:[[:space:]]*\{[^}]*tier:[[:space:]]*[A-Z_]+' "$RULES" | grep -oE 'tier:[[:space:]]*[A-Z_]+' | grep -oE '[A-Z_]+$' || echo "") + if [ "$tier" != "SHADOW" ]; then skip "PR-SIZE-01 not SHADOW tier (is '$tier')"; fi + tmpd=$(mktemp -d); _mk_reports "$tmpd" + out=$(RULES_YAML="$RULES" bash "$AGG" "$tmpd") + prsize_count=$(echo "$out" | jq '.per_check_summary["pr-size"].count') + [ "$prsize_count" = "0" ] + rm -rf "$tmpd" +} + +@test "signals: clean check still appears as PASS with 0 count" { + [ -f "$AGG" ] || skip "aggregate.sh not in this batch" + tmpd=$(mktemp -d); _mk_reports "$tmpd" + out=$(RULES_YAML="$RULES" bash "$AGG" "$tmpd") + status=$(echo "$out" | jq -r '.per_check_summary["secrets"].status') + count=$(echo "$out" | jq -r '.per_check_summary["secrets"].count') + [ "$status" = "PASS" ] + [ "$count" = "0" ] + rm -rf "$tmpd" +} + +@test "signals: posted findings carry their originating check name" { + [ -f "$AGG" ] || skip "aggregate.sh not in this batch" + tmpd=$(mktemp -d); _mk_reports "$tmpd" + out=$(RULES_YAML="$RULES" bash "$AGG" "$tmpd") + # Every posted finding must have a non-null .check (needed for the table). + missing=$(echo "$out" | jq '[.findings[] | select(.check == null)] | length') + [ "$missing" = "0" ] + rm -rf "$tmpd" +} diff --git a/.github/workflows/sdk-module-review.yml b/.github/workflows/sdk-module-review.yml new file mode 100644 index 00000000..ed45abe3 --- /dev/null +++ b/.github/workflows/sdk-module-review.yml @@ -0,0 +1,42 @@ +name: SDK Module Review +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review] + workflow_dispatch: + inputs: + pr_number: + description: "PR number to review" + required: true + +jobs: + review: + runs-on: ubuntu-latest + if: github.event.pull_request.draft == false || github.event_name == 'workflow_dispatch' + permissions: + contents: read + pull-requests: write + checks: write + issues: write + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Install jq + run: sudo apt-get install -y jq + - name: Install Python + uses: actions/setup-python@v5 + with: + python-version: '3.11' + - name: Checkout sibling SDK repo (BDD parity) + uses: actions/checkout@v4 + with: + repository: ${{ vars.SIBLING_SDK_REPO }} + path: .sibling-sdk + token: ${{ secrets.SIBLING_SDK_TOKEN }} + continue-on-error: true + - name: Run SDK review + env: + PR_NUMBER: ${{ github.event.pull_request.number || inputs.pr_number }} + SDK_SIBLING_PATH: ${{ github.workspace }}/.sibling-sdk + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: bash .claude/scripts/orchestrate.sh "$PR_NUMBER" diff --git a/.github/workflows/sdk-skill-isolation-check.yml b/.github/workflows/sdk-skill-isolation-check.yml new file mode 100644 index 00000000..0ec52609 --- /dev/null +++ b/.github/workflows/sdk-skill-isolation-check.yml @@ -0,0 +1,71 @@ +name: SDK Skill Isolation Check +# Validates that sub-PRs into feat/sdk-review-skill satisfy the isolation criteria +# from 06-INCREMENTAL-DELIVERY.md §isolation criteria. +on: + pull_request: + branches: [feat/sdk-review-skill] + +jobs: + isolation: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Install tools + run: | + sudo apt-get update && sudo apt-get install -y jq shellcheck bats + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: '3.11' + - name: Install ruff + run: pip install ruff + - name: 1. Compiles alone — shellcheck + run: | + shellcheck -x --severity=error .claude/scripts/**/*.sh + - name: 1. Compiles alone — ruff + run: | + ruff check .claude/scripts/lib/*.py + - name: 2. Testable alone — bats + run: | + bats tests/sdk-review/*.bats + - name: 3. Revert-safe — nothing in main branch broken + run: | + # Verify that scripts still function after this PR is applied + bash .claude/scripts/lib/detect-language.sh /tmp || echo "detect-language exit acceptable" + bash .claude/scripts/lib/diff-added-lines.sh < /dev/null || echo "diff-added-lines exit acceptable" + - name: 4. Sub-PR size check + run: | + # Warn if PR exceeds max sub-PR size (400 LOC per 06-INCREMENTAL-DELIVERY.md) + adds=$(git diff origin/feat/sdk-review-skill...HEAD --shortstat | grep -oE '[0-9]+ insertion' | grep -oE '[0-9]+' || echo 0) + if [ "${adds:-0}" -gt 400 ]; then + echo "::warning::Sub-PR has $adds additions (>400 target). Consider splitting per 06-INCREMENTAL-DELIVERY.md." + fi + - name: 5. Fixture-tested check (advisory) + run: | + # Every new check-*.sh should ideally have at least one fixture. + # We only enforce this for the safety-critical checks that map to + # locked rules (secrets, license, disclosure, hardcode, binding); + # other checks are covered by end-to-end orchestrate.sh dry-run + # tests against real validation PRs. + required_fixtures="secrets license-spdx disclosure hardcode binding-shape" + missing="" + for check_name in $required_fixtures; do + case "$check_name" in + license-spdx) pattern="tests/sdk-review/fixtures/clean.diff" ;; + *) pattern="tests/sdk-review/fixtures/${check_name}*.diff" ;; + esac + # shellcheck disable=SC2086 + if ! ls $pattern >/dev/null 2>&1; then + # Only fail if the check script exists AND fixture doesn't + if [ -f ".claude/scripts/check-${check_name}.sh" ]; then + missing="$missing $check_name" + fi + fi + done + if [ -n "$missing" ]; then + echo "::error::Safety-critical checks missing fixtures:$missing" + exit 1 + fi + echo "All safety-critical fixtures present." diff --git a/docs/BRANCH-PROTECTION-SETUP.md b/docs/BRANCH-PROTECTION-SETUP.md new file mode 100644 index 00000000..ceec9ce2 --- /dev/null +++ b/docs/BRANCH-PROTECTION-SETUP.md @@ -0,0 +1,177 @@ +# Branch Protection Setup — sdk-module-review as required check + +This guide is for **repo admins**. It documents how to configure the +`sdk-module-review` action as a required status check so that PRs cannot be +merged until the review passes. + +Once configured, every PR against `main` will be blocked from merge until: +1. The `sdk-module-review` action runs +2. The action reports success (no `BLOCK` findings) +3. All other required checks (existing CI, REUSE, etc.) also pass + +--- + +## Prerequisites + +- You have **admin** permission on the repo +- The `feat/sdk-review-skill` PR has been merged to `main`, so + `.github/workflows/sdk-module-review.yml` is live +- At least one PR has run the workflow successfully (so GitHub knows the + check-name `sdk-module-review` exists — required checks can only be added + after they've fired at least once) + +## Steps (GitHub UI) + +1. Navigate to **Settings → Branches → Branch protection rules** +2. If a rule already exists for `main`, edit it. Otherwise click **Add rule** + and enter `main` as the branch name pattern. +3. Under **Protect matching branches**, enable: + - ☑ **Require a pull request before merging** + - ☑ Require approvals: `1` (or more per team convention) + - ☑ Dismiss stale pull request approvals when new commits are pushed + - ☑ Require review from Code Owners + - ☑ **Require status checks to pass before merging** + - ☑ Require branches to be up to date before merging + - In the search box, add: + - `sdk-module-review` ← our skill + - `test` (or whatever the existing CI is called) + - `reuse` (if REUSE-check is used) + - ☑ **Require conversation resolution before merging** + - ☑ **Do not allow bypassing the above settings** (admins included) +4. Click **Create** or **Save changes** + +## Steps (via gh CLI, alternative) + +```bash +# cloud-sdk-python (public GitHub) +gh api -X PUT "repos/SAP/cloud-sdk-python/branches/main/protection" \ + --input - <<'EOF' +{ + "required_status_checks": { + "strict": true, + "contexts": ["sdk-module-review", "test", "reuse"] + }, + "enforce_admins": true, + "required_pull_request_reviews": { + "dismiss_stale_reviews": true, + "require_code_owner_reviews": true, + "required_approving_review_count": 1 + }, + "restrictions": null, + "required_conversation_resolution": true +} +EOF + +# cloud-sdk-java (internal GHES) +gh api --hostname github.tools.sap -X PUT \ + "repos/application-foundation/cloud-sdk-java/branches/main/protection" \ + --input - <<'EOF' +{ + "required_status_checks": { + "strict": true, + "contexts": ["sdk-module-review", "ci", "codeql-sast-analysis"] + }, + "enforce_admins": true, + "required_pull_request_reviews": { + "dismiss_stale_reviews": true, + "require_code_owner_reviews": true, + "required_approving_review_count": 1 + }, + "restrictions": null +} +EOF +``` + +Adjust the `contexts` list to include the CI check-names actually used by the +repo (see the "Checks" tab of any recent PR to confirm the exact names). + +## Verify + +After configuration, open a test PR (or use any open PR): + +```bash +gh pr view <PR_NUMBER> --json statusCheckRollup -q '.statusCheckRollup[].name' +``` + +You should see `sdk-module-review` in the list. If the check hasn't run yet +(never fired on this branch), it will appear as "expected" (pending). + +## Rollout stages + +We recommend a **SHADOW → FLAG → BLOCK** progression to minimise contributor +friction: + +### Stage 1 — SHADOW (weeks 1–2) + +- The workflow runs on every PR but **rules are downgraded to `SHADOW`** +- Findings are logged to `.claude/telemetry/*.jsonl` but **not posted to the PR** +- Contributors are unaffected; maintainers observe FP rate + +To enable SHADOW mode, edit `.claude/config/rules.yaml`: +```yaml +rules: + # temporarily downgrade all BLOCK to SHADOW for first two weeks + BND-02: { tier: SHADOW } + BREAKING-01: { tier: SHADOW } + # ... etc +``` + +Or set an env var in the workflow: `SDK_REVIEW_TIER_OVERRIDE=shadow-all`. + +### Stage 2 — FLAG (weeks 3–4) + +- Rules promoted to `FLAG` — findings posted as inline comments and summary, + but check-run is **not** required for merge +- Contributors see the review and can act on it +- Maintainers verify FP rate stays < 5 % on real PRs + +### Stage 3 — BLOCK (week 5+) + +- `sdk-module-review` added as required status check (this document's main topic) +- Merges blocked on `BLOCK` findings +- `BLOCK_LOCKED` rules (secrets, SPDX in public, token URL concat, breaking + changes without declaration) are non-suppressible + +## Rollback + +If `sdk-module-review` fires too aggressively and needs to be turned off: + +1. Remove `sdk-module-review` from the required-checks list (via UI or `gh api`) +2. Or disable the workflow entirely: `.github/workflows/sdk-module-review.yml` → + set `on: workflow_dispatch` only (no auto-triggers) +3. Individual noisy rules can be downgraded in `.claude/config/rules.yaml`: + ```yaml + rules: + RULE-ID: { tier: FLAG } # was BLOCK; downgrade to advisory + RULE-ID: { tier: SHADOW } # or silence entirely + ``` + +`BLOCK_LOCKED` rules cannot be downgraded via config — they are safety-critical +(secrets, license, disclosure in public repo, BTP token URL concat). To +temporarily disable one, remove it from `rules.yaml` altogether and open a +follow-up issue to reintroduce it. + +## Troubleshooting + +- **Check-run not appearing on new PRs**: verify the workflow file + `.github/workflows/sdk-module-review.yml` is present on `main` and the + action has permissions `contents: read`, `pull-requests: write`, + `checks: write`, `issues: write` (for labels) +- **Check-run runs but never completes**: check the workflow logs for auth + errors (`gh auth status`) or missing `SIBLING_SDK_TOKEN` secret (cross-repo + BDD parity is optional — degrades to `FLAG` if unavailable) +- **False positives**: see `docs/PR-REVIEW.md § Suppressing false positives` + and `§ Tuning noisy rules` +- **Required check missing from branch protection dropdown**: the check must + have fired at least once on any branch before GitHub lists it. Open a + throwaway PR to trigger it, or dispatch the workflow manually: + ```bash + gh workflow run sdk-module-review.yml -f pr_number=<N> + ``` + +## Cross-references + +- [`docs/PR-REVIEW.md`](./PR-REVIEW.md) — user-facing docs on what the skill checks +- [`.claude/config/rules.yaml`](../.claude/config/rules.yaml) — rule catalog with tiers +- [`.claude/config/baseline.json`](../.claude/config/baseline.json) — repo-specific exemptions +- [`.github/workflows/sdk-module-review.yml`](../.github/workflows/sdk-module-review.yml) — the workflow diff --git a/docs/PR-REVIEW.md b/docs/PR-REVIEW.md new file mode 100644 index 00000000..e42dfe32 --- /dev/null +++ b/docs/PR-REVIEW.md @@ -0,0 +1,245 @@ +# SDK Module Review + +Every PR to this repo is reviewed automatically by the **SDK Module Review** skill. +This document explains what it checks, how to interact with findings, and how to tune noisy rules. + +> **Required before review.** The `sdk-module-review` check-run must be green **before a maintainer will review the PR**. It is a required status check in branch protection — merges are blocked until it passes. Reviewers will not spend time on PRs where the automated review is failing. + +--- + +## How it runs + +**Automatic (GitHub Action):** fires on every PR event (`opened`, `synchronize`, `reopened`, `ready_for_review`). No action needed from the contributor — the review appears on the PR within a minute or two. + +**Manual (maintainer, via CLI):** +```bash +gh workflow run sdk-module-review.yml -f pr_number=<N> +``` + +**Local (dev iteration, requires Claude Code):** +``` +/review-new-module <PR_NUMBER> # full review, posts to PR +/review-new-module <PR_NUMBER> --dry-run # analysis only, prints locally +``` + +The skill is **100% deterministic** — no LLM calls in CI. Every run on the same +commit produces the same findings. + +--- + +## Signals posted to your PR + +Every run posts up to four signals: + +1. **Inline comments** — one per finding, anchored to `file:line` in the diff +2. **Summary comment** — aggregated table of all check results in an issue comment +3. **Check-run** — appears in the "Checks" tab (green ✅ or red ❌) +4. **PR label** — `sdk-review: ✅ passed` / `❌ blocked` / `⚠️ flagged` / `skipped` + +On re-run (push more commits, dispatch workflow again), all four artifacts are +**replaced** — you won't see duplicate comments accumulating. + +--- + +## Severities + +- **BLOCK** — must fix before merge. Check-run fails; branch protection refuses the merge. +- **FLAG** — should fix; does not block merge. Check-run passes with warning. +- **PASS** — no findings for this check. +- **SHADOW** — internal telemetry; not posted, used to evaluate new rules before promoting them. + +Some rules are **locked** (marked `BLOCK_LOCKED` in `.claude/config/rules.yaml`). +Locked rules cannot be suppressed via inline comments or downgraded via config — +they are safety-critical (secrets, license, SAP-internal URL leaks, breaking-change +declarations). + +--- + +## What the skill checks + +20 checks · ~150 rules. Configured in [`.claude/config/rules.yaml`](../.claude/config/rules.yaml). + +| Check | Purpose | +|-------|---------| +| **secrets** | AWS keys, JWT, GitHub PATs, private keys, plaintext credentials | +| **license-spdx** | SPDX headers on new source files (respects `REUSE.toml`) | +| **disclosure** | No SAP-internal URLs, ORD IDs, internal Jira in public artifacts | +| **hardcode** | No hardcoded URLs, credentials, magic timeouts | +| **telemetry** | `@record_metrics` decorator on public client methods + emission tests | +| **docs** | `user-guide.md` completeness including BTP dep + regional availability | +| **bdd** | Feature files exist + cross-language parity | +| **patterns** | Factory pattern, exception hierarchy, type hints, `py.typed` | +| **versioning** | SemVer bump matches diff scope; BREAKING family fires on API changes | +| **commits** | Conventional Commits | +| **errors-logging** | `raise X from e` chaining, no sensitive info in exception messages | +| **testing-depth** | Bug fixes have tests; new modules have integration tests | +| **http-hygiene** | Session reuse, configurable timeouts | +| **concurrency** | `asyncio.Queue` dedup, thread safety on shared state | +| **deps-supply** | Dep justification, lockfile drift, no internal artifactory | +| **deletion-hygiene** | Removed symbols have zero residual references | +| **constants** | Magic values → constants/enums | +| **binding-shape** | BTP binding parsing (no `url + "/oauth/token"` concat) | +| **quality-gate-parity** | CI runs the full dev gate (ruff + format + typecheck) | +| **pr-size** | Advisory on large PRs (recommends stacked-PR workflow) | + +--- + +## Documenting BTP dependencies (DC-11..DC-16) + +If your module imports `destination`, `Fragment*`, `Certificate*`, or has +region-specific constants, `user-guide.md` **must** contain the corresponding +section. The skill fires **BLOCK** if these are missing: + +- **DC-11** — `destination` import → `## Dependencies` section mentioning "Destination Service" +- **DC-12** — Fragment usage → same section + `create_fragment_client` example +- **DC-13** — Certificate usage → same section + `create_certificate_client` example +- **DC-14** — Region constants → `## Regional Availability` section listing supported/unsupported regions +- **DC-15** — Reads `VCAP_SERVICES` → `## Configuration` section with sample binding JSON +- **DC-16** — Cross-module SDK dep → note in `## Dependencies` + +### Example templates + +**DC-11 Destination Service required:** +```markdown +## Dependencies + +This module requires **SAP BTP Destination Service**: +- Service instance name: `default` (configurable via `create_client(instance="...")`) +- Required binding: `xsuaa` credentials + `destination` service credentials +- Mount path: `/etc/secrets/sapbtp/destination/<instance>/` +- Local dev: set `CLOUD_SDK_LOCALDEV_DESTINATION=true` to use mock backing +``` + +**DC-14 Regional Availability:** +```markdown +## Regional Availability + +Available in the following BTP regions: +- ✅ `eu10` (Frankfurt) +- ✅ `us10` (Ashburn) +- ✅ `ap11` (Singapore) +- ❌ `cn40` (Shanghai) — not supported due to <reason> +``` + +--- + +## Breaking changes (BREAKING-01..04) + +If your diff contains a **breaking change** (public API removal, method signature +change, dataclass field deletion, enum value removal, exception hierarchy change), +the skill requires: + +1. Commit message uses `feat!:` or `fix!:` prefix +2. PR body has a `## Breaking Changes` section with **non-empty** content +3. PR body ticks the "Breaking change" checkbox +4. `pyproject.toml` / `pom.xml` version is bumped MINOR (or MAJOR if pre-1.0) +5. Migration path documented in PR body or `RELEASE.md` + +All four must be true — half-declared breakages are BLOCKed. This is enforced by +`BREAKING-01` and `BREAKING-02`, both `BLOCK_LOCKED` (cannot be suppressed). + +--- + +## Incremental delivery for large contributions + +For features exceeding ~500 lines or touching multiple modules, prefer **stacked PRs**: + +1. Open a feature branch off `main`: `feat/<capability>` +2. Send small, isolated PRs (≤400 lines each) targeting **your feature branch** +3. Each sub-PR passes CI standalone +4. When feature is complete, open a final PR from your feature branch to `main` + +The skill flags `PR-SIZE-01..05` (currently SHADOW tier — logged only) when a PR +exceeds thresholds. See `CONTRIBUTING.md § Incremental Delivery`. + +--- + +## Suppressing false positives + +If a rule fires incorrectly on a specific line, add a comment: + +```python +# Python +timeout = 30 # sdk-review: ignore[hardcode] +``` + +```java +// Java +final int timeout = 30; // sdk-review: ignore[hardcode] +``` + +Or for an entire file (first 20 lines): +```python +# sdk-review-ignore-file: hardcode,patterns +``` + +**You cannot suppress:** +- Any `SEC-*` rule (secrets) +- `HC-03` (SAP-internal URL leak) +- `DIS-06` (internal artifactory `--index-url`) +- `LIC-01/02` (SPDX headers) +- `BND-02` (BTP token URL concat) +- `BND-05` (binding logs credentials) +- `BREAKING-*` family + +These are locked. Fix the finding or open a discussion with maintainers. + +--- + +## When cross-language BDD parity can't be verified + +The skill checks that new modules have BDD feature files in **both** SDKs (Python +and Java). If the sibling repo can't be reached (SSO required, network unavailable, +missing checkout), `check-bdd.sh` degrades to a FLAG "cross-language parity not +verified" instead of blocking. + +The alias map at `.claude/config/module-aliases.yaml` handles name divergences +(e.g., Python `dms` ↔ Java `documentmanagement`). + +--- + +## Re-running + +Just push a new commit — the Action reruns automatically and replaces prior +review artifacts. Or dispatch manually: + +```bash +gh workflow run sdk-module-review.yml -f pr_number=<N> +``` + +--- + +## Tuning noisy rules + +Maintainers can adjust rule severity or disable rules in +`.claude/config/rules.yaml`: + +```yaml +rules: + HC-04: + tier: OFF # disable + # or + HC-04: + tier: FLAG # downgrade from BLOCK to FLAG +``` + +Locked rules ignore these overrides. + +--- + +## What if something breaks? + +- **False positive that survived suppression** → open an issue with label `sdk-review-tuning` +- **Skill errors on your PR** → open an issue with label `sdk-review-bug` +- **Suggested new rule** → open an issue with label `sdk-review-enhancement` + +--- + +## Reference + +- Rule catalog: [`.claude/config/rules.yaml`](../.claude/config/rules.yaml) +- Module aliases: [`.claude/config/module-aliases.yaml`](../.claude/config/module-aliases.yaml) +- Baseline exemptions: [`.claude/config/baseline.json`](../.claude/config/baseline.json) +- Check scripts: [`.claude/scripts/check-*.sh`](../.claude/scripts/) +- Orchestrator: [`.claude/scripts/orchestrate.sh`](../.claude/scripts/orchestrate.sh) +- Workflow: [`.github/workflows/sdk-module-review.yml`](../.github/workflows/sdk-module-review.yml) diff --git a/src/sap_cloud_sdk/agentgateway/agw_client.py b/src/sap_cloud_sdk/agentgateway/agw_client.py index 33a0ccfc..4889a8b8 100644 --- a/src/sap_cloud_sdk/agentgateway/agw_client.py +++ b/src/sap_cloud_sdk/agentgateway/agw_client.py @@ -9,6 +9,7 @@ import asyncio import logging +from datetime import datetime, timezone from typing import Callable from sap_cloud_sdk.agentgateway.config import ClientConfig @@ -36,7 +37,17 @@ ) from sap_cloud_sdk.agentgateway._token_cache import _GatewayUrlCache, _TokenCache from sap_cloud_sdk.agentgateway.exceptions import AgentGatewaySDKError -from sap_cloud_sdk.core.telemetry import Module, Operation, record_metrics +import sap_cloud_sdk.core.auditlog_ng as auditlog_ng +from sap_cloud_sdk.core.auditlog_ng import AuditClient +from sap_cloud_sdk.core.telemetry import ( + Module, + Operation, + record_metrics, + get_tenant_id, +) +from sap_cloud_sdk.core.auditlog_ng.gen.sap.auditlog.auditevent.v2 import ( + auditevent_pb2 as pb, +) logger = logging.getLogger(__name__) @@ -120,6 +131,7 @@ def __init__( self._config = config or ClientConfig() self._token_cache = _TokenCache(self._config) self._gateway_url_cache = _GatewayUrlCache() + self._audit_client: AuditClient | None = self._create_audit_client() @staticmethod def _resolve_value( @@ -152,6 +164,49 @@ def _resolve_tenant_subdomain(self) -> str: "tenant_subdomain is required for LoB agent flow.", ) + def _create_audit_client(self) -> AuditClient | None: + """Create an audit client from the LoB destination. Returns None for customer agents or on failure.""" + tenant_subdomain = ( + self._tenant_subdomain + if isinstance(self._tenant_subdomain, str) + else self._tenant_subdomain() + if self._tenant_subdomain is not None + else None + ) + if not tenant_subdomain: + return None + try: + return auditlog_ng.create_client( + tenant=tenant_subdomain, + _telemetry_source=Module.AGENTGATEWAY, + ) + except Exception: + logger.debug("Failed to create audit client", exc_info=True) + return None + + def _send_audit_event( + self, + object_id: str, + user_id: str | None = None, + ) -> None: + """Send a DataAccess audit event. Errors are logged and suppressed.""" + tenant_id = get_tenant_id() + if self._audit_client is None or not tenant_id: + return + try: + event = pb.DataAccess() + event.common.timestamp.FromDatetime(datetime.now(timezone.utc)) + event.common.tenant_id = tenant_id + if user_id: + event.common.user_initiator_id = user_id + event.channel_type = "MCP" + event.channel_id = "agent-gateway" + event.object_type = "mcp-tool" + event.object_id = object_id + self._audit_client.send(event) + except Exception: + logger.debug("Failed to send audit event", exc_info=True) + @record_metrics(Module.AGENTGATEWAY, Operation.AGENTGATEWAY_GET_SYSTEM_AUTH) async def get_system_auth(self, app_tid: str | None = None) -> AuthResult: """Get system-scoped authentication (client_credentials flow). @@ -335,6 +390,7 @@ async def list_mcp_tools( self, user_token: str | Callable[[], str] | None = None, app_tid: str | None = None, + user_id: str | None = None, ) -> list[MCPTool]: """List all MCP tools from MCP servers. @@ -355,6 +411,8 @@ async def list_mcp_tools( If provided, uses user-scoped auth instead of system auth. app_tid: BTP Application Tenant ID of the subscriber. Only used for customer agents. + user_id: User identifier recorded in the audit event when an + audit_client is configured on the client. Returns: List of MCPTool objects from all MCP servers. @@ -384,9 +442,11 @@ async def list_mcp_tools( auth = await self.get_user_auth(user_token, app_tid) else: auth = await self.get_system_auth(app_tid=app_tid) - return await get_mcp_tools_customer( + tools = await get_mcp_tools_customer( credentials, auth.access_token, self._config.timeout ) + self._send_audit_event("*", user_id) + return tools # LoB flow - requires tenant_subdomain if app_tid: @@ -397,9 +457,11 @@ async def list_mcp_tools( auth = await self.get_user_auth(user_token) else: auth = await self.get_system_auth() - return await get_mcp_tools_lob( + tools = await get_mcp_tools_lob( tenant, auth.access_token, self._config.timeout ) + self._send_audit_event("*", user_id) + return tools except AgentGatewaySDKError: # Re-raise SDK errors as-is @@ -481,6 +543,7 @@ async def call_mcp_tool( tool: MCPTool, user_token: str | Callable[[], str] | None = None, app_tid: str | None = None, + user_id: str | None = None, **kwargs, ) -> str: """Invoke an MCP tool. @@ -505,6 +568,8 @@ async def call_mcp_tool( for tenant-scoped token exchange. TODO: This parameter's requirement is still being clarified with the IBD team and may be removed if unnecessary. + user_id: User identifier recorded in the audit event when an + audit_client is configured on the client. **kwargs: Tool input parameters (passed directly to the tool). Returns: @@ -546,18 +611,22 @@ async def call_mcp_tool( ) auth = await self.get_system_auth(app_tid) - return await call_mcp_tool_customer( + result = await call_mcp_tool_customer( tool, auth.access_token, self._config.timeout, **kwargs ) + self._send_audit_event(tool.name, user_id) + return result # LoB flow - requires user_token and tenant_subdomain if app_tid: logger.warning("app_tid parameter ignored for LoB agent flow") auth = await self.get_user_auth(user_token, app_tid) - return await call_mcp_tool_lob( + result = await call_mcp_tool_lob( tool, auth.access_token, self._config.timeout, **kwargs ) + self._send_audit_event(tool.name, user_id) + return result except AgentGatewaySDKError: # Re-raise SDK errors as-is @@ -644,4 +713,7 @@ def create_client( user_auth = await agw_client.get_user_auth(user_token="user-jwt") ``` """ - return AgentGatewayClient(tenant_subdomain=tenant_subdomain, config=config) + return AgentGatewayClient( + tenant_subdomain=tenant_subdomain, + config=config, + )