Skip to content

chore: fix .claude/scripts BDD audit tooling defects#1665

Open
mkostromin-sigma wants to merge 5 commits into
prebid:mainfrom
mkostromin-sigma:chore/1664-claude-scripts-audit-fixes
Open

chore: fix .claude/scripts BDD audit tooling defects#1665
mkostromin-sigma wants to merge 5 commits into
prebid:mainfrom
mkostromin-sigma:chore/1664-claude-scripts-audit-fixes

Conversation

@mkostromin-sigma

@mkostromin-sigma mkostromin-sigma commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fix five defects in local .claude/scripts/ BDD audit helpers (surfaced as out-of-scope Bugbot noise on fix(ci): free disk before BDD In-Network tox sync #1663; not on the CI critical path).
  • audit_xfails.py: drop removed ast.Str; consult premature_xfails so PREMATURE_XFAIL is actually assigned; identifier-boundary match; string-only docstring skip.
  • inspect_parallel.sh: resolve inspector relative to the repo (INSPECT_BDD_STEPS override).
  • bdd_full_audit.py: partial xpass → PARTIAL_XPASS; all-four → GRADUATE; report iterates FIX_NOW (no parallel list).
  • salvage_audit_output.py: kind-scoped dedupe so Pass 2 deep traces are not duplicated on re-run.
  • Unit pins in tests/unit/test_claude_bdd_audit_scripts.py (chore: fix .claude/scripts BDD audit tooling defects #1665 review — all threads resolved after green CI).

CI carry-alongs (overlap with #1669)

While this PR was blocked on red suite jobs unrelated to .claude/scripts/, the tip also includes:

Prefer merging #1669 first for the shared CI pieces; this PR’s primary deliverable remains the audit-script fixes + unit pins. Rebase after #1669 if those commits become redundant.

Closes #1664.

Test plan

  • Unit: classify_xpassGRADUATE / PARTIAL_XPASS; generate_work_items splits both
  • Unit: salvage twice → deep count stable; triage+deep same name/line both survive
  • Unit: premature match is identifier-boundary; int Constant is not skipped as docstring
  • Unit: report renders PARTIAL_XPASS via FIX_NOW iteration
  • CI green on this tip
  • Review threads resolved after green CI

@KonstantinMirin KonstantinMirin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — PR #1665

Overview — This cleanly fixes four real defects in the .claude/scripts BDD audit tooling: the Python 3.12 ast.Str removal, the deep-trace dedup that re-wrote every record on each run (now kind-scoped keys + a write guard), the partial-xpass that mis-graduated as if all transports passed, and the inspector-path hardcoding. The salvage_audit_output.py refactor is exemplary — _store_key / _placeholder_step / _load_existing_keys were extracted and routed through every call site (load + pass-1 + pass-2), no leftover copy. Two things to resolve before merge: the three correctness fixes ship with no regression test (each reverts green), and the new PARTIAL_XPASS category is maintained in two hand-synced enumerations that will silently drift.

Should fix

Correctness fixes ship without a pinning test. The three behavior changes this PR exists to make each regress silently — nothing in the tree asserts them (no test imports these scripts):

  • Partial-xpass bucketing (bdd_full_audit.py classify_xpass / generate_work_items): revert the partial branch back to GRADUATE, or collapse grad_groups back to a single detail key, and the suite stays green — a partial xpass graduating "as if complete" would pass CI.
  • Deep-trace dedup (salvage_audit_output.py pass-2 write + _store_key): drop the new if key not in existing_keys: guard and re-running salvage silently duplicates every deep record again; drop the kind: prefix and a triage+deep record for the same name:line (always 0 for placeholders) collide and suppress each other.

Add unit tests (the four-transport BDD bar does not apply to this pure-Python meta-tooling — unit coverage is the right bar here): assert classify_xpass returns GRADUATE for a full transport set and PARTIAL_XPASS for a strict subset; assert generate_work_items on a mixed input yields two distinct work items; call write_to_store twice and assert the deep-record count does not grow; assert a triage and a deep record with identical function_name/line_number both survive.

One category, two parallel lists that must be hand-synced. Adding PARTIAL_XPASS required editing both the FIX_NOW dict (bdd_full_audit.py:50) and the hardcoded ordered render list in generate_report (bdd_full_audit.py:507-513). They are the same enumeration in the same order, written twice — the render loop does by_cat.get(cat, []) over the literal list, so a future category added to the dict but forgotten in the list has its work items collected and silently dropped from the report, no error. This push updated both, which is exactly why the coupling should go rather than be relied on. Drop the literal list and iterate the dict: for cat in FIX_NOW: (insertion order already matches the intended section order).

Nice to have

  • PREMATURE_XFAIL Priority-0 match is bare substring containment (audit_xfails.py:288, if fname and fname in haystack): a premature name that is a substring of another symbol or a traceback frame (check_x matching check_xy) misclassifies, and because Priority-0 returns immediately it shadows the correct category. Match on an identifier boundary (f"::{fname}" / f"def {fname}(") instead of raw in.
  • ast.Constant docstring skip (audit_xfails.py:194) is the correct 3.12 fix but now skips any constant expression, not just string docstrings; the comment still says "Skip docstrings". Narrow with and isinstance(stmt.value.value, str) if the intent is strictly docstrings.
  • classify_xpass docstring (bdd_full_audit.py:302) still says "All xpasses are FIX_NOW — remove the stale xfail tag", which now mis-describes the PARTIAL_XPASS branch ("investigate remaining gaps before graduating"). Reword to note both outcomes.
  • PARTIAL_XPASS (and its GRADUATE sibling) diverge from the sibling audit_xfails.py, which names the identical two xpass outcomes PARTIAL_PASS / STALE. Each script is internally consistent, but the suite no longer greps together. If a shared vocabulary is intended, reuse PARTIAL_PASS; otherwise leave it deliberately.
  • _load_existing_keys re-implements the JSONL splitlines/skip-blank/json.loads/obj["step"] scaffold already present in the step_lookup build loop; a tiny iter_jsonl_steps(path) generator would fold both. Optional.

Notes / prior-review follow-ups

No prior maintainer rounds on this PR — nothing to re-raise. Credit where due: the salvage_audit_output.py DRY extraction is the correct shape (helpers applied at every site, no orphaned inline copy), the pass-1 loop now also records existing_keys.add(key) for intra-run dedup, and the deep-trace rewrite-every-run bug is genuinely fixed.

Comment thread .claude/scripts/bdd_full_audit.py
Comment thread .claude/scripts/bdd_full_audit.py Outdated
Comment thread .claude/scripts/salvage_audit_output.py
Comment thread .claude/scripts/audit_xfails.py Outdated
Comment thread .claude/scripts/audit_xfails.py Outdated
mkostromin-sigma added a commit to mkostromin-sigma/salesagent that referenced this pull request Jul 16, 2026
Address prebid#1665 review: unit-pin GRADUATE/PARTIAL_XPASS and salvage
kind-scoped dedupe; iterate FIX_NOW for report sections; match premature
step names on identifier boundaries; skip string docstrings only.
@mkostromin-sigma

Copy link
Copy Markdown
Collaborator Author

Review follow-up (KonstantinMirin)

Addressed all should-fix + the two nits called out inline:

Item Resolution
Pin classify_xpass / generate_work_items tests/unit/test_claude_bdd_audit_scripts.py
Pin salvage deep dedupe + kind-scoped keys same module
Drop parallel FIX_NOW render list for cat in FIX_NOW:
Identifier-boundary premature match _mentions_identifier()
String-only docstring skip isinstance(..., str)

Also left deliberately: PARTIAL_XPASS vs sibling audit_xfails.PARTIAL_PASS vocabulary (internally consistent per script). Optional iter_jsonl_steps DRY left for a follow-up if wanted.

CI is fully green on the latest run (BDD In-Network included — temporarily carries the #1662 free-disk mitigation while #1669 is in flight).

Python 3.12-safe AST scan, wire PREMATURE_XFAIL classification, use
repo-local inspector path, label partial xpasses as PARTIAL_XPASS, and
dedupe salvage Pass 2 deep traces with kind-scoped keys.
Address prebid#1665 review: unit-pin GRADUATE/PARTIAL_XPASS and salvage
kind-scoped dedupe; iterate FIX_NOW for report sections; match premature
step names on identifier boundaries; skip string docstrings only.
Image build + /opt/venv + a second full tox env under tox_data overflowed
ubuntu-latest after prebid#1613/prebid#1634, ENOSPCing uv sync. Reclaim runner disk,
prune the builder cache, and cap PGDATA tmpfs for the serial e2e_rest leg.
@mkostromin-sigma
mkostromin-sigma force-pushed the chore/1664-claude-scripts-audit-fixes branch from ba265f8 to 4e0315e Compare July 16, 2026 23:35
Security Audit / pip-audit fail on mcp 1.27.2 (GHSA-vj7q-gjh5-988w).
Retries still failed on persistent list-tags 503; pinact v4 -no-api
enforces SHA pins without calling api.github.com.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Under Review

Development

Successfully merging this pull request may close these issues.

chore: fix .claude/scripts BDD audit tooling defects

2 participants