Skip to content

Commit 873c585

Browse files
committed
fix(sdk-review): FP-R-01 (docstring URLs) + FP-S-01 (Session in __init__) + COM-01 squash-subject
Three more FPs surfaced reviewing what actually posted to py#161: FP-R-01: HC-01 fired on 'https://dms.example.com/...' inside a docstring Example: block — documentation, not runtime code. Added 'docstring-lines' AST helper; check-hardcode skips URLs on docstring lines (Python). FP-S-01: HTTP-01 fired on requests.Session() created in __init__ — which is the pattern the rule RECOMMENDS. Added 'http-session-lines' AST helper that lists only sessions created OUTSIDE __init__ (per-invocation); check-http- hygiene fires only on those. COM-01: rewritten to check a single subject — the PR title (squash-merge subject that lands on main), falling back to HEAD's non-merge subject. Was emitting one finding per intermediate commit (33 on py#161); those messages are discarded by squash-merge. orchestrate now exports PR_TITLE. Net effect on py#161: 8 BLOCK + 34 FLAG → 4 BLOCK + 0 FLAG, all 4 real TPs (DIS-07, DC-02, PY-TEL-06, TD-10 on a genuinely new module). Zero FP. Bats 81/81 both repos.
1 parent 0c4e956 commit 873c585

5 files changed

Lines changed: 112 additions & 28 deletions

File tree

.claude/scripts/check-commits.sh

Lines changed: 23 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,42 @@
11
#!/usr/bin/env bash
22
# check-commits.sh — Conventional Commits enforcement.
3+
#
4+
# The repos squash-merge PRs, so ONLY the final squashed subject (which
5+
# defaults to the PR title) becomes a commit on main. Checking every
6+
# intermediate commit floods the review with findings for messages that
7+
# will be discarded. We therefore check a single subject:
8+
# 1. PR_TITLE (env, set by orchestrate from the PR metadata) if available
9+
# 2. else the most recent non-merge commit subject (HEAD)
310
set -euo pipefail
411
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
512
source "$SCRIPT_DIR/lib/json-emit.sh"
613

714
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
2115
HEAD_SHA="${HEAD_SHA:-HEAD}"
16+
PR_TITLE="${PR_TITLE:-}"
2217

2318
STARTED=$(now_iso)
2419
findings=$(mktemp); trap 'rm -f "$findings"' EXIT
2520

26-
# List commit subjects
27-
commits=$(git log "${BASE_SHA}..${HEAD_SHA}" --format='%H %s' 2>/dev/null || echo "")
2821
prefix_regex='^(feat|fix|refactor|chore|docs|test|ci|build|perf|style|revert)(\([a-z0-9_/-]+\))?!?:[[:space:]]+.+'
2922

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
23+
# Pick the single subject that will land on main after squash-merge.
24+
if [ -n "$PR_TITLE" ]; then
25+
subject="$PR_TITLE"
26+
location="PR_TITLE"
27+
else
28+
# Most recent non-merge commit subject.
29+
subject=$({ git log "$HEAD_SHA" --no-merges -1 --format='%s' 2>/dev/null || true; })
30+
location="COMMIT:$({ git rev-parse "$HEAD_SHA" 2>/dev/null || echo HEAD; })"
31+
fi
32+
33+
if [ -n "$subject" ] && ! echo "$subject" | grep -qiE '^Merge '; then
3934
if ! echo "$subject" | grep -qE "$prefix_regex"; then
40-
emit_finding "COM-01" "FLAG" "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"
35+
emit_finding "COM-01" "FLAG" "$location" 1 \
36+
"Squash-merge subject '$subject' does not follow Conventional Commits" \
37+
"The PR title becomes the squashed commit — prefix with feat:/fix:/chore:/docs:/test:/refactor:/ci:/build:/perf:/style:/revert:" >> "$findings"
4338
fi
44-
done
39+
fi
4540

4641
status=$(status_from_findings < "$findings")
4742
emit_report "commits" "$LANGUAGE" "$status" "$STARTED" < "$findings"

.claude/scripts/check-hardcode.sh

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,21 @@ echo "$diff_content" | awk -v ignore="$ignore_files" '
6969
[ -z "$file" ] && continue
7070
if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi
7171

72+
# FP-R-01: skip URLs that live inside a Python docstring (documentation
73+
# examples, not runtime code). Compute the docstring line set once per file.
74+
if [ "$LANGUAGE" = "python" ] && echo "$file" | grep -q '\.py$'; then
75+
if [ "${_docstring_file:-}" != "$file" ]; then
76+
_docstring_file="$file"
77+
_docstring_lines=""
78+
if [ -f "$REPO_ROOT/$file" ]; then
79+
_docstring_lines=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" docstring-lines "$REPO_ROOT/$file" 2>/dev/null || echo "")
80+
fi
81+
fi
82+
if [ -n "$_docstring_lines" ] && echo "$_docstring_lines" | grep -qx "$line_num"; then
83+
continue
84+
fi
85+
fi
86+
7287
# HC-01: hardcoded URL. Extract each URL and check individually so a line
7388
# with both example.com (allowed) and api.com (real) still fires on the real one.
7489
# Use word boundary via (^|[^A-Za-z0-9]) so we don't match tokens inside identifiers.

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,19 @@ echo "$diff_content" | awk '
4040
[ -z "$file" ] && continue
4141
if [ "$(is_skill_file "$file")" = "true" ]; then continue; fi
4242
if echo "$content" | grep -qE '(requests|httpx)\.(Session|AsyncClient)\(\)'; then
43+
# FP-S-01: a Session created in __init__ is the RECOMMENDED pattern, not a
44+
# finding. Use AST to confirm this line is a per-invocation session (i.e.
45+
# created outside __init__). Only fire if the line is in that set. When the
46+
# file isn't on disk (no REPO_ROOT context), fall back to firing.
47+
if [ "$LANGUAGE" = "python" ] && echo "$file" | grep -q '\.py$' && [ -f "$REPO_ROOT/$file" ]; then
48+
if [ "${_http_file:-}" != "$file" ]; then
49+
_http_file="$file"
50+
_http_lines=$(python3 "$SCRIPT_DIR/lib/ast_python_checks.py" http-session-lines "$REPO_ROOT/$file" 2>/dev/null || echo "")
51+
fi
52+
if ! echo "$_http_lines" | grep -qx "$line_num"; then
53+
continue # session is in __init__ (or class/module level) → correct pattern
54+
fi
55+
fi
4356
emit_finding_if_touched "HTTP-01" "FLAG" "$file" "$line_num" \
4457
"HTTP session created per invocation — prefer single instance in __init__" "" >> "$findings"
4558
fi

.claude/scripts/lib/ast_python_checks.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,64 @@ def main(argv: list[str]) -> int:
501501
module_dir = files[0]
502502
return 0 if check_pt_04(module_dir) else 1
503503

504+
# FP-R-01: docstring-lines prints "1" for every line number that falls
505+
# inside a module/class/function docstring, so line-based checks (e.g.
506+
# check-hardcode HC-01) can skip URLs that live in documentation examples.
507+
if check_name == "docstring-lines":
508+
for f in files:
509+
tree = parse_file(f)
510+
if tree is None:
511+
continue
512+
for node in ast.walk(tree):
513+
if not isinstance(
514+
node, (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)
515+
):
516+
continue
517+
doc = ast.get_docstring(node, clean=False)
518+
if not doc:
519+
continue
520+
# The docstring is the first statement's Constant node.
521+
body = getattr(node, "body", [])
522+
if not body:
523+
continue
524+
first = body[0]
525+
if (
526+
isinstance(first, ast.Expr)
527+
and isinstance(first.value, ast.Constant)
528+
and isinstance(first.value.value, str)
529+
):
530+
start = first.value.lineno
531+
end = getattr(first.value, "end_lineno", start)
532+
for ln in range(start, end + 1):
533+
print(ln)
534+
return 0
535+
536+
# FP-S-01: http-session-lines prints the line number of every
537+
# requests.Session()/httpx.Client()/AsyncClient() call that is NOT inside
538+
# __init__ (or a module/class-level assignment). A session created in
539+
# __init__ is the RECOMMENDED pattern, so HTTP-01 must not fire on it.
540+
# Only lines printed here are per-invocation sessions worth flagging.
541+
if check_name == "http-session-lines":
542+
SESSION_CTORS = {"Session", "Client", "AsyncClient"}
543+
for f in files:
544+
tree = parse_file(f)
545+
if tree is None:
546+
continue
547+
# Map every function def to whether it is __init__.
548+
for node in ast.walk(tree):
549+
if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
550+
continue
551+
if node.name == "__init__":
552+
continue # sessions here are the correct pattern
553+
for sub in ast.walk(node):
554+
if (
555+
isinstance(sub, ast.Call)
556+
and isinstance(sub.func, ast.Attribute)
557+
and sub.func.attr in SESSION_CTORS
558+
):
559+
print(sub.lineno)
560+
return 0
561+
504562
if check_name not in CHECKS:
505563
print(f"ERROR: unknown check {check_name}", file=sys.stderr)
506564
return 2

.claude/scripts/orchestrate.sh

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ fi
5757
if [ "$DRY_RUN" != "true" ]; then
5858
gh pr view "$PR_NUMBER" --json body -q .body > "$TMPDIR_RUN/pr-body.txt" 2>/dev/null || echo "" > "$TMPDIR_RUN/pr-body.txt"
5959
HEAD_SHA=$(get_pr_head_sha "$PR_NUMBER")
60+
# PR title feeds check-commits (squash-merge subject). Non-fatal if it fails.
61+
PR_TITLE=$(gh pr view "$PR_NUMBER" --json title -q .title 2>/dev/null || echo "")
62+
export PR_TITLE
6063
elif [ -n "${LOCAL_PR_BODY:-}" ] && [ -f "$LOCAL_PR_BODY" ]; then
6164
cp "$LOCAL_PR_BODY" "$TMPDIR_RUN/pr-body.txt"
6265
HEAD_SHA="${HEAD_SHA:-HEAD}"

0 commit comments

Comments
 (0)