Skip to content

Commit d3570cb

Browse files
committed
feat(sdk-review): batch 6/6 — wire-up + docs + CONTRIBUTING amendment
Final batch that turns the collection of checks into a working skill: ## Orchestration (.claude/scripts/) - orchestrate.sh — main entry point. Runs preflight, dispatches all 20 checks in parallel, applies baseline + tier gating, posts 4 signals (inline comments, summary, check-run, label) unless --dry-run. - aggregate.sh — merges N check reports into a single summary; applies rules.yaml tiers to each finding. ## Slash command (.claude/commands/) - review-new-module.md — Claude Code slash command that invokes orchestrate.sh ## GitHub Actions (.github/workflows/) - sdk-module-review.yml — auto-trigger on every PR (opened/synchronize/reopened/ready_for_review) + workflow_dispatch - sdk-skill-isolation-check.yml — validates sub-PRs into feat/sdk-review-skill (shellcheck, ruff, bats, size budget, fixtures per check) ## Docs - docs/PR-REVIEW.md — user-facing guide (rules, severities, suppression, BTP dep documentation, breaking changes, tuning noisy rules) - docs/BRANCH-PROTECTION-SETUP.md — admin guide (UI + gh CLI) to configure sdk-module-review as required status check + SHADOW→FLAG→BLOCK rollout ## CONTRIBUTING.md - New section: 'AI-assisted contribution skills — required for review' - States clearly that reviewers will NOT review PRs where sdk-module-review is failing/missing - Documents /review-new-module local + CI auto-trigger ## After merging this batch The skill is fully operational. See docs/BRANCH-PROTECTION-SETUP.md to configure the check-run as a required status check. Part 6 of 6. Ref: AFSDK-3937
1 parent 8eb9e39 commit d3570cb

8 files changed

Lines changed: 884 additions & 0 deletions

File tree

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: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
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
26+
reports=$(ls "$TMPDIR_RUN"/report-*.json 2>/dev/null | sort)
27+
if [ -z "$reports" ]; then
28+
echo '{"findings":[],"shadow_findings":[],"summary":{"block_count":0,"flag_count":0,"shadow_count":0,"locked_count":0},"per_check_summary":{}}'
29+
exit 0
30+
fi
31+
32+
# Merge raw findings, then re-classify each finding by tier from rules.yaml
33+
merged=$(mktemp)
34+
# shellcheck disable=SC2086
35+
jq -s '.' $reports > "$merged"
36+
37+
# For each finding, look up its rule tier and adjust
38+
retagged_findings="[]"
39+
retagged_shadow="[]"
40+
locked_count=0
41+
42+
n=$(jq '[.[] | .findings // [] | length] | add // 0' "$merged")
43+
if [ "$n" -gt 0 ]; then
44+
all_findings=$(jq '[.[] | .findings // []] | flatten' "$merged")
45+
rule_ids=$(echo "$all_findings" | jq -r '.[] | .rule' | sort -u)
46+
47+
# Build lookup: rule -> tier
48+
tier_map='{}'
49+
while IFS= read -r rule; do
50+
[ -z "$rule" ] && continue
51+
tier=$(get_tier "$rule")
52+
[ -z "$tier" ] && tier="FLAG" # default
53+
tier_map=$(echo "$tier_map" | jq --arg r "$rule" --arg t "$tier" '. + {($r): $t}')
54+
done <<< "$rule_ids"
55+
56+
# Split findings into posted vs shadow based on tier
57+
retagged_findings=$(echo "$all_findings" | jq --argjson tiers "$tier_map" '
58+
map(
59+
. as $f
60+
| ($tiers[$f.rule] // "FLAG") as $tier
61+
| if $tier == "SHADOW" then empty
62+
elif $tier == "BLOCK_LOCKED" then . + {severity: "BLOCK", tier: "BLOCK_LOCKED", locked: true}
63+
elif $tier == "BLOCK" then . + {severity: "BLOCK", tier: "BLOCK"}
64+
elif $tier == "FLAG" then . + {severity: "FLAG", tier: "FLAG"}
65+
else . + {tier: $tier} end
66+
)
67+
')
68+
retagged_shadow=$(echo "$all_findings" | jq --argjson tiers "$tier_map" '
69+
map(
70+
. as $f
71+
| ($tiers[$f.rule] // "FLAG") as $tier
72+
| if $tier == "SHADOW" then . + {tier: "SHADOW"} else empty end
73+
)
74+
')
75+
locked_count=$(echo "$retagged_findings" | jq '[.[] | select(.locked == true)] | length')
76+
fi
77+
78+
block_count=$(echo "$retagged_findings" | jq '[.[] | select(.severity == "BLOCK")] | length')
79+
flag_count=$(echo "$retagged_findings" | jq '[.[] | select(.severity == "FLAG")] | length')
80+
shadow_count=$(echo "$retagged_shadow" | jq 'length')
81+
per_check=$(jq '[.[] | {(.check): {status: .status, count: ((.findings // []) | length)}}] | add' "$merged")
82+
83+
jq -n \
84+
--argjson findings "$retagged_findings" \
85+
--argjson shadow "$retagged_shadow" \
86+
--argjson block "$block_count" \
87+
--argjson flag "$flag_count" \
88+
--argjson shadow_c "$shadow_count" \
89+
--argjson locked "$locked_count" \
90+
--argjson per_check "$per_check" \
91+
'{
92+
version: "1.0.0",
93+
findings: $findings,
94+
shadow_findings: $shadow,
95+
summary: {
96+
block_count: $block,
97+
flag_count: $flag,
98+
shadow_count: $shadow_c,
99+
locked_count: $locked
100+
},
101+
per_check_summary: $per_check
102+
}'
103+
104+
rm -f "$merged"

.claude/scripts/orchestrate.sh

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
#!/usr/bin/env bash
2+
# orchestrate.sh — main entry point. Runs all 20 checks and posts results.
3+
set -euo pipefail
4+
5+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
6+
LIB="$SCRIPT_DIR/lib"
7+
8+
# shellcheck source=lib/json-emit.sh
9+
source "$LIB/json-emit.sh"
10+
# shellcheck source=lib/github-api.sh
11+
source "$LIB/github-api.sh"
12+
13+
PR_NUMBER="${1:-}"
14+
DRY_RUN="${DRY_RUN:-false}"
15+
if [ "${2:-}" = "--dry-run" ]; then DRY_RUN=true; fi
16+
17+
if [ -z "$PR_NUMBER" ]; then
18+
echo "Usage: orchestrate.sh <PR_NUMBER> [--dry-run]" >&2
19+
exit 2
20+
fi
21+
22+
REPO_ROOT="${REPO_ROOT:-$(pwd)}"
23+
export REPO_ROOT
24+
CONFIG_DIR="$REPO_ROOT/.claude/config"
25+
export CONFIG_DIR
26+
if [ -n "${TMPDIR_RUN:-}" ]; then
27+
mkdir -p "$TMPDIR_RUN"
28+
else
29+
TMPDIR_RUN="$(mktemp -d)"
30+
fi
31+
trap '[ -n "${KEEP_TMP:-}" ] || rm -rf "$TMPDIR_RUN"' EXIT
32+
33+
echo "▶ SDK Module Review — PR #$PR_NUMBER (dry-run=$DRY_RUN)"
34+
echo " Working dir: $TMPDIR_RUN"
35+
36+
# 1. Preflight — detect language + hostname + fetch PR data
37+
LANGUAGE=$("$LIB/detect-language.sh" "$REPO_ROOT")
38+
export LANGUAGE
39+
echo " Language: $LANGUAGE"
40+
41+
if [ "$DRY_RUN" != "true" ]; then
42+
HOSTNAME=$(detect_hostname)
43+
check_gh_auth "$HOSTNAME"
44+
fi
45+
46+
# 2. Fetch diff + PR metadata
47+
if [ -f "${DIFF_FILE:-}" ]; then
48+
cp "$DIFF_FILE" "$TMPDIR_RUN/pr.diff"
49+
elif [ "$DRY_RUN" = "true" ] && [ -n "${LOCAL_DIFF:-}" ]; then
50+
cp "$LOCAL_DIFF" "$TMPDIR_RUN/pr.diff"
51+
else
52+
gh pr diff "$PR_NUMBER" > "$TMPDIR_RUN/pr.diff"
53+
fi
54+
55+
if [ "$DRY_RUN" != "true" ]; then
56+
gh pr view "$PR_NUMBER" --json body -q .body > "$TMPDIR_RUN/pr-body.txt" 2>/dev/null || echo "" > "$TMPDIR_RUN/pr-body.txt"
57+
HEAD_SHA=$(get_pr_head_sha "$PR_NUMBER")
58+
elif [ -n "${LOCAL_PR_BODY:-}" ] && [ -f "$LOCAL_PR_BODY" ]; then
59+
cp "$LOCAL_PR_BODY" "$TMPDIR_RUN/pr-body.txt"
60+
HEAD_SHA="${HEAD_SHA:-HEAD}"
61+
else
62+
echo "" > "$TMPDIR_RUN/pr-body.txt"
63+
HEAD_SHA="${HEAD_SHA:-HEAD}"
64+
fi
65+
66+
export DIFF_FILE="$TMPDIR_RUN/pr.diff"
67+
export PR_BODY_FILE="$TMPDIR_RUN/pr-body.txt"
68+
export HEAD_SHA
69+
export BASE_SHA="${BASE_SHA:-$(git merge-base origin/main HEAD 2>/dev/null || echo HEAD~10)}"
70+
export BREAKING_JSON="$TMPDIR_RUN/breaking.json"
71+
72+
# 3. Compute added-lines set (used for hunk attribution)
73+
"$LIB/diff-added-lines.sh" < "$DIFF_FILE" > "$TMPDIR_RUN/added-lines.txt"
74+
export ADDED_LINES_FILE="$TMPDIR_RUN/added-lines.txt"
75+
76+
# 4. Run breaking-change detector
77+
python3 "$LIB/breaking-detector.py" "$BASE_SHA" "$HEAD_SHA" > "$BREAKING_JSON" 2>/dev/null || echo '{"breaking_detected":false,"kinds":[],"details":[]}' > "$BREAKING_JSON"
78+
79+
# 5. Detect disclosure profile from remote
80+
if [ "$DRY_RUN" != "true" ]; then
81+
remote_url=$(git remote get-url origin 2>/dev/null || echo "")
82+
if [[ "$remote_url" == *"github.tools.sap"* ]]; then
83+
export DISCLOSURE_PROFILE="internal"
84+
else
85+
export DISCLOSURE_PROFILE="public"
86+
fi
87+
else
88+
export DISCLOSURE_PROFILE="${DISCLOSURE_PROFILE:-public}"
89+
fi
90+
91+
# 6. Run all 20 checks in parallel
92+
checks=(secrets license-spdx disclosure hardcode telemetry
93+
docs bdd patterns versioning commits
94+
errors-logging testing-depth http-hygiene concurrency
95+
deps-supply deletion-hygiene constants binding-shape
96+
quality-gate-parity pr-size)
97+
98+
for check in "${checks[@]}"; do
99+
script="$SCRIPT_DIR/check-${check}.sh"
100+
if [ ! -x "$script" ]; then continue; fi
101+
DIFF_FILE="$DIFF_FILE" PR_BODY_FILE="$PR_BODY_FILE" \
102+
"$script" < "$DIFF_FILE" > "$TMPDIR_RUN/report-${check}.json" 2> "$TMPDIR_RUN/${check}.err" &
103+
done
104+
wait
105+
106+
# 6.5. Collect suppression tuples from files touched by the diff, then filter each report
107+
touched_files=$(grep -oE '^\+\+\+ b/[^[:space:]]+' "$DIFF_FILE" 2>/dev/null | sed 's|^+++ b/||' | sort -u || true)
108+
supp_file="$TMPDIR_RUN/suppressions.txt"
109+
: > "$supp_file"
110+
if [ -n "$touched_files" ]; then
111+
while IFS= read -r f; do
112+
[ -z "$f" ] && continue
113+
[ -f "$REPO_ROOT/$f" ] || continue
114+
bash "$LIB/suppression.sh" parse_line "$REPO_ROOT/$f" 2>/dev/null | sed "s|^$REPO_ROOT/|| ; s|^\./||" >> "$supp_file" || true
115+
bash "$LIB/suppression.sh" parse_file "$REPO_ROOT/$f" 2>/dev/null | sed "s|^$REPO_ROOT/|| ; s|^\./||" >> "$supp_file" || true
116+
done <<< "$touched_files"
117+
fi
118+
119+
for check in "${checks[@]}"; do
120+
report="$TMPDIR_RUN/report-${check}.json"
121+
[ -f "$report" ] || continue
122+
filtered=$(bash "$LIB/apply-suppression.sh" apply "$report" "$supp_file" 2>/dev/null || cat "$report")
123+
echo "$filtered" > "$report"
124+
done
125+
126+
# 7. Aggregate reports (applies tier gating per rules.yaml)
127+
RULES_YAML="$REPO_ROOT/.claude/config/rules.yaml" \
128+
"$SCRIPT_DIR/aggregate.sh" "$TMPDIR_RUN" > "$TMPDIR_RUN/summary.json"
129+
130+
# 8. Post signals (unless dry-run)
131+
if [ "$DRY_RUN" = "true" ]; then
132+
echo ""
133+
echo "▶ DRY-RUN summary:"
134+
jq -r '
135+
" BLOCK: \(.summary.block_count) FLAG: \(.summary.flag_count) SHADOW: \(.summary.shadow_count)",
136+
"",
137+
"Findings:",
138+
(.findings[] | " [\(.severity)] \(.rule) at \(.file):\(.line) — \(.message)")
139+
' "$TMPDIR_RUN/summary.json"
140+
echo ""
141+
echo "▶ Full report at: $TMPDIR_RUN/summary.json"
142+
exit "$(jq -r '.summary.block_count | if . > 0 then 1 else 0 end' "$TMPDIR_RUN/summary.json")"
143+
fi
144+
145+
# 9. Idempotency: delete prior bot artifacts
146+
delete_prior_bot_artifacts "$PR_NUMBER"
147+
148+
# 10. Post inline comments
149+
n_inline=$(jq '.findings | length' "$TMPDIR_RUN/summary.json")
150+
i=0
151+
while [ "$i" -lt "$n_inline" ]; do
152+
f=$(jq -c ".findings[$i]" "$TMPDIR_RUN/summary.json")
153+
file=$(echo "$f" | jq -r '.file')
154+
line=$(echo "$f" | jq -r '.line')
155+
rule=$(echo "$f" | jq -r '.rule')
156+
sev=$(echo "$f" | jq -r '.severity')
157+
msg=$(echo "$f" | jq -r '.message')
158+
body="<!-- sdk-review:v1 check=$rule id=$rule-$i -->
159+
**[$sev] $rule**
160+
161+
$msg"
162+
post_inline_comment "$PR_NUMBER" "$HEAD_SHA" "$file" "$line" "$body" || true
163+
i=$((i + 1))
164+
done
165+
166+
# 11. Post summary comment
167+
summary_body=$(jq -r '
168+
"<!-- sdk-review:v1 kind=summary -->
169+
## SDK Module Review
170+
171+
| Check | Status | Findings |
172+
|-------|--------|----------|
173+
" + (.per_check_summary | to_entries | map("| \(.key) | \(.value.status) | \(.value.count) |") | join("\n")) + "
174+
175+
<details><summary>Details</summary>
176+
177+
" + (.findings | map("- **[\(.severity)] \(.rule)** at `\(.file):\(.line)` — \(.message)") | join("\n")) + "
178+
179+
</details>
180+
181+
---
182+
_Generated by sdk-review-skill · v1_"
183+
' "$TMPDIR_RUN/summary.json")
184+
185+
post_summary_comment "$PR_NUMBER" "$summary_body"
186+
187+
# 12. Post check-run
188+
conclusion=$(jq -r 'if .summary.block_count > 0 then "failure" elif .summary.flag_count > 0 then "neutral" else "success" end' "$TMPDIR_RUN/summary.json")
189+
title=$(jq -r "\"SDK Review: \(.summary.block_count) BLOCK, \(.summary.flag_count) FLAG\"" "$TMPDIR_RUN/summary.json")
190+
post_or_update_check_run "$HEAD_SHA" "$conclusion" "$title" "$summary_body"
191+
192+
# 13. Apply label
193+
case "$conclusion" in
194+
success) label="sdk-review: ✅ passed" ;;
195+
neutral) label="sdk-review: ⚠️ flagged" ;;
196+
failure) label="sdk-review: ❌ blocked" ;;
197+
esac
198+
apply_label "$PR_NUMBER" "$label"
199+
200+
# 14. Exit
201+
exit "$(jq -r '.summary.block_count | if . > 0 then 1 else 0 end' "$TMPDIR_RUN/summary.json")"
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
name: SDK Module Review
2+
on:
3+
pull_request:
4+
types: [opened, synchronize, reopened, ready_for_review]
5+
workflow_dispatch:
6+
inputs:
7+
pr_number:
8+
description: "PR number to review"
9+
required: true
10+
11+
jobs:
12+
review:
13+
runs-on: ubuntu-latest
14+
if: github.event.pull_request.draft == false || github.event_name == 'workflow_dispatch'
15+
permissions:
16+
contents: read
17+
pull-requests: write
18+
checks: write
19+
issues: write
20+
steps:
21+
- uses: actions/checkout@v4
22+
with:
23+
fetch-depth: 0
24+
- name: Install jq
25+
run: sudo apt-get install -y jq
26+
- name: Install Python
27+
uses: actions/setup-python@v5
28+
with:
29+
python-version: '3.11'
30+
- name: Checkout sibling SDK repo (BDD parity)
31+
uses: actions/checkout@v4
32+
with:
33+
repository: ${{ vars.SIBLING_SDK_REPO }}
34+
path: .sibling-sdk
35+
token: ${{ secrets.SIBLING_SDK_TOKEN }}
36+
continue-on-error: true
37+
- name: Run SDK review
38+
env:
39+
PR_NUMBER: ${{ github.event.pull_request.number || inputs.pr_number }}
40+
SDK_SIBLING_PATH: ${{ github.workspace }}/.sibling-sdk
41+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
42+
run: bash .claude/scripts/orchestrate.sh "$PR_NUMBER"

0 commit comments

Comments
 (0)