feat(sdk-review): foundation — libs + config + test harness#211
Draft
tiagoek wants to merge 14 commits into
Draft
feat(sdk-review): foundation — libs + config + test harness#211tiagoek wants to merge 14 commits into
tiagoek wants to merge 14 commits into
Conversation
This branch collects incremental sub-PRs for the SDK Module Review skill. Sub-PRs target this branch; a final PR will merge everything into main. Batches (see docs/plans/AFSDK-3937/06-INCREMENTAL-DELIVERY.md): - batch-1: Foundations (libs + config) - batch-2: AST + detectors - batch-3: Safety-first checks (secrets, license, disclosure, hardcode, telemetry) - batch-4: Structural checks (docs, bdd, patterns, versioning, commits) - batch-5: Depth checks (testing, errors, http, concurrency, deletion, deps, constants, binding, quality-gate, pr-size) - batch-6: Wire-up (orchestrate, aggregate, workflow, docs, CONTRIBUTING amendments) Ref: AFSDK-3937
Adds the foundation for the SDK Module Review skill: ## Libs (.claude/scripts/lib/) - json-emit.sh — findings JSON emitter + status derivation - diff-added-lines.sh — extract file:line pairs from diff (BSD-safe) - baseline.sh — apply baseline.json exemptions to reports - predicates.sh — scope predicates (commit_types, module_shape, REUSE.toml) - suppression.sh — parse # sdk-review: ignore[<check>] comments - apply-suppression.sh — filter reports by suppression tuples - github-api.sh — gh CLI wrapper (auth, comments, check-run, labels) - detect-language.sh — Python vs Java detection - detect-modules.sh — module extraction from diff - tier-manager.sh — apply SHADOW/FLAG/BLOCK/BLOCK_LOCKED tiers - skill-self-skip.sh — self-review protection (skip skill's own files) - ast_python_checks.py — Python AST checks (used by errors, telemetry, patterns, constants) - breaking-detector.py — API-diff detector for BREAKING family ## Config (.claude/config/) - rules.yaml — 70+ rules with tiers per rule - module-aliases.yaml — cross-language module name aliases (dms↔documentmanagement) - baseline.json — per-repo exemptions (REUSE.toml here → LIC-01/02 exempt) **Standalone testable.** No checks or orchestrator yet — comes in later batches. Part 1 of 6. Ref: AFSDK-3937
Explain why the alias file contains only one pair (dms ↔ documentmanagement): after auditing both SDKs, that's the only cross-language module today whose Python and Java names disagree. Same-name modules (destination, objectstore) don't need entries — the resolver falls back to the identical name. Include the full module inventory so future contributors know when to append here.
Adds 57 bats tests + 6 fixture diffs. Tests exercise the libs from batch 1. All check-specific tests use conditional skip guards so tests skip cleanly when scripts are not yet present in the current batch. Part 2 of 6. Ref: AFSDK-3937
The prior comment listed the module inventory as of 2026-07, which: - Confused readers (looked like the aliases list was incomplete) - Would age fast (SDK gains modules; comment would drift) - Duplicated what check-bdd.sh already discovers dynamically from src/ Keep the file to the minimum it needs to do its job: explain what the alias map is, when to add entries, and the one real cross-language rename today (dms ↔ documentmanagement). Module discovery is dynamic — no manual list.
The prior 'dms ↔ documentmanagement' alias was speculative: the Java repo
has never had a 'documentmanagement' module. The comment on this file
already stated the correct rule ('add an entry in the SAME PR that
introduces the divergent-name module') — the initial entry violated that
rule by preempting a module that doesn't exist.
Empty the list. Whoever creates 'documentmanagement' on the Java side (or
any other cross-language divergent-name module) adds the alias in that PR.
Discovery of module names is dynamic in check-bdd.sh — the empty list is
the correct starting state.
…agment is intentional for jq substring match
…nfig/baseline.json The baseline.file field pointed to .claude/baseline.json but the actual JSON lives at .claude/config/baseline.json (co-located with rules.yaml in the config dir). Any consumer of the rules.yaml baseline reference would have failed to find the file.
…olved Bug-3: strip PY-/JV- prefix from every rule key. rules.yaml uses agnostic LIC-01/LIC-02 as canonical rule IDs; baseline.json had drifted to PY-LIC-01/JV-LIC-01/etc, meaning baseline.sh is_in_line_baseline lookup never matched. Remove JV-* entries entirely (they belong to cloud-sdk-java's own baseline, not the Python repo's). Bug-6: at_commit was 'TBD' placeholder. Resolve to origin/main SHA at baseline creation time (d19427f). Add top-level _documentation key explaining the semantic invariant — baseline snapshots pre-existing debt against canonical main, not against a drifting foundation branch.
Add bats regression test asserting: 1. baseline.json is valid JSON 2. Every rule key in .exemptions appears in rules.yaml 3. at_commit is a real git SHA (not TBD placeholder) Guards against the drift Bohn caught in review — where baseline.json used PY-LIC-01 while rules.yaml used LIC-01, causing baseline lookup to always fail silently.
Empirical dry-run against the 19 open PRs (see docs/plans 09-FP-REMEDIATION) showed the v2 hunk-attribution primitive was specced but never wired: 84% of Python findings fired on lines the PR did not touch. This helper closes that gap. Individual check-*.sh scripts will consume is_line_touched / is_range_touched in a follow-up commit (per-check surgery to avoid one giant diff). Both helpers are permissive when $ADDED_LINES_FILE is unset (bats tests use synthetic diffs without the orchestrate wrapper) so nothing regresses in the isolated test harness.
peer-consistency.sh is a shared library (like hunk-filter, tier-manager, predicates). It belongs alongside the other libs in foundation, not downstream in the skill branch. Moving here so PR reviewers see the building blocks in the right layer.
…here File on disk is ast_python_checks.py (Python convention). Docstrings and bats test descriptions were using ast-python-checks (hyphen), creating noise for any grep-based automation or reviewer trying to match the filename.
Per Bohn review — bats tests were sitting in tests/sdk-review/ which polluted the product test tree (pytest CI runs against tests/, this tooling uses bats). Moving to .claude/tests/ keeps the skill self- contained under .claude/ and out of the product test space. BATS_TEST_DIRNAME hop counts unchanged (both paths are 2 levels deep from repo root). Verified all 60 bats tests still green. skill-self-skip.sh: .claude/* already covers .claude/tests/* — kept legacy tests/sdk-review/* line as belt-and-suspenders.
tiagoek
force-pushed
the
feat/sdk-review-foundation
branch
from
dfb70d7 to
f427f66
Compare
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
Foundation for the SDK Module Review skill (AFSDK-3937): shared bash/python libraries, config schema, and bats test harness. No behavior change — nothing runs against PRs from this PR alone.
Composes together with #212 (the skill).
Addresses maintainer review (2026-07-07)
All 7 in-scope defects flagged by @bohn-review resolved on this branch:
rules.yaml.baseline.filenow points to the actual location.claude/config/baseline.json(was pointing to.claude/baseline.json)baseline.jsonno longer usesPY-/JV-prefix; matches the agnostic form inrules.yaml. Regression testtest_baseline_contract.batsasserts the contract.<!-- sdk-review:v1documented as an intentional prefix (matches many concrete markers like<!-- sdk-review:v1 kind=summary -->via jqcontains()).hunk-filter.shandpeer-consistency.shmoved tofoundation(they are shared libs, not skill-only).baseline.jsonat_commitresolved to the actualorigin/mainSHA at baseline creation. Added_documentationkey explaining the semantic.ast_python_checks.py(file) vsast-python-checks.py(docstring) normalized to underscore.tests/sdk-review/to.claude/tests/, keeping the skill self-contained under.claude/and out of the product test tree.Bug 1 (rebase on
main) deferred to a supervised window — Pythonpyproject.tomlversion bump is trivial to resolve, but Javapom.xmlconflict resolution needs a careful eye. Will do both together in a follow-up.What's in this PR
.claude/scripts/lib/— 15 helper libs (JSON emit, tier manager, hunk filter, peer consistency, predicates, baseline snapshot, module aliases parser, GitHub API wrapper, self-skip, suppression, AST helpers, breaking-change detector).claude/config/—rules.yaml,baseline.json,module-aliases.yaml.claude/tests/— bats harness (test_smoke.bats,test_extended.bats,test_baseline_contract.bats) + fixturesWhat's NOT in this PR
check-*.shscripts (see feat(sdk-review): skill — 20 checks + orchestrate + GH Action + FP remediation #212)orchestrate.shand aggregation logic (see feat(sdk-review): skill — 20 checks + orchestrate + GH Action + FP remediation #212)Tests
bats .claude/tests/— 60 tests, all green. Some tests intentionallyskipwhen they need a check script from #212; that's the stack contract.Related: internal tracking AFSDK-3937.