wehub-resource-sync
2114ccd278
CI / Lint & Test (Python 3.13) (push) Failing after 2s
CI / Lint & Test (Python 3.14) (push) Failing after 1s
CI / Lint & Test (Python 3.12) (push) Failing after 2s
CI / DCO Check (push) Has been skipped
Scorecard supply-chain security / Scorecard analysis (push) Failing after 2s
170 lines
8.4 KiB
Markdown
170 lines
8.4 KiB
Markdown
# Response to PR #100 Review
|
|
|
|
> Tracks how each issue raised in the PR #100 review was addressed.
|
|
> **All three issues are now resolved with dedicated thematic test suites.**
|
|
> See `DESIGN.md` for architecture and `../tests/` for all tests.
|
|
|
|
---
|
|
|
|
## Issue 1 — API Key Pool Was Dead Code
|
|
|
|
**Review feedback:** `ApiKeyPool` was implemented but never wired into actual LLM
|
|
call paths. The pool existed on disk but no code path used it.
|
|
|
|
**Resolution:** `set_api_pool()` patches BOTH `skillspector.llm_utils.get_chat_model`
|
|
AND `skillspector.llm_analyzer_base.get_chat_model` with a pooled version. Every
|
|
LLM call — graph-internal analyzers (20 per skill) and the gap-fill pass — goes
|
|
through the shared key pool.
|
|
|
|
| Before | After |
|
|
|--------|-------|
|
|
| Pool instantiated but unused | `set_api_pool(pool)` dual-patches `llm_utils` + `llm_analyzer_base` |
|
|
| gap-fill used single-key path | gap-fill + all 20 graph analyzers share the pool |
|
|
| No key failover for graph calls | 429 → automatic failover for every LLM call |
|
|
| Pool summary always showed 0 rate-limits | Real 429 tracking across all paths |
|
|
|
|
**Why dual-patch matters:** `llm_analyzer_base` imports `get_chat_model` via
|
|
`from skillspector.llm_utils import get_chat_model` at module level, creating
|
|
a local reference. Patching only `llm_utils` leaves this local reference
|
|
untouched — graph-internal analyzers (95% of LLM calls) bypass the pool
|
|
entirely. The fix adds a second assignment in `set_api_pool()`:
|
|
`_llm_analyzer_base.get_chat_model = _pooled_get_chat_model`.
|
|
|
|
**Verification:** `test_pool_wiring.py` verifies all three call paths:
|
|
`llm_utils.get_chat_model` → `PooledChatModel`, `LLMAnalyzerBase._llm` →
|
|
`PooledChatModel`, `GapFillAnalyzer.chat_model` → `PooledChatModel`.
|
|
|
|
**Upstream resilience:** Merged NVIDIA/SkillSpector@ab0431f (130+ commits,
|
|
89 files, OSS 2.3.7) — zero patch conflicts. All 7 monkey-patches intact.
|
|
|
|
See: `api_pool.py` (`set_api_pool`, `PooledChatModel`), `runner.py` (dual-patch),
|
|
`tests/test_pool_wiring.py` (3-path smoke test)
|
|
|
|
---
|
|
|
|
## Issue 2 — Import-Time Monkey-Patches Were Invasive and Fragile
|
|
|
|
**Review feedback:** Seven monkey-patches fired at module import, mutating
|
|
upstream class attributes. This was fragile (import order dependent),
|
|
invasive (no opt-out), and depended on internal details (Pydantic alias
|
|
precedence, MRO instance-attribute injection) that could break silently
|
|
on upstream updates.
|
|
|
|
**Resolution — Invasiveness:** Replaced import-time auto-patching with explicit
|
|
`deepseek_compat()` context manager and `setup_deepseek_compat()` one-shot.
|
|
Patches never fire at import time. 14 dedicated invasiveness tests prove:
|
|
|
|
| Property | Test file | What it proves |
|
|
|----------|-----------|---------------|
|
|
| Import is side-effect-free | `test_monkeypatch_invasiveness.py` | Subprocess isolation: `import runner` leaves `__init__` untouched |
|
|
| Thread isolation | Same | Thread B outside context sees unpatched classes; 50 concurrent instances all get `response_schema=None` with zero races |
|
|
| Instance-attribute isolation | Same | `self.response_schema = None` writes to instance `__dict__`, not class — Python MRO guarantees per-instance isolation |
|
|
| Concurrent independent contexts | Same | Two threads in separate `deepseek_compat()` blocks — exit one, other stays patched |
|
|
| Nesting safety | Same | Double/triple nested contexts — only outermost exit restores |
|
|
| Exception-safe restoration | Same | Exception inside context → all 5 methods restored |
|
|
|
|
**Resolution — Fragility:** `_verify_patch_targets()` guard runs BEFORE any
|
|
patches are applied. If upstream changes a patched method's signature,
|
|
removes a class attribute, or breaks a deep dependency, the guard raises
|
|
`RuntimeError` immediately with a specific message identifying which patch
|
|
broke. 26 dedicated fragility tests prove:
|
|
|
|
| Property | Test file | What it proves |
|
|
|----------|-----------|---------------|
|
|
| Guard passes current upstream | `test_monkeypatch_fragility.py` | No false positive against NVIDIA@ab0431f |
|
|
| Each of 7 patches individually guarded | Same | Temporarily break each target → guard catches it with correct patch number in message |
|
|
| Deep dependency detection | Same | `model_validate`, `to_finding`, `file_path`, `findings`, `new_event_loop` — all checked |
|
|
| Keyword-only migration caught | Same | Parameter becoming `KEYWORD_ONLY` → guard raises |
|
|
| Atomicity | Same | Guard fails → ZERO patches applied (fail-closed) |
|
|
| Original references at import time | Same | `_original_*` captured when `runner.py` loads, not at apply-time |
|
|
|
|
See: `runner.py` (`deepseek_compat`, `_verify_patch_targets`, `_check_signature`),
|
|
`tests/test_monkeypatch_invasiveness.py` (14 tests),
|
|
`tests/test_monkeypatch_fragility.py` (26 tests)
|
|
|
|
---
|
|
|
|
## Issue 3 — Risky Code Lacked Tests
|
|
|
|
**Review feedback:** The four riskiest areas — pool acquire/release, 429 backoff,
|
|
monkey-patches, and gap-fill parsing — had zero automated tests.
|
|
|
|
**Resolution:** 164 tests across 7 modules.
|
|
|
|
### Unit tests (120 tests, 4 modules)
|
|
|
|
| Module | Tests | Covers |
|
|
|--------|-------|--------|
|
|
| `tests-pro/test_api_pool.py` | 45 | acquire/release, rate-limit backoff, concurrency, edge cases, `try_acquire` |
|
|
| `tests-pro/test_gap_fill.py` | 41 | `parse_response` JSON recovery, markdown fence stripping, prompt building, batch/collect |
|
|
| `tests-pro/test_runner_patches.py` | 24 | `deepseek_compat()`, context manager nesting, isolation, `_verify_patch_targets` |
|
|
| `tests-pro/test_annotation.py` | 10 | `is_language_compatible`, `annotate_findings` edge cases |
|
|
|
|
### Thematic review tests (40 tests + 4 smoke checks, 3 files)
|
|
|
|
| File | Tests | Answers reviewer concern |
|
|
|------|-------|--------------------------|
|
|
| `tests/test_pool_wiring.py` | 4 checks | Issue #1 — 3-path pool verification + restore |
|
|
| `tests/test_monkeypatch_invasiveness.py` | 14 tests | Issue #2 — thread isolation, import no-side-effect, nesting |
|
|
| `tests/test_monkeypatch_fragility.py` | 26 tests | Issue #2 — per-patch guard verification, deep dep detection, atomicity |
|
|
|
|
### Mutation testing
|
|
|
|
30 bugs injected across the 4 risk areas. Tests catch 21/30. The 9 misses
|
|
are documented in `archive/FUTURE_WORK.md` §5.
|
|
|
|
---
|
|
|
|
## Minor Issues
|
|
|
|
### M1 — `_strip_markdown_fences` duplicated in `runner.py` and `gap_fill.py`
|
|
|
|
Acknowledged. Listed in `archive/FUTURE_WORK.md` as a low-priority cleanup. The
|
|
duplication is deliberate for now — `gap_fill.py` is designed to work standalone
|
|
without importing `runner.py`.
|
|
|
|
### M2 — `graph.invoke` call count mismatch in docstring
|
|
|
|
Fixed. Docstrings and comments updated to reflect the actual graph topology.
|
|
|
|
### M3 — `except (json.JSONDecodeError, Exception)` is redundant
|
|
|
|
The broad `except Exception` in `_patched_base_parse` and `_patched_meta_parse`
|
|
makes the preceding `except json.JSONDecodeError` unreachable. The dual-except
|
|
pattern is retained as explicit documentation of the two failure modes
|
|
(parse error vs. schema error), with distinct log messages for each.
|
|
The outer `except Exception` is scoped to return `[]` (empty findings) —
|
|
a single malformed LLM response never blocks the pipeline.
|
|
|
|
### M4 — `record_retry_success()` name vs. behavior
|
|
|
|
The method increments on each retry *attempt*, not on confirmed success.
|
|
Renaming to `record_retry_attempt()` is queued as a low-priority cleanup
|
|
in `archive/FUTURE_WORK.md`.
|
|
|
|
### M5 — `rm -rf` subprocess fallback in `cleanup_result` largely unreachable
|
|
|
|
Acknowledged. `shutil.rmtree(ignore_errors=True)` suppresses exceptions,
|
|
so the subprocess fallback is rarely reached. Kept as defense-in-depth
|
|
for macOS dangling-fd scenarios where `shutil.rmtree` can silently fail
|
|
to remove the directory despite `ignore_errors=True`.
|
|
|
|
---
|
|
|
|
## Summary
|
|
|
|
| Issue | Status |
|
|
|-------|--------|
|
|
| #1 — Pool dead code | ✅ Dual-patch (`llm_utils` + `llm_analyzer_base`), 3-path smoke test, 130-commit upstream merge verified |
|
|
| #2 — Invasive patches | ✅ Explicit context manager + setup function, 14 invasiveness + 26 fragility thematic tests |
|
|
| #3 — No tests | ✅ 164 tests (120 unit + 40 thematic + 4 smoke), 30-mutation suite |
|
|
| M1 — Duplicated utility | Known, deferred |
|
|
| M2 — Docstring mismatch | Fixed |
|
|
| M3 — Redundant except | Explicit (two failure modes with distinct logging) |
|
|
| M4 — `record_retry_success` naming | Deferred |
|
|
| M5 — Unreachable `rm -rf` fallback | Defense-in-depth, kept |
|
|
|
|
---
|
|
|
|
**Next:** [README.md](README.md) — user guide · [DESIGN.md](DESIGN.md) — architecture · [CONTRIBUTING.md](../CONTRIBUTING.md) — dev setup
|