fix: report incomplete batch gap-fill analysis

[yashrajp22]

Malformed responses, setup errors, provider failures, or exhausted runtime in gap-fill analysis could become an empty result marked as applied. Gap-fill now requires an explicit findings field, retries invalid structured responses, records failed work in the inspection ledger, and keeps both core findings and successful gap-fill findings. Constructor and batching errors also record each planned file.

Gap-fill and the provider pool use the remaining per-skill deadline, including retries, with time reserved for reporting. Incomplete results return batch exit 2 and expose gap_fill_status, gap_fill_error, and reason codes under enhancements; they remain included in report details and incomplete counts. GapFillError exposes retained findings directly.

Validation: 542 source tests and 571 installed-wheel-core tests passed, plus 29 contributor parser tests with 10 subtests. Four added regressions reproduce failures against the original source and wheel core. Cases include actual deadline expiry, setup failures, malformed and valid empty responses, partial success, preserved findings, and report visibility. Contributor tools are not packaged in the wheel. Tests use deterministic fake providers; these focused tests made no live provider calls.

Combined verification across the updated PRs: 6,243 regression tests passed against source and again against the freshly installed wheel, with seven conditional skips and four expected failures per run. All 19 source/wheel sample pairs matched. The 12-skill corpus retained its findings and risk ratings; four former hangs now finish with explicit partial-analysis results. The 93 extension tests passed. Two synthetic live NVIDIA Build checks passed on the final wheel: benign-note was complete/SAFE, and the exfiltration sample retained SSD-3 with complete semantic and meta analysis and a DO_NOT_INSTALL recommendation. All seven recorded LLM analyses succeeded. Live checks used the configured model/reasoning defaults through a test-only proxy that kept the real credential outside the scanner.

[rng1995]

[SkillSpector Review]

Hi @yashrajp22, thank you for routing the batch scanner's gap-fill pass through the core batch executor! A failed batch is now retried under the core policy, recorded in the inspection ledger and kept out of gap_fill_applied. Completeness is recomputed with finalize_ledger rather than by editing counters by hand, and the new integration test drives the real retry loop, ledger and report formatters.

Value and readiness: The problem is real on main. I ran main's own run_gap_fill and run_one with malformed JSON, {"findings": 1} and a provider RuntimeError. In each case run_gap_fill returned [] after one call per file with no retry, and run_one returned error=None with gap_fill_applied=True, is_complete=True, execution_successful=True and recommendation SAFE. I then transcribed the PR's gap_fill.py:218-306 and runner.py:778-802 (nothing was imported from the PR) and ran them on main's core, with and without deepseek_compat. Every failure inside a batch is now handled as described:

• Malformed, schema-invalid and BOM-prefixed JSON, and plain-text refusals, get 4 attempts and are then recorded as llm_structured_response_invalid.
• Provider errors are recorded as llm_batch_failed.
• In every failure mode, gap_fill_applied=False, is_complete=False, SAFE becomes CAUTION, an error is returned and the command exits 2.
• Core findings and findings from successful batches are kept.
• Valid empty responses still succeed after one call.

Two gaps remain against the PR's stated outcome ("Stops failed gap-fill analysis from being marked as successfully applied. Preserves existing findings and reports the failure."), and both block merge:

• A gap-fill failure before batching, such as the pool-mode TypeError, now replaces the entry with an ERROR stub and drops the core findings that main kept (finding 1).
• A JSON object without a findings key, such as {"error": ...} or {}, is still accepted as a successful, complete pass (finding 2).

The other findings are non-blocking. Once findings 1 and 2 are fixed and CI has run, the PR is ready for final maintainer review. The merge also needs sequencing with #796.

Material findings

1. [Blocker] contrib/batch_scan/runner.py:804: a gap-fill failure before batching now replaces the whole entry with an ERROR stub, which drops the core findings.
   • The broad except Exception: return [] is gone, so GapFillAnalyzer(...) and get_batches (gap_fill.py:301-302) now raise freely. run_one catches only GapFillError (runner.py:782), so any other exception reaches runner.py:804-805. entry_from_error (runner.py:811-838) then returns issues=[], score 0 and severity/recommendation ERROR.
   • Concrete trigger, which is pool mode:
     • create_api_key_pool_from_env returns a pool whenever at least 2 keys are configured, from SKILLSPECTOR_API_KEYS or from OPENAI_API_KEY plus OPENAI_API_KEY_2..9 (api_pool.py:603-664).
     • batch_scan.py:195-197 then calls set_api_pool, which installs _pooled_get_chat_model(model=None) (runner.py:89-98).
     • LLMAnalyzerBase.__init__ always calls get_chat_model(model=model, timeout=...) (llm_analyzer_base.py:938), so constructing GapFillAnalyzer raises TypeError: ... unexpected keyword argument 'timeout'.
     • This affects every non-English skill with use_llm and a non-empty provider-eligible cache.
   • I reproduced this with main's code, a stub pool and deepseek_compat, on a Chinese skill using the real graph:
     • main's run_one kept score 68, HIGH, DO NOT INSTALL and issues E1/PE3/SC2/TM2. It also reported gap_fill_applied=True, which was the old silent bug.
     • The transcribed PR run_one returned the TypeError text with {score 0, severity ERROR, recommendation ERROR} and issues: [].
     • The graph itself completes in pool mode. Core LLM analyzers already fail on main but are caught, so main's entry was already is_complete=False. The core findings are what this PR loses.
   • Scope: ValueError was already re-raised on main, so a missing-credential failure was an ERROR stub before this PR as well. The types whose behaviour changed are TypeError and other construction or batching errors, and NotImplementedError from run_batches_detailed.
   • Consequence: this fails closed (ERROR, exit 2), so it is not a bypass. But it contradicts "Preserves existing findings and reports the failure" for failures outside run_batches_detailed. The new test mocks get_chat_model with lambda **kw, so the test does not exercise this path.
   • Expected fix:
     • Treat gap-fill exceptions separately from graph exceptions. The simplest way is for run_gap_fill to wrap construction and get_batches failures in a GapFillError whose outcome marks every planned file as failed. The existing branch at runner.py:790-802 then keeps the core entry, marks it incomplete and returns the error.
     • Decide whether ValueError (configuration) should stay a hard error.
     • Add a test in which GapFillAnalyzer construction raises (for example, a get_chat_model that rejects timeout), and assert that TM1 survives.
     • The pool-mode timeout mismatch in _pooled_get_chat_model and compat Patch 1 is a separate bug on main and belongs in its own change.
2. [Blocker] contrib/batch_scan/gap_fill.py:260: a JSON object without a findings key is still accepted as a successful, complete pass.
   • GapFillResult.findings defaults to an empty list (gap_fill.py:94), so GapFillResult.model_validate(data) accepts any dict that lacks the key. run_gap_fill then raises nothing, and runner.py:788 sets gap_fill_applied=True. The PR pins this behaviour in contrib/batch_scan/tests/tests-pro/test_gap_fill.py:226-227.
   • Transcription on main's core: {"error": "I cannot help with that"}, {"refusal": "..."}, {} and a fenced {"error": ...} each gave 1 call, error=None, gap_fill_applied=True and 0 findings. A plain-text refusal and {"findings": null} correctly gave 4 calls and llm_structured_response_invalid.
   • This is not a regression. main returned [] for these inputs as well, and core's LLMAnalysisResult uses the same default (llm_analyzer_base.py:644). The gap is narrow: gap-fill runs in raw chat mode, so a provider refusal usually arrives as prose, and that case is caught.
   • Consequence: a non-compliant response that happens to be valid JSON is still reported as "applied, no findings, complete". If the core scan said SAFE, the skill stays SAFE. That is the silent pass this PR sets out to remove.
   • Expected fix:
     • In _parse_json_response, raise _StructuredResponseValidationError when not isinstance(data, dict) or "findings" not in data, or validate against a private model in which findings is required. Leave GapFillResult's default unchanged.
     • Change test_missing_findings_key_keeps_schema_default to expect the raise, and add {"other": "value"} and {} to the invalid list.
     • Trade-off: a bare {} meaning "nothing found" would then retry and end incomplete. That is defensible because the prompt requires the key (gap_fill.py:147-160).
3. [Non-blocking] contrib/batch_scan/gap_fill.py:199: gap-fill has no deadline. The new structured retries add time inside the 90 s worker budget, and the runtime_limit path cannot occur in production.
   • super().__init__(base_prompt=prompt, model=resolved_model) passes no timeout, and compat Patch 1 _patched_base_init (runner.py:128-136) has no timeout parameter. So _require_time_remaining() returns None (llm_analyzer_base.py:986-991), LLMRuntimeLimitError is never raised, and RUNTIME_LIMIT is never recorded for gap-fill.
   • The test's timeout case raises LLMRuntimeLimitError("deadline") from the fake model (tests/test_batch_scan_security.py:188) and sets the retry delays to zero (:195). Both are synthetic.
   • Retry cost:
     • Each malformed batch now gets STRUCTURED_RESPONSE_MAX_ATTEMPTS=4 calls with 0.5/1.0/2.0 s backoff (llm_analyzer_base.py:79-87), where main made 1 call.
     • Batches run one after another, and a large file can span several batches.
     • Transcription with the real delays, 2 malformed files: at L=0.2 s, main took 0.41 s for 2 calls and the PR 8.68 s for 8 calls (7.08 s of it backoff). At L=1.0 s, it was 2.01 s against 15.07 s.
     • That matches N·(4L+3.5 s) against N·L. By arithmetic (not run), 2 batches at L=10 s is about 87 s against 20 s, before core-graph time.
   • The worker is killed at 90 s (batch_scan.py:204, :232-234), and the supervisor substitutes an ERROR stub with no issues.
   • The missing deadline predates this PR, since main already retried provider and rate-limit errors. What the PR adds is the malformed-output retry cost. Per-request HTTP timeouts and provider errors are correctly recorded as llm_batch_failed, with findings kept.
   • Consequence: a slow provider that keeps returning malformed output can push a skill past the worker kill, where it would otherwise be reported as incomplete with findings kept. Readers may also take the timeout test and the PR body's "runtime limit" wording as covering production timeouts, which they do not.
   • Expected fix:
     • Thread a monotonic deadline through run_one → run_gap_fill → GapFillAnalyzer, for example timeout=lambda: deadline - time.monotonic() - margin. A counterfactual transcription with a 3 s deadline at L=0.5 s stopped after 3 calls in 3.03 s and recorded runtime_limit for both batches.
     • This needs Patch 1 to forward timeout, which #796 adds.
     • If the fix is deferred, drop "runtime limit" from the PR body's list of handled failures, note the per-batch retry cost, and relabel the synthetic timeout test case.
4. [Non-blocking] contrib/batch_scan/runner.py:802: every gap-fill incompleteness exits 2, but the JSON shows no error for the non-fatal modes.
   • runner.py:802 returns str(gap_error) for every GapFillError. batch_scan.py:468-473 prints a per-skill ERROR, and :534-535 exits 2. The entry never gets an error key, and reports.py:118 counts Errors: only from r.get("error").
   • Transcription of the PR branch on a real main graph result, passed to main's reports._format_json/_format_terminal:
     • json and schema: execution_successful=True, no skills[].error, failed_executions=0, incomplete_skills=1, row (llm_structured_response_invalid, fatal=False), no Errors: line, exit 2.
     • timeout: the same, with (runtime_limit, fatal=False), exit 2.
     • provider: execution_successful=False, failed_executions=1, (llm_batch_failed, fatal=True), exit 2.
   • Comparison:
     • Core skillspector scan exits 2 on a result only when execution_successful is False (cli.py:858-859).
     • Inside batch_scan, main's run_one returns error=None for core LLM failures. I measured a malformed core output and a fatal core provider failure (failed_executions=1), and both exit 0.
     • #796 does not change run_one's error return.
   • The JSON is not completely silent: gap_fill_applied: false, incomplete_skills and ledger_exceptions are present. But gap_fill_applied: false is also set for an empty cache with no error.
   • Consequence: a CI job that gets exit 2 finds no per-skill error and failed_executions: 0 for two of the three modes, and the same reason code exits differently depending on whether gap-fill or a core analyzer produced it. This fails closed but is inconsistent.
   • Expected fix: pick one rule and document it.
     • Option (a): return an error only when completeness["execution_successful"] is False. To treat core and gap-fill fatal failures the same way, batch_scan.py would also count entry.get("execution_successful") is False, which is a gap on main outside this PR.
     • Option (b): keep exit 2 and add a machine-readable field such as enhancements.gap_fill_status/gap_fill_error with the reason codes.
     • Either way, update the exit-code table in contrib/batch_scan/docs/README.md:310-316 and coordinate with #796.
5. [Non-blocking] tests/test_batch_scan_security.py:133: the new test does not assert two of the behaviours the PR claims.
   • The parametrized test runs in CI and would fail on main in every case. With the test's mocks, main's run_one returns error=None and gap_fill_applied=True, so the assertion at :201 fails. The core claim is covered.
   • The assertions (:201-234) never read entry["risk_assessment"] or entry["execution_successful"], and batch_scan's exit code depends only on errors.
   • Mutation check (transcription): deleting runner.py:799-801, the execution_successful copy and the SAFE→CAUTION change, still passes every assertion in all 8 cases. The recommendation starts as SAFE in this test, so lines 800-801 run but their result is never checked.
   • There is no case where the failure happens before run_batches_detailed (finding 1), and the timeout case is synthetic (finding 3).
   • The edits to contrib/batch_scan/tests/tests-pro/test_gap_fill.py do not run in CI: pyproject.toml:117 sets testpaths = ["tests"] and Makefile:110 runs pytest ... tests/. That file pins a BOM-prefixed valid response as a failure (:218) and a missing findings key as a valid empty result (:226).
   • Expected fix:
     • Add assert entry["risk_assessment"]["recommendation"] == "CAUTION" and assert entry["execution_successful"] is (failure_mode != "provider").
     • Optionally assert failed_executions == int(failure_mode == "provider").
     • Add a construction-failure case.
     • Optionally move the parse-failure unit cases under tests/.
6. [Non-blocking] contrib/batch_scan/runner.py:759: docstrings and docs do not reflect the changed contracts.
   • runner.py:756-760 says that on failure entry is a stub error entry. For a gap-fill failure, runner.py:802 returns the full entry, recomputed and marked incomplete, together with an error.
   • run_gap_fill's docstring (gap_fill.py:276-297) still says it returns "A (possibly empty) list" and has no Raises section. It now raises GapFillError (:305), and construction errors propagate.
   • The section header at gap_fill.py:266 still says "Backward-compatible entry point".
   • The parse_response docstring (:219-224) does not mention the new raise.
   • run_gap_fill is public API (contrib/batch_scan/__init__.py:30, :67), but GapFillError, which a caller needs in order to read .outcome.successful, is not exported.
   • contrib/batch_scan/docs/README.md:205 says gap_fill_applied is true "if LLM gap-fill was used". On partial success it is now false while gap_fill_findings > 0.
   • contrib/batch_scan/tests/docs/TEST_GUIDE.md:122 still lists 9 TestParseResponseInvalidInput tests; there are now 2 (the file goes from 35 to 28 tests).
   • Expected fix:
     • Add Raises: GapFillError (with .outcome) to run_gap_fill, and update run_one's Returns section and the stale header.
     • Optionally export GapFillError.
     • Define gap_fill_applied in the README as "true only if every gap-fill batch completed".
     • Update the TEST_GUIDE row.
7. [Non-blocking] contrib/batch_scan/runner.py:784: minor duplication, and a branch reachable only from tests.
   • The comprehension at runner.py:784 repeats collect_findings (llm_analyzer_base.py:1512-1525). The runner has no analyzer instance to call it on.
   • The isinstance(response, GapFillResult) branch (gap_fill.py:225-226) cannot be reached in production. response_schema = None (:192), so _invoke_batch always passes a str. The branch exists only to keep TestParseResponsePydanticModel passing; that test's len >= 0 assertion predates this PR.
   • Expected fix (optional): expose the retained findings on GapFillError (for example a findings attribute) and use it at runner.py:784. Either tighten the Pydantic-path test to assert the returned rule IDs, or drop the branch and expect the raise.

PIC tradeoffs:

• Exit semantics. Exit 2 for any gap-fill incompleteness fails closed but is stricter than core, which exits 2 only on fatal execution failure. Within the same batch run, core-analyzer failures exit 0 (finding 4).
• Surfacing misconfiguration. Removing the broad except Exception exposes real misconfiguration that main hid, such as the pool-mode TypeError. Today the cost is losing that skill's core findings (finding 1).
• Retry cost. Retrying malformed gap-fill output follows core policy and recovers from transient bad output. It also multiplies calls and latency by up to 4 per batch, with no deadline (finding 3).
• BOM-prefixed JSON. A BOM-prefixed but otherwise valid response is now retried and counted as a failed batch, where main dropped it silently. This fails closed, and providers rarely emit a BOM. Stripping a leading U+FEFF in _parse_json_response would accept it instead.
• Empty object. Requiring the findings key (finding 2) means a bare {} is no longer accepted as "nothing found".
• Report shape. Gap-fill ledger rows appear in analysis_completeness only on failure. On success the gap-fill pass is not visible there, so the data shape differs between the two paths.
• Merge order with #796. #796 conflicts textually in contrib/batch_scan/runner.py (the skillspector.llm_analyzer_base import line), and both PRs edit tests/test_batch_scan_security.py. #796 also makes Patch 1 forward timeout, which the deadline fix in finding 3 needs.

Verification and gaps:

• Baseline on main: reproduced with main's own code, as described above. Each failure mode made one call, returned error=None, gap_fill_applied=True, is_complete=True and execution_successful=True, and kept SAFE.
• PR behaviour: checked through my own transcriptions of gap_fill.py:218-306 and runner.py:778-802, run on main's core with and without deepseek_compat.
  • A 200k-deep JSON nesting (RecursionError) gives llm_batch_failed, fatal.
  • Fenced valid JSON succeeds after 1 call.
  • _StructuredResponseValidationError is retried, and provider and runtime errors are not.
  • GapFillAnalyzer overrides parse_response, so compat Patch 2 does not apply to it.
• Completeness recompute: finalize_ledger on real graph results reproduced the graph's own analysis_completeness exactly, on 16 fixtures plus oversized, binary-referenced and >1 MB variants. The PR-style merge adds exactly one gap_fill exception row per failed batch, with no finding_accounting_error or unaccounted_work, including when a successful batch emits a finding.
  • Finding IDs are random (models.py:118-129), so gap findings cannot collide with core IDs.
  • The worker result stays JSON-serializable, because the reason and outcome codes are StrEnum.
• Tests:
  • test_gap_fill_failure_is_incomplete_and_preserves_findings fails on main in every case. So does the new empty-cache assertion at tests/test_batch_scan_security.py:258. Both run in CI through make test-ci.
  • The tests-pro changes do not run in CI.
  • tests/test_batch_scan_security.py passes ruff check and ruff format --check.
• CI: no checks have run. The workflow run on 46af7b5 is action_required (waiting for a maintainer), so the PR description's test counts are unverified, and CI must pass before merge.
• Conflicts: mergeable with main; the main commits merged since the review (#691, #577, #608) touch none of these files. It conflicts with open PR #796 in contrib/batch_scan/runner.py, and both edit tests/test_batch_scan_security.py in different places.
• Head update: I reviewed 46af7b553b1667402d52d8dc2996f806453b6191. The current head a8fb26641167df4b8eadf71213d928e84bc4e23e only adds automated merges of main (#691, #577, #608) from update-pr-branches.yml; those commits touch none of this PR's files (#691 only routes structured-output binding through an overridable method in llm_analyzer_base.py), so line references to llm_analyzer_base.py below are to the current head. The PR's own diff is unchanged (same patch-id), so this review applies to the current head.
• I did not run the PR's tests or code, per policy.
• Unverified areas:
  • No live provider was available. The recompute was checked against static and no-credential graph results, not against real LLM-analyzer and meta-analyzer ledger rows.
  • The 90 s worker kill in finding 3 is arithmetic from measured call counts and backoff, not an observed kill.
  • Pool mode was reproduced with a stub pool object.
  • Chunked files, where only some chunks of one file fail, were not exercised.
  • Local Python is 3.13; CI uses 3.12.

---

Decision: Changes Requested (reviewed head 46af7b553b1667402d52d8dc2996f806453b6191; current head a8fb26641167df4b8eadf71213d928e84bc4e23e only adds merges of main)