feat(sdk-review): skill — 20 checks + orchestrate + GH Action + FP remediation#212
Draft
tiagoek wants to merge 11 commits into
Draft
feat(sdk-review): skill — 20 checks + orchestrate + GH Action + FP remediation#212tiagoek wants to merge 11 commits into
tiagoek wants to merge 11 commits into
Conversation
This was referenced Jul 7, 2026
tiagoek
force-pushed
the
feat/sdk-review-skill
branch
2 times, most recently
from
59e5ff8 to
a04c990
Compare
…mediation 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).
tiagoek
force-pushed
the
feat/sdk-review-foundation
branch
from
dfb70d7 to
f427f66
Compare
tiagoek
force-pushed
the
feat/sdk-review-skill
branch
from
a04c990 to
32daaf1
Compare
… + CON-01 exclusions The skill rebuild in earlier commit lost 60 lines of ast_python_checks (pt-04 check for Exception subclasses in __init__.py, plus CON-01 generated-file exclusions and per-file cap). Restore from the last known-good version. Bats now 79/79 green on skill tip.
BSD awk (macOS) requires the '/' inside a character class to be escaped as '\/'. GNU awk accepts either form. Discovered during live run on PR #209. Impact: check-testing-depth now returns valid JSON on macOS orchestrate runs; previously it silently emitted an awk error and aggregate.sh skipped the report.
Discovered during live run against PR #209. Multiple checks used the pattern: count=$(echo "$diff" | grep -E 'pattern' 2>/dev/null | wc -l | tr -d ' ') Under 'set -o pipefail', when grep finds zero matches it returns exit 1, which propagates through the pipe. Under 'set -e', the containing script exits silently — no stderr, no stdout, exit code 1. The 'aggregate.sh skips invalid JSON' safety net (added earlier) masked this by producing a summary that omitted the failing checks. Fix: wrap each grep-in-pipe with { grep … || true; } so zero-match returns success. Affects: - check-deps-supply.sh (count_lines helper) - check-pr-size.sh (count_lines helper + mods_count) - check-telemetry.sh (client_files, test_files, new_decorators) - check-testing-depth.sh (new_modules) - check-deletion-hygiene.sh (hits) Impact: live orchestrate.sh runs now emit valid JSON for all 20 checks instead of 15/20. No FP regression (all fixes preserve semantics). Discovered by: real dry-run on cloud-sdk-python PR #209 (Ricardo's agentgateway auditlog PR).
15 tasks
Discovered live on cloud-sdk-python PR #209 (Ricardo, agentgateway auditlog). The check was counting string occurrences from the on-disk AST — global to the file — then anchoring the finding to the first occurrence's line number. When a PR (a) shifts line numbers by adding upstream code and (b) does not touch any of the actual repeated-string lines, PY-CON-01 fires for tech debt that pre-existed the PR. Fix: read $ADDED_LINES_FILE (already exported by orchestrate.sh) inside check_con_01. Filter each literal's occurrence list to lines actually in the PR's added set. Only fire when ${filtered} is non-empty AND total occurrences (across file, unchanged) still >= threshold. Anchor line becomes the first added-line occurrence. Backward-compat: if ADDED_LINES_FILE is unset (bats tests exercising the lib directly), fall back to the legacy full-file behavior. Two new regression tests pin both branches (untouched pre-existing repetition = no fire; at least one occurrence on added line = fires with correct anchor). Bats: 81/81 green on both repos.
…ation 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.
…main branch)
Two more FPs from live-posting to real PRs (jv#5, py#161):
FP-P-01: github-api.sh 'gh api' calls didn't pin --hostname, so every call
against the internal github.tools.sap repo hit github.com and 404'd —
no summary/label/check-run posted on Java PRs. Added gh_api() wrapper that
injects --hostname $(detect_hostname); all 12 call sites use it.
FP-Q-01: breaking-detector flagged deletions of symbols the PR never touched.
When a PR branch is behind main, base..head surfaces files+symbols that MAIN
added; the AST diff read those as PR deletions (47 false BREAKING details on
py#161, all BLOCK_LOCKED). Two-layer fix:
1. files_changed() intersects with the PR's own changed files
(from ADDED_LINES_FILE).
2. every deletion finding (api_removal, method/field/enum deletion) is
gated on the removed symbol actually appearing on a '-' line of the
PR diff (DIFF_FILE). No diff context → no gating (backward-compat).
Validated: py#161 8 BLOCK → 6 (the 2 BREAKING FPs gone; 6 remaining are real
TPs on a new module). jv#5 posts label+summary correctly now.
Bats 81/81 both repos.
git log BASE..HEAD fails when HEAD_SHA is a PR ref not present in the local
checkout; under set -e/pipefail that killed the whole check (invalid JSON,
aggregate skipped it). Wrap in { … || true; }.
…t__) + 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.
…bust idempotency
Reported problems from live posting (2026-07-09):
1. bug-T-01: per_check_summary table count didn't match posted findings —
table counted raw per-report findings (6) while only 3 survived tier-gating
and posted. Now aggregate.sh stamps each finding with its .check and counts
ONLY posted (retagged) findings per check. disclosure/pr-size that get
shadowed show 0. Sum of table counts == total findings (asserted).
2. Non-anchored findings (PR_BODY, missing files/dirs, commit metadata, tests/
synthetic paths) no longer attempt a failed inline post. orchestrate splits:
findings whose file:line is in ADDED_LINES_FILE post inline; the rest go
into a 'Findings not tied to a specific code line' section of the summary,
plus a reconciliation line ('N findings: X inline, Y listed above').
3. Idempotency hardened: delete_prior_bot_artifacts loops up to 5×, re-listing
until no bot comment remains — kills the duplicate-summary / repeated-comment
accumulation caused by a single delete pass racing pagination.
Regression tests: new .claude/tests/test_signals.bats (5 tests) pins the
reconciliation invariant, shadow exclusion, PASS-check presence, and .check
stamping. Bats 86/86 both repos.
DC-02 emitted $user_guide which included $REPO_ROOT, leaking the local checkout path (e.g. /tmp/py161-tree/src/...) into the PR comment. Strip the REPO_ROOT prefix so it shows src/sap_cloud_sdk/<mod>/user-guide.md.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Automated SDK PR review skill for
cloud-sdk-python(AFSDK-3937). Wires 20 deterministic checks that post 4 signals per PR: inline comments, summary comment, GitHub check-run, and PR label.Depends on #211 (foundation — libs + config + test harness). Once #211 merges, rebase this on
mainand the diff shrinks to just the checks + orchestration.Addresses maintainer review (2026-07-07)
rules.yamlbaseline path8f20445LIC-01vsPY-LIC-01)5ff6fc3+fca651f726caf5hunk-filter.sh+peer-consistency.shmoved to foundation08f1309+a20b8c5baseline.jsonat_commitresolved5ff6fc3ast_python_checks.pynaming standardized7597a93.claude/tests/f427f66main(version bump behind)pom.xmlversion conflict resolution on Java side (same discipline applied here for consistency)What this delivers
Rule engine — 20 check scripts across 4 domains:
check-secrets,check-license-spdx,check-disclosure,check-hardcode,check-telemetrycheck-docs,check-bdd,check-patterns,check-versioning(with BREAKING family),check-commitscheck-errors-logging,check-testing-depth,check-http-hygiene,check-concurrency,check-deps-supply,check-deletion-hygiene,check-constants,check-binding-shape,check-quality-gate-parity,check-pr-sizeOrchestration —
orchestrate.shruns all 20 in parallel per PR, aggregates viaaggregate.shwith rule-tier post-processing, and posts 4 signals throughlib/github-api.sh.Empirical FP remediation — the skill was dry-run against 26+ real PRs across both SDKs. Initial FP rate was 84%. This PR includes 10 targeted fixes (FP-A-01 through FP-J-01) that brought FP rate to <15% while preserving all real BLOCKs.
Documentation —
docs/PR-REVIEW.mdexplains what runs, how to suppress, how to re-run.CONTRIBUTING.mdgets a new "Incremental Delivery" section referenced by the PR-SIZE-* advisory rule.Tests — 79 bats tests (60 from foundation + 19 regression tests pinning each FP fix). All green.
GitHub Action —
.github/workflows/sdk-module-review.ymltriggers onopened,synchronize,reopened,ready_for_review. Zero LLM in CI. Slash commands available for local dev iteration.Related: internal tracking AFSDK-3937.