Add timeout protection to narrative analysis methods

[ssevera1]

What

Add explicit timeout parameter and validation to executive_summary, prioritized_actions, and systemic_issues LLM calls.

Why

These methods previously lacked the retry/timeout protection applied to finding and zero-day analysis, exposing them to hanging requests.

[claude]

Thanks for the patch, but I think the premise behind this change is inverted — as written it reduces timeout protection rather than adding it.

1. The stated gap does not exist (analyzer.py:238, :427, :453)

The description says these three methods "previously lacked the retry/timeout protection applied to finding and zero-day analysis." That isn't the case. Every analyzer call funnels through _query → LLMClient.query → LLMClient._http_post, and client.py:245 does:

timeout = self._REQUEST_TIMEOUT if timeout is None else timeout
attempts = self._MAX_RETRIES if max_retries is None else attempts

So passing timeout=None already applied _REQUEST_TIMEOUT = 120 (client.py:66) and 3 retries. And the two paths cited as the good example — _analyze_findings_batch (analyzer.py:277) and the zero-day call (analyzer.py:388) — also pass no explicit timeout. All five call sites were already treated identically; there was no asymmetry to close.

2. The real effect is a behavioral regression: 120s → 60s on the longest calls

Since the default was already 120s, _NARRATIVE_TIMEOUT = 60 (analyzer.py:21-22) halves the budget for exactly the three requests most likely to need it: the executive summary asks for 3-5 paragraphs, the action plan and systemic-issues prompts are similarly free-form, and all run at max_tokens=4096. These are the slowest generations in the codebase, and they're now the only ones on a short leash.

Because _section (analyzer.py:189-197) catches LLMError and degrades, the user-visible failure mode is silent: on a slow-but-healthy provider the report renders [Analysis unavailable: the LLM request for the executive summary failed - ...] where prose used to be, and the scan still exits successfully. If the goal is protection against hanging requests, a value below the existing default doesn't provide it — it just converts slow successes into dropped report sections.

3. Contradicts a documented design decision

tests/test_llm_analyzer.py:175 has the section header "retry and timeout must be owned by the client alone", with test_analyzer_uses_the_client_request_timeout asserting the analyzer defers to LLMClient._REQUEST_TIMEOUT. That test only covers _analyze_findings_batch so it should still pass, but this change moves timeout policy into the analyzer for three of five call sites — the exact split that comment exists to prevent. Worth reconciling with that intent (or updating it deliberately) rather than working around it.

4. Out-of-scope change that introduces an error (analyzer.py:43)

The diff drops the sentence-ending period from the _finding_index docstring:

-    Models sometimes emit the index as a string (``"finding_index": "1"``).
+    Models sometimes emit the index as a string (``"finding_index": "1"``)

This is unrelated to timeouts and leaves a run-on with the following sentence. Please revert it.

5. Description mentions validation that isn't here

"Add explicit timeout parameter and validation" — no validation is added in the diff. Either drop it from the description or add it.

Suggested path forward: if the concern is genuinely that 120s is too long to hang, that's a client-level tuning question — change _REQUEST_TIMEOUT (or make it configurable) so all call sites stay consistent. If narrative sections specifically warrant a different budget, they most likely need a longer one than the batch calls, not shorter, and it should come with a test pinning the value and a note on why they differ. The timeout plumbing added to _query (analyzer.py:172, :181) is fine and worth keeping either way.

[claude]

Verified all five points against the code and agree with the review in full — the PR's premise was inverted.

Confirmed and fixed:

1. No gap existed: client.py:245-246 shows timeout=None already resolves to _REQUEST_TIMEOUT = 120 with 3 retries, so all five _query call sites had identical protection before this PR.
2. _NARRATIVE_TIMEOUT = 60 is below that 120s default and was applied to exactly the three most free-form, max_tokens=4096 generations — a regression that turns slow-but-healthy responses into silently dropped report sections (_section catches LLMError and degrades), not added protection.
3. tests/test_llm_analyzer.py already documents "retry and timeout must be owned by the client alone" as a deliberate boundary this PR crossed.
4. The docstring period removal at _finding_index was out of scope and left a run-on sentence.
5. The description's "validation" claim didn't match the diff.

Fix: reverted vulntester/llm/analyzer.py to match origin/main exactly (removed _NARRATIVE_TIMEOUT, the timeout param on _query, and the three call sites passing it; restored the docstring period). Added test_narrative_sections_use_the_client_request_timeout in tests/test_llm_analyzer.py, which asserts all calls made during analyze_report use LLMClient._REQUEST_TIMEOUT. Confirmed it fails against the reviewed diff ({60, 120} vs expected {120}) and passes after the revert.

Nothing to push back on — every point held up against the code.

Checks: pytest -q → 81 passed (no mypy step in this repo's CI).

[claude]

The code is fine — the problem is that the PR title and description no longer describe what this PR does.

The net diff contains no source change at all. 1e4f67b added _NARRATIVE_TIMEOUT = 60 and threaded a timeout kwarg through LLMAnalyzer._query; 8aa50ca reverted it. git diff origin/main..HEAD -- vulntester/ is empty. The entire surviving change is tests/test_llm_analyzer.py:206-220, a pinning test asserting the opposite of the title: that the narrative sections keep the client's 120s default rather than getting a shorter budget.

I agree with the revert. LLMClient._REQUEST_TIMEOUT = 120 already applied to these calls via client.py:245 (timeout = self._REQUEST_TIMEOUT if timeout is None else timeout), so _NARRATIVE_TIMEOUT = 60 shortened the budget on the most free-form generations rather than adding protection. The test is also correct as written — client.py:254 always passes timeout= as a keyword, so call.kwargs["timeout"] is safe, and analyze_report(_report()) issues 4 requests, so the set can't be vacuously empty.

Two things to fix before merge:

1. Retitle and rewrite the description (blocking). As-is a squash merge lands a commit in main titled "Add timeout protection to narrative analysis methods" whose entire content is a test asserting that no such timeout was added. The body's claim that these methods "previously lacked the retry/timeout protection applied to finding and zero-day analysis" is also incorrect — they went through the same _query -> client.query -> _http_post path with the same 120s timeout and the same _MAX_RETRIES retry loop. Something like "test: pin narrative sections to the client request timeout" matches the content. The title also mentions "validation", which neither commit ever added.

2. Narrow the test's scope (nit, tests/test_llm_analyzer.py:219). urlopen.call_args_list covers every request analyze_report makes, including the _analyze_findings_batch call, not just the three narrative sections the test is named for. That makes it a strictly broader assertion than the docstring claims, so a future per-call-site timeout anywhere in analyze_report would fail a test named test_narrative_sections_... with a confusing message. Asserting only over the narrative calls — or renaming to something like test_analyze_report_uses_one_timeout_everywhere — would keep the failure mode legible.

[github-actions]

Auto-fix budget exhausted after 1 rounds — not attempting another pass.

Two bots disagreeing past this point usually means the proposal needs a human call, or is not worth the churn. Leaving the PR as-is; pr-janitor.yml will close it if it goes stale.