Skip to content

Commit 69b3401

Browse files
tiagoektiagoek
committed
fix(sdk-review): FP-L/M/N/O + COM-01 tier + perf — from live PR validation
Discovered running the skill against 3 real PRs (py#209, py#161, jv#5): FP-L-01: breaking-detector no longer flags additive signature changes (adding an optional trailing arg). New _is_breaking_sig_change() with 6 unit-tested cases. Signature tuple extended to 6 elements (default counts). Was BLOCK_LOCKED — the most damaging FP. FP-M-01: detect-language.sh handles multi-module Maven (<module>/src/main/java), not just root src/main/java. cloud-sdk-java is multi-module after the artifact-separation PR. FP-N-01: performance. hardcode/secrets/disclosure/concurrency/http-hygiene used a shell loop over every added line (80k forks on a 13k-line diff → timeout). Added awk pre-filter so only candidate lines reach the loop. check-hardcode on jv#5: 120s+ → 1.8s. FP-O-01: ignore_files / test-file skips cover <module>/src/test/ (multi-module), so HC-* and BND-02 no longer fire on Java test fixtures. COM-01: tier BLOCK → FLAG. Commit-message convention violations flooded reviews (33 BLOCKs on py#161); squash-merge normalizes them anyway. pipefail hardening: check-versioning (version grep|head), check-docs (grep|sed|sort), check-license-spdx (find|head SIGPIPE) all wrapped so a zero-match no longer silently kills the script under set -e. check-docs / check-license-spdx: Java module detection + SPDX sampling now walk the multi-module tree. orchestrate: per-check timeout 60s → 180s (belt-and-suspenders on top of the perf fix). Validated: jv#5 now 0 invalid-JSON reports (was 9), 0 BLOCK, 3 real FLAGs. Bats 81/81 green both repos.
1 parent 6c4bdb4 commit 69b3401

15 files changed

Lines changed: 283 additions & 50 deletions

‎.claude/config/rules.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ rules:
107107
DEP-04: { tier: BLOCK_LOCKED }
108108

109109
# -------- COMMITS --------
110-
COM-01: { tier: BLOCK }
110+
COM-01: { tier: FLAG }
111111

112112
# -------- DELETION --------
113113
DEL-01: { tier: BLOCK }

‎.claude/scripts/check-binding-shape.sh‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,23 @@ findings=$(mktemp); trap 'rm -f "$findings"' EXIT
1515
diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "")
1616

1717
# BND-02 (LOCKED): token URL via string concat + "/oauth/token"
18+
# FP-N-01: pre-filter — only lines mentioning /oauth/token can match.
19+
# FP-O-01: skip test files (multi-module: <module>/src/test/…) — a hardcoded
20+
# token path in a test fixture is not production binding code.
1821
echo "$diff_content" | awk '
19-
BEGIN { file=""; line=0 }
20-
/^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next }
22+
BEGIN { file=""; line=0; skip=0 }
23+
/^diff --git a\// {
24+
file=$4; sub(/^b\//, "", file); line=0
25+
skip = (file ~ /(^|\/)src\/test\// || file ~ /(^|\/)(tests?|mocks?)\//) ? 1 : 0
26+
next
27+
}
2128
/^@@/ { 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 }
29+
/^\+/ && !/^\+\+\+/ {
30+
c = substr($0, 2)
31+
if (!skip && c ~ /\/oauth\/token/) { print file "\t" line "\t" c }
32+
line++
33+
next
34+
}
2335
/^ / { line++; next }
2436
' | while IFS=$'\t' read -r file line_num content; do
2537
[ -z "$file" ] && continue

‎.claude/scripts/check-commits.sh‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ echo "$commits" | while IFS= read -r line; do
3737
# Also skip legacy "Merge branch/pull/etc" defaults
3838
if echo "$subject" | grep -qiE '^Merge '; then continue; fi
3939
if ! echo "$subject" | grep -qE "$prefix_regex"; then
40-
emit_finding "COM-01" "BLOCK" "COMMIT:$sha" 1 \
40+
emit_finding "COM-01" "FLAG" "COMMIT:$sha" 1 \
4141
"Commit '$subject' does not follow Conventional Commits" \
4242
"Prefix with feat:/fix:/chore:/docs:/test:/refactor:/ci:/build:/perf:/style:/revert:" >> "$findings"
4343
fi

‎.claude/scripts/check-concurrency.sh‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,17 @@ findings=$(mktemp); trap 'rm -f "$findings"' EXIT
1616
diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "")
1717

1818
# CC-01: asyncio.Queue with no accompanying Lock/set — flag when queue+append pattern present
19+
# FP-N-01: pre-filter — only lines mentioning asyncio.Queue can match.
1920
echo "$diff_content" | awk '
2021
BEGIN { file=""; line=0 }
2122
/^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next }
2223
/^@@/ { 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+
/^\+/ && !/^\+\+\+/ {
25+
c = substr($0, 2)
26+
if (c ~ /asyncio\.Queue\(/) { print file "\t" line "\t" c }
27+
line++
28+
next
29+
}
2430
/^ / { line++; next }
2531
' | while IFS=$'\t' read -r file line_num content; do
2632
[ -z "$file" ] && continue

‎.claude/scripts/check-disclosure.sh‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,20 @@ diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "")
2626
sev_public() { if [ "$PROFILE" = "public" ]; then echo "BLOCK"; else echo "FLAG"; fi; }
2727
sev_internal_ok() { if [ "$PROFILE" = "public" ]; then echo "BLOCK"; else echo "PASS"; fi; }
2828

29-
# Scan added lines only
29+
# Scan added lines only.
30+
# FP-N-01: pre-filter in awk. All DIS-* rules key off SAP-internal hostnames
31+
# or --index-url. Only lines containing 'sap' (case-insensitive) or
32+
# 'index-url' can match — everything else is skipped before the shell loop.
3033
echo "$diff_content" | awk '
3134
BEGIN { file=""; line=0 }
3235
/^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next }
3336
/^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next }
34-
/^\+/ && !/^\+\+\+/ { print file "\t" line "\t" substr($0, 2); line++; next }
37+
/^\+/ && !/^\+\+\+/ {
38+
c = substr($0, 2)
39+
if (c ~ /[Ss][Aa][Pp]|index-url/) { print file "\t" line "\t" c }
40+
line++
41+
next
42+
}
3543
/^ / { line++; next }
3644
' | while IFS=$'\t' read -r file line_num content; do
3745
[ -z "$file" ] && continue

‎.claude/scripts/check-docs.sh‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,12 @@ findings=$(mktemp); trap 'rm -f "$findings"' EXIT
1414
diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "")
1515

1616
# Detect new module directories (module has new files at top-level)
17+
# FP-M-01: multi-module Maven nests sources under <module>/src/main/java.
18+
# Match the module segment after .../com/sap/cloud/sdk/ at any prefix depth.
1719
if [ "$LANGUAGE" = "python" ]; then
18-
new_modules=$(echo "$diff_content" | grep -oE '^\+\+\+ b/src/sap_cloud_sdk/[a-z_]+/[^/]+\.py' | sed 's|^+++ b/src/sap_cloud_sdk/||; s|/[^/]*\.py$||' | sort -u)
20+
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)
1921
else
20-
new_modules=$(echo "$diff_content" | grep -oE '^\+\+\+ b/src/main/java/com/sap/cloud/sdk/[a-z_]+/[^/]+\.java' | sed 's|^+++ b/src/main/java/com/sap/cloud/sdk/||; s|/[^/]*\.java$||' | sort -u)
22+
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)
2123
fi
2224

2325
while IFS= read -r mod; do

‎.claude/scripts/check-hardcode.sh‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,22 +28,46 @@ LOCKFILE_PATTERNS='.*\.lock$|(.*/)?uv\.lock$|(.*/)?poetry\.lock$|(.*/)?Pipfile\.
2828
# .env, .env.X, .env_X, .env-X — any file whose basename starts with .env
2929
ENV_EXAMPLE_PATTERNS='(.*/)?\.env(\..*|_.*|-.*)?$'
3030
if [ "$LANGUAGE" = "python" ]; then
31-
ignore_files="^(tests?/|mocks?/|docs?/|.*/spec/|.*/constants\.py|.*/user-guide\.md|README\.md|.*\.md$|.*\.ya?ml$|.*\.xml$|.*\.properties$|.*pom\.xml$|${LOCKFILE_PATTERNS}|${ENV_EXAMPLE_PATTERNS})"
31+
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})"
3232
else
33-
ignore_files="^(src/test/|mocks?/|docs?/|.*Constants\.java|.*/constants/|.*/user-guide\.md|README\.md|.*\.md$|.*\.ya?ml$|.*\.xml$|.*\.properties$|.*pom\.xml$|${LOCKFILE_PATTERNS}|${ENV_EXAMPLE_PATTERNS})"
33+
# FP-O-01: multi-module Maven puts tests at <module>/src/test/java, not
34+
# src/test/. Match src/test/ at any depth. Same for mocks/constants.
35+
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})"
3436
fi
3537

36-
echo "$diff_content" | awk '
38+
# FP-N-01: performance. A per-line shell loop over the full added-line set
39+
# forks 5-6 subprocesses per line; on a 13k-line diff that is ~80k forks and
40+
# blows past the orchestrate timeout. Pre-filter in awk so ONLY lines that
41+
# could match some HC-* rule reach the shell loop. The awk stage also does
42+
# the file-level ignore so we never even emit lines from ignored files.
43+
#
44+
# Interesting-line heuristic (broad — real rule regexes still run in-loop):
45+
# - contains http:// or https:// (HC-01)
46+
# - contains "Authorization" or "Bearer" (HC-02)
47+
# - contains os.environ[ or System.getenv( (HC-04)
48+
# - contains timeout/retries/max_retries = (HC-06)
49+
echo "$diff_content" | awk -v ignore="$ignore_files" '
3750
BEGIN { file=""; line=0 }
3851
/^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next }
3952
/^@@/ { if (match($0, /\+[0-9]+/)) line=substr($0, RSTART+1, RLENGTH-1)+0; next }
40-
/^\+/ && !/^\+\+\+/ { print file "\t" line "\t" substr($0, 2); line++; next }
53+
/^\+/ && !/^\+\+\+/ {
54+
c = substr($0, 2)
55+
# File-level ignore (awk regex; mirrors the shell ignore_files pattern)
56+
if (file ~ ignore) { line++; next }
57+
# Interesting-line pre-filter
58+
if (c ~ /https?:\/\// || \
59+
c ~ /Authorization|Bearer/ || \
60+
c ~ /os\.environ\[|System\.getenv\(/ || \
61+
c ~ /(timeout|retries|max_retries)[ \t]*=[ \t]*[0-9]/) {
62+
print file "\t" line "\t" c
63+
}
64+
line++
65+
next
66+
}
4167
/^ / { line++; next }
4268
' | while IFS=$'\t' read -r file line_num content; do
4369
[ -z "$file" ] && continue
4470
if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi
45-
# Filter out test/mock/docs/constants files
46-
if echo "$file" | grep -qE "$ignore_files"; then continue; fi
4771

4872
# HC-01: hardcoded URL. Extract each URL and check individually so a line
4973
# with both example.com (allowed) and api.com (real) still fires on the real one.

‎.claude/scripts/check-http-hygiene.sh‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,29 @@ findings=$(mktemp); trap 'rm -f "$findings"' EXIT
1616
diff_content=$(cat "$DIFF_FILE" 2>/dev/null || echo "")
1717

1818
# HTTP-PY-01: Session per invocation (only fire on newly added lines)
19+
# FP-N-01: pre-filter — only lines that create a session/client can match,
20+
# and skip test/mock/doc/example/markdown files right in awk.
1921
echo "$diff_content" | awk '
20-
BEGIN { file=""; line=0 }
21-
/^diff --git a\// { file=$4; sub(/^b\//, "", file); line=0; next }
22+
BEGIN { file=""; line=0; skip=0 }
23+
/^diff --git a\// {
24+
file=$4; sub(/^b\//, "", file); line=0
25+
# File-level skip: tests/mocks/docs/examples (any depth) + md/rst/txt
26+
skip = (file ~ /(^|\/)(tests?|mocks?|docs?|examples?)\// || file ~ /\.(md|rst|txt)$/) ? 1 : 0
27+
next
28+
}
2229
/^@@/ { 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 }
30+
/^\+/ && !/^\+\+\+/ {
31+
c = substr($0, 2)
32+
if (!skip && c ~ /(requests|httpx)\.(Session|AsyncClient)\(\)/) {
33+
print file "\t" line "\t" c
34+
}
35+
line++
36+
next
37+
}
2438
/^ / { line++; next }
2539
' | while IFS=$'\t' read -r file line_num content; do
2640
[ -z "$file" ] && continue
2741
if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi
28-
# only for impl files (not tests/mocks/docs/examples/markdown)
29-
# FP-B-02: HTTP-01 must not fire on markdown code fences.
30-
if echo "$file" | grep -qE '^(tests?/|mocks?/|docs?/|examples?/)'; then continue; fi
31-
if echo "$file" | grep -qiE '\.(md|rst|txt)$'; then continue; fi
3242
if echo "$content" | grep -qE '(requests|httpx)\.(Session|AsyncClient)\(\)'; then
3343
emit_finding_if_touched "HTTP-01" "FLAG" "$file" "$line_num" \
3444
"HTTP session created per invocation — prefer single instance in __init__" "" >> "$findings"

‎.claude/scripts/check-license-spdx.sh‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,23 @@ if [ "$reuse_present" != "true" ]; then
3131
src_root="$REPO_ROOT/src"
3232
ext="py"
3333
else
34-
src_root="$REPO_ROOT/src/main/java"
34+
# Multi-module Maven: sources live under <module>/src/main/java, so scan
35+
# the whole repo for .java rather than a fixed src/main/java root.
36+
src_root="$REPO_ROOT"
3537
ext="java"
3638
fi
3739
if [ -d "$src_root" ]; then
3840
total=0; with_spdx=0
41+
# Sample up to 20 files. Read them into an array first so no pipe stays
42+
# open when we stop early (avoids SIGPIPE / exit 141 under pipefail).
43+
sample_files=$(find "$src_root" -type f -name "*.${ext}" 2>/dev/null | head -20 || true)
3944
while IFS= read -r f; do
45+
[ -z "$f" ] && continue
4046
total=$((total+1))
4147
if head -10 "$f" 2>/dev/null | grep -q "SPDX-License-Identifier"; then
4248
with_spdx=$((with_spdx+1))
4349
fi
44-
done < <(find "$src_root" -type f -name "*.${ext}" 2>/dev/null | head -20)
50+
done <<< "$sample_files"
4551
if [ "$total" -gt 0 ]; then
4652
# Percent threshold: <20% adoption means the repo hasn't converged yet
4753
if [ $((with_spdx * 100 / total)) -lt 20 ]; then

‎.claude/scripts/check-secrets.sh‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,12 @@ fi
2727
findings=$(mktemp)
2828
trap 'rm -f "$findings"' EXIT
2929

30-
# Parse diff and scan each added line
30+
# Parse diff and scan each added line.
31+
# FP-N-01: pre-filter in awk so only lines that could contain a secret reach
32+
# the shell loop. The heuristic is a BROAD superset of every SEC-* anchor —
33+
# the precise per-rule regexes still run in the loop. Secrets are
34+
# BLOCK_LOCKED, so the pre-filter is intentionally over-inclusive (favor
35+
# false-inclusion over ever missing a real secret).
3136
echo "$diff_content" | awk '
3237
BEGIN { file=""; line=0 }
3338
/^diff --git a\// {
@@ -41,7 +46,12 @@ echo "$diff_content" | awk '
4146
next
4247
}
4348
/^\+/ && !/^\+\+\+/ {
44-
print file "\t" line "\t" substr($0, 2)
49+
c = substr($0, 2)
50+
# Broad superset of SEC-01..08 anchors:
51+
# AKIA / AIza / gh?_ / sk- / xox / eyJ / PRIVATE KEY / clientsecret
52+
if (c ~ /AKIA|AIza|gh[pousr]_|sk-|xox[baprs]-|eyJ|PRIVATE KEY|[Cc]lient[_]?[Ss]ecret/) {
53+
print file "\t" line "\t" c
54+
}
4555
line++
4656
next
4757
}

0 commit comments

Comments
 (0)