feat(sdk-review): skill — 20 checks + orchestrate + GH Action + FP remediation

.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 <PR_NUMBER>` — full review, posts findings
+- `/review-new-module <PR_NUMBER> --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 <PR_NUMBER>`
+
+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).

.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 }

.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 <rule> — 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"

.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 `<module>.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/<path>` 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"

.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: <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"

.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"