Skip to content

Commit 32daaf1

Browse files
committed
feat(sdk-review): skill — 20 checks + orchestrate + GH Action + FP remediation
Consolidated skill delivery after Bohn review. All prior batch-level commits collapsed into one clean commit that ships on top of the foundation. Includes: - 20 deterministic check scripts (safety, structural, depth, meta) - orchestrate.sh + aggregate.sh with parallel check execution - .github/workflows/sdk-module-review.yml (auto-trigger on PR events) - docs/PR-REVIEW.md + CONTRIBUTING § Incremental Delivery - All 10 FP-remediation fixes (A-01 through J-01, plus G-01 peer-consistency) - Bats regression test suite covering every FP fix (76 tests, all green) Bohn review items addressed: - Bug 2 (baseline path): fixed in foundation (rules.yaml) - Bug 3 (rule IDs): fixed in foundation (baseline.json) - Bug 4 (marker prefix): documented as intentional - Bug 5 (hunk-filter → foundation): moved - Bug 6 (at_commit): resolved to origin/main SHA - Bug 7 (naming): ast_python_checks unified - Bug 8 (tests location): moved to .claude/tests/ Deferred: - Bug 1 (rebase on main): awaits explicit maintainer window (pom.xml version conflict resolution requires care).
1 parent f427f66 commit 32daaf1

27 files changed

Lines changed: 2410 additions & 0 deletions
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# /review-new-module — orchestrator slash command
2+
Run the SDK Module Review skill against a PR.
3+
4+
## Usage
5+
- `/review-new-module <PR_NUMBER>` — full review, posts findings
6+
- `/review-new-module <PR_NUMBER> --dry-run` — analysis only, no posting
7+
8+
## What it does
9+
1. Detects language (Python vs Java) via `pyproject.toml` / `pom.xml`
10+
2. Fetches PR diff + body
11+
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)
12+
4. Applies baseline exemptions + scope predicates + tier gating
13+
5. Detects breaking changes via AST diff
14+
6. Posts 4 signals: inline comments, summary comment, check-run, label
15+
16+
## Implementation
17+
Run: `bash .claude/scripts/orchestrate.sh <PR_NUMBER>`
18+
19+
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/scripts/aggregate.sh

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
#!/usr/bin/env bash
2+
# aggregate.sh — merge N check reports into a single summary, applying rule tiers.
3+
set -euo pipefail
4+
5+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
6+
TMPDIR_RUN="$1"
7+
RULES_YAML="${RULES_YAML:-$SCRIPT_DIR/../config/rules.yaml}"
8+
9+
# get_tier <rule> — reads rules.yaml, returns tier or empty
10+
get_tier() {
11+
local rule="$1"
12+
# rules.yaml format: " RULE-ID: { tier: X, ... }"
13+
awk -v rule="$rule" '
14+
match($0, "^ " rule ":") {
15+
if (match($0, /tier:[[:space:]]*[A-Z_]+/)) {
16+
t = substr($0, RSTART, RLENGTH)
17+
sub(/^tier:[[:space:]]*/, "", t)
18+
print t
19+
exit
20+
}
21+
}
22+
' "$RULES_YAML" 2>/dev/null
23+
}
24+
25+
# Collect all report-*.json files. Skip files that are not valid JSON
26+
# (e.g. a check that timed out or crashed producing partial stdout) so a
27+
# single bad report can't take the whole aggregation down.
28+
raw_reports=$(ls "$TMPDIR_RUN"/report-*.json 2>/dev/null | sort)
29+
reports=""
30+
for r in $raw_reports; do
31+
if jq -e '.' "$r" >/dev/null 2>&1; then
32+
reports="$reports $r"
33+
else
34+
echo "WARN: skipping invalid JSON report: $r" >&2
35+
fi
36+
done
37+
if [ -z "$reports" ]; then
38+
echo '{"findings":[],"shadow_findings":[],"summary":{"block_count":0,"flag_count":0,"shadow_count":0,"locked_count":0},"per_check_summary":{}}'
39+
exit 0
40+
fi
41+
42+
# Merge raw findings, then re-classify each finding by tier from rules.yaml
43+
merged=$(mktemp)
44+
# shellcheck disable=SC2086
45+
jq -s '.' $reports > "$merged"
46+
47+
# For each finding, look up its rule tier and adjust
48+
retagged_findings="[]"
49+
retagged_shadow="[]"
50+
locked_count=0
51+
52+
n=$(jq '[.[] | .findings // [] | length] | add // 0' "$merged")
53+
if [ "$n" -gt 0 ]; then
54+
all_findings=$(jq '[.[] | .findings // []] | flatten' "$merged")
55+
rule_ids=$(echo "$all_findings" | jq -r '.[] | .rule' | sort -u)
56+
57+
# Build lookup: rule -> tier
58+
tier_map='{}'
59+
while IFS= read -r rule; do
60+
[ -z "$rule" ] && continue
61+
tier=$(get_tier "$rule")
62+
[ -z "$tier" ] && tier="FLAG" # default
63+
tier_map=$(echo "$tier_map" | jq --arg r "$rule" --arg t "$tier" '. + {($r): $t}')
64+
done <<< "$rule_ids"
65+
66+
# Split findings into posted vs shadow based on tier
67+
retagged_findings=$(echo "$all_findings" | jq --argjson tiers "$tier_map" '
68+
map(
69+
. as $f
70+
| ($tiers[$f.rule] // "FLAG") as $tier
71+
| if $tier == "SHADOW" then empty
72+
elif $tier == "BLOCK_LOCKED" then . + {severity: "BLOCK", tier: "BLOCK_LOCKED", locked: true}
73+
elif $tier == "BLOCK" then . + {severity: "BLOCK", tier: "BLOCK"}
74+
elif $tier == "FLAG" then . + {severity: "FLAG", tier: "FLAG"}
75+
else . + {tier: $tier} end
76+
)
77+
')
78+
retagged_shadow=$(echo "$all_findings" | jq --argjson tiers "$tier_map" '
79+
map(
80+
. as $f
81+
| ($tiers[$f.rule] // "FLAG") as $tier
82+
| if $tier == "SHADOW" then . + {tier: "SHADOW"} else empty end
83+
)
84+
')
85+
locked_count=$(echo "$retagged_findings" | jq '[.[] | select(.locked == true)] | length')
86+
fi
87+
88+
block_count=$(echo "$retagged_findings" | jq '[.[] | select(.severity == "BLOCK")] | length')
89+
flag_count=$(echo "$retagged_findings" | jq '[.[] | select(.severity == "FLAG")] | length')
90+
shadow_count=$(echo "$retagged_shadow" | jq 'length')
91+
per_check=$(jq '[.[] | {(.check): {status: .status, count: ((.findings // []) | length)}}] | add' "$merged")
92+
93+
jq -n \
94+
--argjson findings "$retagged_findings" \
95+
--argjson shadow "$retagged_shadow" \
96+
--argjson block "$block_count" \
97+
--argjson flag "$flag_count" \
98+
--argjson shadow_c "$shadow_count" \
99+
--argjson locked "$locked_count" \
100+
--argjson per_check "$per_check" \
101+
'{
102+
version: "1.0.0",
103+
findings: $findings,
104+
shadow_findings: $shadow,
105+
summary: {
106+
block_count: $block,
107+
flag_count: $flag,
108+
shadow_count: $shadow_c,
109+
locked_count: $locked
110+
},
111+
per_check_summary: $per_check
112+
}'
113+
114+
rm -f "$merged"

.claude/scripts/check-bdd.sh

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
#!/usr/bin/env bash
2+
# check-bdd.sh — feature file existence + cross-language parity via alias map.
3+
set -euo pipefail
4+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
5+
source "$SCRIPT_DIR/lib/json-emit.sh"
6+
7+
LANGUAGE="${LANGUAGE:-python}"
8+
REPO_ROOT="${REPO_ROOT:-.}"
9+
DIFF_FILE="${DIFF_FILE:-/dev/stdin}"
10+
SDK_SIBLING_PATH="${SDK_SIBLING_PATH:-}"
11+
CONFIG_DIR="${CONFIG_DIR:-$REPO_ROOT/.claude/config}"
12+
13+
STARTED=$(now_iso)
14+
findings=$(mktemp); trap 'rm -f "$findings"' EXIT
15+
16+
diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "")
17+
18+
# Extract new/modified modules from diff
19+
if [ "$LANGUAGE" = "python" ]; then
20+
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)
21+
else
22+
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)
23+
fi
24+
25+
# resolve alias mapping (python name → java name and vice versa)
26+
# The YAML is a simple list of pairs:
27+
# aliases:
28+
# - python: dms
29+
# java: documentmanagement
30+
# awk on `$NF` (last field) extracts the value cleanly even when the key
31+
# column varies. We only pair a python: line with the very next java: line.
32+
resolve_sibling_name() {
33+
local mod="$1" dir="$LANGUAGE"
34+
local aliases="$CONFIG_DIR/module-aliases.yaml"
35+
if [ ! -f "$aliases" ]; then echo "$mod"; return; fi
36+
if [ "$dir" = "python" ]; then
37+
awk -v mod="$mod" '
38+
/^[[:space:]]*-?[[:space:]]*python:[[:space:]]*/ {
39+
sub(/^[^:]*:[[:space:]]*/, ""); p_name=$0; next
40+
}
41+
/^[[:space:]]*java:[[:space:]]*/ {
42+
sub(/^[^:]*:[[:space:]]*/, ""); j_name=$0
43+
if (p_name == mod) { print j_name; exit }
44+
p_name=""
45+
}
46+
' "$aliases"
47+
return
48+
else
49+
awk -v mod="$mod" '
50+
/^[[:space:]]*-?[[:space:]]*python:[[:space:]]*/ {
51+
sub(/^[^:]*:[[:space:]]*/, ""); p_name=$0; next
52+
}
53+
/^[[:space:]]*java:[[:space:]]*/ {
54+
sub(/^[^:]*:[[:space:]]*/, ""); j_name=$0
55+
if (j_name == mod) { print p_name; exit }
56+
p_name=""
57+
}
58+
' "$aliases"
59+
return
60+
fi
61+
}
62+
63+
while IFS= read -r mod; do
64+
[ -z "$mod" ] && continue
65+
66+
# Path for THIS repo's feature file
67+
# FP-C-01: BDD-01 must accept ANY .feature file under the module's integration
68+
# dir, not just `<module>.feature`.
69+
if [ "$LANGUAGE" = "python" ]; then
70+
feature_dir="$REPO_ROOT/tests/$mod/integration"
71+
feature_path="$feature_dir/$mod.feature" # kept for BDD-02 sibling comparison
72+
mod_dir="$REPO_ROOT/src/sap_cloud_sdk/$mod"
73+
else
74+
feature_dir="$REPO_ROOT/src/test/resources/com/sap/applicationfoundation/$mod/integration"
75+
feature_path="$feature_dir/$mod.feature" # kept for BDD-02 sibling comparison
76+
mod_dir="$REPO_ROOT/src/main/java/com/sap/cloud/sdk/$mod"
77+
fi
78+
79+
# BDD-01: at least one .feature file exists in the module's integration dir
80+
# (only fire if module has any source files and PR creates the module).
81+
has_any_feature=false
82+
if [ -d "$feature_dir" ]; then
83+
if compgen -G "$feature_dir/*.feature" > /dev/null 2>&1; then
84+
has_any_feature=true
85+
fi
86+
fi
87+
88+
if [ "$has_any_feature" = "false" ] && [ -d "$mod_dir" ]; then
89+
# Detect if this PR creates any new file INSIDE the module.
90+
# `new file mode` is on its own line before the `+++ b/<path>` header,
91+
# so we scan block-by-block to link them.
92+
is_new_module=$(echo "$diff_content" | awk -v mod="$mod" -v lang="$LANGUAGE" '
93+
BEGIN {
94+
if (lang == "python") pat = "src/sap_cloud_sdk/" mod "/"
95+
else pat = "src/main/java/com/sap/cloud/sdk/" mod "/"
96+
}
97+
/^diff --git/ { is_new = 0; next }
98+
/^new file mode/ { is_new = 1; next }
99+
/^\+\+\+ b\// {
100+
if (is_new && index($0, pat) > 0) { print "true"; exit }
101+
}
102+
')
103+
if [ "$is_new_module" = "true" ]; then
104+
emit_finding "BDD-01" "BLOCK" "tests/$mod/integration/" 1 \
105+
"New module '$mod' has no .feature files under integration/" \
106+
"Create at least one $feature_dir/*.feature with cross-language-consistent scenarios" >> "$findings"
107+
fi
108+
fi
109+
110+
# BDD-02: sibling repo parity
111+
sibling_name=$(resolve_sibling_name "$mod")
112+
[ -z "$sibling_name" ] && sibling_name="$mod"
113+
114+
if [ -n "$SDK_SIBLING_PATH" ] && [ -d "$SDK_SIBLING_PATH" ]; then
115+
if [ "$LANGUAGE" = "python" ]; then
116+
# Python repo — sibling is Java
117+
sibling_feature="$SDK_SIBLING_PATH/src/test/resources/com/sap/applicationfoundation/$sibling_name/integration/$sibling_name.feature"
118+
sibling_module_dir="$SDK_SIBLING_PATH/src/main/java/com/sap/cloud/sdk/$sibling_name"
119+
else
120+
sibling_feature="$SDK_SIBLING_PATH/tests/$sibling_name/integration/$sibling_name.feature"
121+
sibling_module_dir="$SDK_SIBLING_PATH/src/sap_cloud_sdk/$sibling_name"
122+
fi
123+
# If the sibling module dir exists, and sibling has no feature but we do (or vice versa)
124+
if [ -d "$sibling_module_dir" ] && [ ! -f "$sibling_feature" ] && [ -f "$feature_path" ]; then
125+
emit_finding "BDD-02" "FLAG" "$feature_path" 1 \
126+
"Sibling SDK ($sibling_name) has module but no BDD feature — parity broken" "" >> "$findings"
127+
fi
128+
if [ -d "$sibling_module_dir" ] && [ -f "$sibling_feature" ] && [ ! -f "$feature_path" ] && [ -d "$mod_dir" ]; then
129+
emit_finding "BDD-02" "BLOCK" "tests/.../$mod.feature" 1 \
130+
"Module '$mod' exists in sibling SDK ($sibling_name) with BDD feature — this repo must have equivalent" "" >> "$findings"
131+
fi
132+
fi
133+
done <<< "$modules"
134+
135+
status=$(status_from_findings < "$findings")
136+
emit_report "bdd" "$LANGUAGE" "$status" "$STARTED" < "$findings"
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
#!/usr/bin/env bash
2+
# check-binding-shape.sh — placeholder: stub check emitting empty findings.
3+
# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.binding-shape
4+
set -euo pipefail
5+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
6+
source "$SCRIPT_DIR/lib/json-emit.sh"
7+
source "$SCRIPT_DIR/lib/skill-self-skip.sh"
8+
9+
LANGUAGE="${LANGUAGE:-python}"
10+
DIFF_FILE="${DIFF_FILE:-/dev/stdin}"
11+
REPO_ROOT="${REPO_ROOT:-.}"
12+
13+
STARTED=$(now_iso)
14+
findings=$(mktemp); trap 'rm -f "$findings"' EXIT
15+
diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "")
16+
17+
# BND-02 (LOCKED): token URL via string concat + "/oauth/token"
18+
echo "$diff_content" | awk '
19+
BEGIN { file=""; line=0 }
20+
/^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next }
21+
/^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next }
22+
/^\+/ && !/^\+\+\+/ { print file "\t" line "\t" substr($0, 2); line++; next }
23+
/^ / { line++; next }
24+
' | while IFS=$'\t' read -r file line_num content; do
25+
[ -z "$file" ] && continue
26+
# Self-review protection: skip skill files
27+
if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi
28+
if echo "$content" | grep -qE 'rstrip\("/"\)[[:space:]]*\+[[:space:]]*"/oauth/token"|\.replaceAll\("/\+\$", ""\)[[:space:]]*\+[[:space:]]*"/oauth/token"'; then
29+
emit_finding "BND-02" "BLOCK" "$file" "$line_num" \
30+
"BTP token URL built via string concat — different services expose different fields" \
31+
"Use HttpUrl.parse().newBuilder() or honour a 'token_url' field if present" >> "$findings"
32+
fi
33+
if echo "$content" | grep -qE '\+[[:space:]]*"/oauth/token"'; then
34+
emit_finding "BND-02" "BLOCK" "$file" "$line_num" \
35+
"Hardcoded /oauth/token path — BTP services vary" "" >> "$findings"
36+
fi
37+
done
38+
39+
status=$(status_from_findings < "$findings")
40+
emit_report "binding-shape" "$LANGUAGE" "$status" "$STARTED" < "$findings"

.claude/scripts/check-commits.sh

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
#!/usr/bin/env bash
2+
# check-commits.sh — Conventional Commits enforcement.
3+
set -euo pipefail
4+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
5+
source "$SCRIPT_DIR/lib/json-emit.sh"
6+
7+
LANGUAGE="${LANGUAGE:-python}"
8+
# Resolve BASE_SHA from GITHUB_BASE_REF merge-base when not explicitly set —
9+
# HEAD~10 is a poor fallback (only reachable when the PR has ≥10 commits).
10+
if [ -z "${BASE_SHA:-}" ]; then
11+
base_ref="${GITHUB_BASE_REF:-main}"
12+
# Try origin/<ref>, then <ref>, then finally HEAD~10 as last resort
13+
if git rev-parse --verify "origin/${base_ref}" >/dev/null 2>&1; then
14+
BASE_SHA=$(git merge-base HEAD "origin/${base_ref}" 2>/dev/null || echo "HEAD~10")
15+
elif git rev-parse --verify "$base_ref" >/dev/null 2>&1; then
16+
BASE_SHA=$(git merge-base HEAD "$base_ref" 2>/dev/null || echo "HEAD~10")
17+
else
18+
BASE_SHA="HEAD~10"
19+
fi
20+
fi
21+
HEAD_SHA="${HEAD_SHA:-HEAD}"
22+
23+
STARTED=$(now_iso)
24+
findings=$(mktemp); trap 'rm -f "$findings"' EXIT
25+
26+
# List commit subjects
27+
commits=$(git log "${BASE_SHA}..${HEAD_SHA}" --format='%H %s' 2>/dev/null || echo "")
28+
prefix_regex='^(feat|fix|refactor|chore|docs|test|ci|build|perf|style|revert)(\([a-z0-9_/-]+\))?!?:[[:space:]]+.+'
29+
30+
echo "$commits" | while IFS= read -r line; do
31+
[ -z "$line" ] && continue
32+
sha=$(echo "$line" | cut -d' ' -f1)
33+
subject=$(echo "$line" | cut -d' ' -f2-)
34+
# Skip merge commits (identified by 2+ parents, robust regardless of message shape)
35+
parents=$(git rev-list --parents -n 1 "$sha" 2>/dev/null | awk '{print NF-1}')
36+
if [ "${parents:-0}" -gt 1 ]; then continue; fi
37+
# Also skip legacy "Merge branch/pull/etc" defaults
38+
if echo "$subject" | grep -qiE '^Merge '; then continue; fi
39+
if ! echo "$subject" | grep -qE "$prefix_regex"; then
40+
emit_finding "COM-01" "BLOCK" "COMMIT:$sha" 1 \
41+
"Commit '$subject' does not follow Conventional Commits" \
42+
"Prefix with feat:/fix:/chore:/docs:/test:/refactor:/ci:/build:/perf:/style:/revert:" >> "$findings"
43+
fi
44+
done
45+
46+
status=$(status_from_findings < "$findings")
47+
emit_report "commits" "$LANGUAGE" "$status" "$STARTED" < "$findings"
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
#!/usr/bin/env bash
2+
# check-concurrency.sh — placeholder: stub check emitting empty findings.
3+
# TODO: implement rules from 01-PYTHON.md / 02-JAVA.md §3.concurrency
4+
set -euo pipefail
5+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
6+
source "$SCRIPT_DIR/lib/json-emit.sh"
7+
source "$SCRIPT_DIR/lib/hunk-filter.sh"
8+
source "$SCRIPT_DIR/lib/skill-self-skip.sh"
9+
10+
LANGUAGE="${LANGUAGE:-python}"
11+
DIFF_FILE="${DIFF_FILE:-/dev/stdin}"
12+
REPO_ROOT="${REPO_ROOT:-.}"
13+
14+
STARTED=$(now_iso)
15+
findings=$(mktemp); trap 'rm -f "$findings"' EXIT
16+
diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "")
17+
18+
# CC-01: asyncio.Queue with no accompanying Lock/set — flag when queue+append pattern present
19+
echo "$diff_content" | awk '
20+
BEGIN { file=""; line=0 }
21+
/^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next }
22+
/^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next }
23+
/^\+/ && !/^\+\+\+/ { print file "\t" line "\t" substr($0, 2); line++; next }
24+
/^ / { line++; next }
25+
' | while IFS=$'\t' read -r file line_num content; do
26+
[ -z "$file" ] && continue
27+
if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi
28+
if echo "$content" | grep -qE 'asyncio\.Queue\('; then
29+
# Check if same file has a Lock or set() nearby
30+
if [ -f "$REPO_ROOT/$file" ] && ! grep -qE 'asyncio\.Lock|set\(\)' "$REPO_ROOT/$file" 2>/dev/null; then
31+
emit_finding_if_touched "CC-01" "FLAG" "$file" "$line_num" \
32+
"asyncio.Queue without dedup — if items must be unique, add a set() + asyncio.Lock" "" >> "$findings"
33+
fi
34+
fi
35+
done
36+
37+
status=$(status_from_findings < "$findings")
38+
emit_report "concurrency" "$LANGUAGE" "$status" "$STARTED" < "$findings"

0 commit comments

Comments
 (0)