azcomputerguru/claudetools
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
dbe355a2d5dcf8263b0fbe08bb93e5c838f4e69c
claudetools/session-logs/2026-06/2026-06-25-howard-bitdefender-skill-audit-fixes.md
Howard Enos d87aa9dfd8 sync: auto-sync from HOWARD-HOME at 2026-06-25 14:58:19 
Author: Howard Enos
Machine: HOWARD-HOME
Timestamp: 2026-06-25 14:58:19
2026-06-25 14:58:50 -07:00

9.8 KiB
Raw Blame History

Session Log — 2026-06-25 — Bitdefender skill audit + iterative fix-to-convergence

User

  • User: Howard Enos (howard)
  • Machine: Howard-Home
  • Role: tech

Session Summary

Audited the bitdefender skill (GravityZone Cloud JSON-RPC CLI over the live ACG partner tenant; .claude/skills/bitdefender/, ~3,200 lines across gz.py CLI + gz_client.py API client + selftest.py) for correctness, CLAUDE.md compliance, and bugs, then fixed everything found and looped review->fix->re-review until two consecutive review passes returned no MEDIUM+ defects.

The first audit (two parallel review sub-agents, one per big file) found two CRITICAL gating gaps on the live tenant: move, scan, create-package, make-group called the API with no --confirm gate, and the raw destructive-method denylist omitted the matching method names so raw bypassed the gates too. It also found no HTTP 429 backoff (a real 429 was in errorlog), no client-side object-id validation, non-atomic cache writes, and assorted doc drift. Fixed the gating cluster (C1/C2/H1/H3/M1) first, then H2 (429/5xx retry + connection pooling), then M3 (atomic cache write + advisory lock), then the doc LOWs + sweep pagination, each committed and verified independently.

A second review pass caught issues partly introduced by the first round of fixes — most importantly that the new retry fired on timeout/5xx for non-idempotent writes, which could double-execute a createScanTask/createPackage if a timeout landed after a server-side commit. Made retry idempotency-aware (429 and pre-send connect always retried; ambiguous timeout/5xx retried only for get*/list* reads), finished the _require_oid rollout across all destructive + required-id handlers, made the write-through cache best-effort, added a pagination ceiling, extended the raw denylist (createInstallTask), and wrapped non-JSON 200s.

A third pass found the urllib fallback misclassifying a post-send reset as the always-safe connect code, a negative Retry-After crash, and three more unvalidated company/parent ids (sweep/install-links/company-create); all fixed. A fourth (convergence) audit returned CONVERGED: yes with only a read-only companies single-page listing LOW and an unused-import NIT, both then fixed. A fifth/final confirmation pass returned CONVERGED: yes — no MEDIUM+ defects. The skill's own live selftest grew from 75 to 81 cases and ends green; new behavior is unit-tested offline (retry idempotency, Retry-After clamp/ceiling, urllib reset classification, atomic cache write + lock, context manager).

Eight descriptive commits landed on origin/main over the session (d8f0974 -> befd265); a few were absorbed by the background auto-sync into generic commits (recurring friction), but no change was lost.

Key Decisions

  • Validation-before-gate ordering in handlers (bad id -> rc2 before the --confirm gate -> rc3). Fail fast on malformed input; never describe/perform an action for an invalid target. Required updating selftest gate-refusal cases to use valid-FORMAT ids.
  • Retry only safe failure classes for writes. 429 (pre-processing rate-limit reject) and connect (pre-send) have no side effect -> always retried. Timeout/5xx are ambiguous (may have committed server-side) -> retried only for idempotent get*/list* methods. This preserved the 429-sweep fix while removing the double-execute hazard.
  • Did NOT validate ids of unpinned format (package id, report id, hashItemId, quarantine item id, incident type). Their formats aren't confirmed 24-hex, so validating would risk rejecting valid input; they are --confirm-gated and bad ids match the expected-error markers (no mislog). Deliberate, not an oversight.
  • raw denylist kept as best-effort (substring denylist on a power-user passthrough) rather than flipped to a read-method allowlist — minimal change, plus reworded docstring/help to say "verify any raw method yourself; not exhaustive."
  • Surgical edits + the existing selftest as the regression gate, rather than regenerating files. The live selftest (read paths + gating + validation against the real tenant) is the definitive "everything works" check.
  • Looped to convergence per the user's instruction: kept running fresh review passes until two consecutive passes found no MEDIUM+ defect, fixing only real/defensible items and consciously leaving documented LOW/NITs.

Problems Encountered

  • Retry double-execution risk introduced by my own H2 fix — the broad timeout/5xx retry would re-run non-idempotent writes. Caught on the second review pass; fixed with idempotency-aware retry (Fix A) + unit tests proving writes don't retry on timeout/5xx but do on 429/connect.
  • Selftest expectation drift after behavior changes — moving gate messages to stderr and adding client-side id validation changed several exit codes/streams (rc1->rc2 for bad ids; "Would" stdout->stderr; empty-set-json now rc2). Updated the affected assertions and added new bad-id/connect/Retry-After cases each round; ended 81/81.
  • Dummy-key test polluted errorlog.md — a --confirm smoke test with a dummy key hit a real 401 that was logged. Removed the artifact line; the API-key-error is otherwise correctly handled.
  • Background auto-sync interleaving — repeatedly swept in-progress edits into generic "auto-sync" commits and once blocked a rebase via another session's unstaged files. Worked around by stashing the other session's changes, rebasing, pushing, and popping; no work lost, but several descriptive commit messages were absorbed.

Configuration Changes

All under .claude/skills/bitdefender/:

  • scripts/gz.py — gated move/scan/create-package/make-group; _require_oid rollout across endpoint/policy/endpoints/sweep/install-links/company(+suspend/activate/delete/ create)/account(+update/delete)/assign-policy/set-label/reconfigure/delete-endpoint/ delete-group/isolate/unisolate/blocklist-add/quarantine/incidents; extended DESTRUCTIVE_RAW_PATTERNS (move*/createscan/createpackage/createcustomgroup/ createinstall); gate messages -> stderr; incident-status/note empty-set-json check; push-test parse-before-gate; companies lists full fleet; docstring/help fixes; removed dead _require_company_for_sweep.
  • scripts/gz_client.py — 429/5xx/timeout/connect retry with idempotency gating + backoff honoring Retry-After (clamped [0,120]); single reused httpx client + close()
    • context manager; atomic _write_cache (temp+fsync+os.replace) + _cache_lock advisory lock; best-effort write-through; pagination by short-page + MAX_PAGINATION_PAGES ceiling; list_all_companies(); non-JSON-200 -> GravityZoneError; urllib URLError conservative connect/timeout classification; removed unused field import.
  • scripts/selftest.py — VID valid-format placeholder; gate-refusal cases use VID; bad-id/sweep/install-links rc2 assertions; stderr "Would" assertions; incident empty-json assertion. 81 cases.
  • SKILL.md — gating list adds move/scan/create-package/make-group; delete-package --id <packageId> (was --package <name>); cache "no PII" wording corrected.
  • references/api-reference.md — deletePackage param corrected to packageId.
  • errorlog.md — removed a dummy-key 401 test artifact.

Credentials & Secrets

None created, rotated, or newly discovered. The GravityZone API key is loaded from the SOPS vault (msp-tools/gravityzone.sops.yaml field credentials.api_key) via vault.sh; never hardcoded, logged, or cached. Verified intact across all changes.

Infrastructure & Servers

GravityZone Cloud Public API: https://cloud.gravityzone.bitdefender.com/api/v1.0/jsonrpc (HTTP Basic, key as username/empty password). Live ACG partner tenant. ACG internal company id 5c428b246c031893678b4569; companies container 5c4280716c0318f3478b456e; root company 5c4280716c0318f3478b456a. No infrastructure changed.

Commands & Outputs

  • python -m py_compile gz.py gz_client.py selftest.py — clean each round.
  • python selftest.py — read-only + refusal-path harness against the live tenant; ended 81/81 passed, 0 failed (sets GZ_SUPPRESS_ERRORLOG=1, no errorlog pollution).
  • python gz.py sweep --company 5c428b246c031893678b4569 — exit 0, swept 9 endpoints (verified refactored pagination end-to-end).
  • Offline unit checks: retry idempotency (write+timeout NOT retried, write+429/connect retried, read+timeout retried), _retry_delay clamp+ceiling, urllib reset classification, atomic-write/lock round-trip + stale-steal + temp-cleanup.

Pending / Incomplete Tasks

  • None for bitdefender — converged; two consecutive review passes clean.
  • Consciously left (documented, not defects): id validation deliberately omitted on unpinned-format ids (package/report/hashItemId/quarantine-item); socket.timeout is TimeoutError assumption (non-issue on the fleet's Python 3.12); raw denylist is best-effort by design.
  • Recurring friction worth addressing fleet-wide: background auto-sync interleaving manual commits (absorbs descriptive messages). Option: pause auto-sync during focused dev.

Reference Information

  • Skill: .claude/skills/bitdefender/ (SKILL.md, scripts/{gz.py,gz_client.py,selftest.py}, references/{api-reference.md,BUILDOUT.md}).
  • Commits (origin/main, claudetools): d8f0974 (gating+id-validation cluster), 51751e6 (H2 retry+pooling), 4d75083 (M3 atomic cache), 1852f75 (doc LOWs+ pagination), a96a15a (auto-sync swept second-pass hardening), 778e12d (companies pagination+connect-retry+Retry-After ceiling+ctx mgr), 3d6cb46 (urllib reset+ Retry-After clamp+sweep/install-links validation), befd265 (companies full fleet+ drop unused import).
  • Exit-code contract: 0 success, 1 API/runtime failure, 2 bad input/validation, 3 gated refusal, 130 KeyboardInterrupt.
Reference in New Issue View Git Blame Copy Permalink