fix: dedupe insightsService's transport onto the shared aiProvider one

[Bryandero98]

Summary

• insightsService.js defined a second callProviderAISimple, behaviorally divergent from the canonical server/services/aiProvider.js one: it classified a non-JSON/blank 200 body as an error (correct) but a valid 200 body with empty content as a successful { text: '' } — the opposite of aiProvider's classification, which treats a whitespace-only completion as a provider error.
• Both generateThemeAnalysis and refreshCrossDomainNarrative persist their result unconditionally once result.error is unset, so the divergent copy could overwrite a real cached narrative/theme with nothing on an otherwise-valid-but-content-empty response.
• Deletes the duplicate and routes both entry points through the shared aiProvider.callProviderAISimple — same call shape, drop-in compatible (also incidentally fixes a smaller divergence: the deleted copy called ensureOllamaProviderReady for any API-type provider, not just Ollama-backed ones, unlike the shared implementation's isOllamaProvider gate).
• Moves the transport/malformed-body/SSRF-guard test matrix onto aiProvider.test.js (where the shared implementation now lives, and where its whitespace-only-completion test already lived). insightsService.test.js keeps only the disk-only-read-path contract, matching its own docstring's stated scope.

Test plan

• npx vitest run services/insightsService.test.js services/aiProvider.test.js — 24 tests pass
• Confirmed no other file destructure-imports the deleted callProviderAISimple export from insightsService.js (the route file uses import * as insightsService, unaffected)
• Repo-wide grep for any other file importing from insightsService.js — only the route file and its own test

Fixes #5617

[Bryandero98]

Appreciate the detailed writeup on this one — pinning the exact behavioral divergence (line-referenced, both sides) made it straightforward to confirm which classification was canonical instead of guessing. Let me know if you'd rather see the moved test coverage organized differently.

[atomantic]

Reviewed by /do:review (security-focused pass, external contributor) — 0 critical, 2 improvements. This one is good; rebase and it's mergeable. Both findings are non-blocking and we're happy to take them in a follow-up if you'd rather not spin another round.

Rebase required. The branch is 188 commits behind main and currently CONFLICTING. In this repo a conflicted PR gets no CI at all ("no checks reported"), so nothing here has been pipeline-verified. The single conflict is an append-at-EOF collision in aiProvider.test.js (main added a Codex-subscription block at the same spot) — keep both sides, no semantic overlap.

Verified locally (isolated worktree, rebased onto current main, conflict resolved by keeping both blocks):

• Full cd server && npm test → 1809 files / 37248 tests pass, exit 0.
• Transport parity checked field by field, since a dedupe is only safe if the survivor is equivalent:
  • Timeout: identical — both provider.timeout || 300000.
  • Endpoint guard: preserved exactly, and now enforced inside postChatCompletion, so the SSRF / key-exfiltration policy, the allowCustomEndpoint opt-in, and keyless local-LLM passthrough all behave the same. Your three new guard tests pin it at the new home.
  • Non-2xx: identical message shape (Provider returned {status}: {body}).
  • Ollama readiness: improved — the old copy called ensureProviderReady for every type === 'api' provider; the shared one scopes it to isOllamaProvider, which is what it always meant.
  • Empty-content 200: changed — see the inline comment; net improvement, but it needs to be in the description.
• Security scan: pure ASCII apart from em-dashes, no network calls, no process.env reads, no eval/exec/child_process, no dependency or CI changes, no new secrets path. Single commit, single author. Net -59 lines and one fewer copy of a keyed-request transport, which is a genuine security win on its own — one place to audit instead of two.

The two notes:

1. The empty-completion verdict flips from { text: '' } success to { error }. Good change (it's what finally stops an empty narrative overwriting the cache) and already pinned on main, but the PR reads as a pure dedupe and doesn't mention it.
2. The read-path no-cold-bootstrap tests drop from a fetchWithTimeout-level assertion to a callProviderAISimple-level one, which a future non-callProviderAISimple path would slip past.

Coherence check: description matches the diff otherwise, and the test plan holds up — I re-ran it. Deleting the duplicated transport rather than wrapping it is the right call, and moving its contract tests to live beside the surviving implementation is exactly where they belong.

Generated by /do:review

[Bryandero98]

Rebased onto current `main` (single conflict, exactly as flagged — an append-at-EOF collision in `aiProvider.test.js` where `main`'s new Codex-subscription test block and this PR's malformed-response test block both landed at the same spot). Kept both in full: worth noting for anyone re-resolving a similar case, git's merge deduplicated the closing `});`/`});` lines it judged identical between the two sides and left them out of the conflict markers entirely — had to manually restore them after the auto-resolve to close `main`'s own `describe` block, or the file would have silently mis-nested the two blocks into one.

Re: the empty-completion-verdict note — already called out explicitly in this PR's own description (first bullet: "a valid 200 body with empty content..."), so no separate edit needed there.

Verified: `npx vitest run services/aiProvider.test.js services/insightsService.test.js` — 36/36 pass (up from the 24 in the original test plan, since `main` picked up new Codex-transport tests in the meantime).

🤖 Generated with Claude Code

[atomantic]

Second /do:review pass (security-focused, external contributor) — 0 findings. Approving and merging. Both threads from the first round are answered and resolved.

What changed since round one: rebased onto current main, conflict resolved, and CI is now actually running — CI Gate pass, Server tests pass, Plan test impact pass. mergeStateStatus: CLEAN.

Re-verified independently on 4cc7525:

• Rebase resolution is correct. aiProvider.test.js at HEAD is byte-identical to main's copy plus a pure append of the two new describe blocks — no mis-nesting, no swallowed brace, main's Codex-subscription block intact. This was the one thing worth checking twice, since a folded describe would have silently changed hook scoping while staying green.
• Production diff is a pure deletion. insightsService.js loses only the duplicate function and its now-unused imports/constant; grepped the file for every removed symbol (fetchWithTimeout, readResponseJson, ensureOllamaProviderReady, evaluateSecretEndpoint, DEFAULT_AI_TIMEOUT_MS) — zero residual references. Only server/routes/insights.js consumes this module, via import * as, so the dropped named export breaks nothing.
• Signature parity holds. Shared callProviderAISimple(provider, model, prompt, options) carries the same temperature = 0.3 / max_tokens = 1000 defaults and the same provider.timeout || 300000; both call sites pass their own overrides and gate on result.error before atomicWrite, so the cache-overwrite path #5617 describes is closed.
• Security scan, adversarial pass: no non-ASCII beyond em-dashes, no bidi or zero-width characters, no new process.env / child_process / eval / network calls, no dependency, lockfile, or CI changes. Net −59 lines and one fewer copy of a keyed-request transport — one place to audit instead of two, which is the real security win here.
• The SSRF / key-exfiltration guard is preserved and now better covered. It lives in postChatCompletion, and the three appended guard tests pin all three arms at the surviving implementation: blocked-when-keyed, allowed under allowCustomEndpoint, and keyless-passthrough. None of them weakens a guard — they pin behavior that was previously asserted only against the deleted copy, so the consolidation would otherwise have lost that coverage.
• New transport tests are complementary, not duplicates — main already covered whitespace-only and the happy path; this adds non-JSON body, blank body, and non-2xx.

Against the ticket (#5617): all five acceptance criteria met. The one soft spot is AC 3 — that an errored transport leaves themes.json / narrative.json untouched is proven transitively (transport errors + if (result.error) return) rather than by a test at the Insights boundary. That's pre-existing, not a regression, and we're taking it in our own follow-up along with the read-path assertion altitude.

Behavior deltas worth recording, all fallout of adopting the canonical transport rather than choices this PR made:

1. An empty/whitespace-only completion flips from { text: '' } success to { error } — stated in the description and commit body, and it is what finally stops an empty narrative overwriting the cache.
2. Non-API rejection message text changes (Insights analysis requires an API-based provider → This operation requires an API-based provider).
3. Ollama readiness is now scoped by isOllamaProvider instead of firing for every type === 'api' provider — the old copy's behavior was a bug.
4. Insights can now dispatch to a Codex subscription-text provider, which it previously refused outright.

Thanks — deleting the duplicate rather than wrapping it, and relocating its contract tests to sit beside the surviving implementation, is exactly the right shape for this.

[atomantic]

Merged as 7bc373f — thanks, this was a clean one. Issue #5617 closed automatically.

The read-path assertion altitude I raised in the second thread is now our follow-up: #5845. It restores the wire-level fetch spy alongside the export-level one, and adds the test neither file had — that an errored transport leaves themes.json / narrative.json untouched, which is the actual bug #5617 describes and was previously only proven transitively through the if (result.error) return guard.