test: pin the insights no-cold-bootstrap contract at the wire, and the persist guard at its callers - #5845
Merged
Conversation
…e persist guard at its callers Follow-up to #5626 (review threads we chose not to hold that PR open for). Two gaps the transport dedupe left in insightsService.test.js: The read-path assertions moved from a fetchWithTimeout-level spy down to a callProviderAISimple-level one when the transport moved to aiProvider. That is one export, but the AGENTS.md contract is broader — "PortOS must never queue up AI provider calls a user hasn't knowingly triggered", by any route. A future read path reaching a provider through a direct fetch, a streaming helper, or a second transport would slip past the export-level spy while it stayed green. Restore a global fetch spy alongside it so the assertion sits back at the wire. Nothing anywhere pinned the actual bug #5617 describes: both generators persist unconditionally once result.error is unset, so a transport that misclassifies a failed call as a successful empty completion overwrites a real cached theme/narrative with nothing. That was only proven transitively — the shared transport returns an error, and you read the `if (result.error) return` guard at each call site. Assert it at the Insights boundary instead: an errored transport result surfaces as { available: false, reason } and leaves atomicWrite untouched, for both generateThemeAnalysis and refreshCrossDomainNarrative, plus the positive case so the guard can't pass by never writing at all. Verified the two guard tests are not vacuous: neutering both `if (result.error)` guards in insightsService.js fails exactly those two and nothing else.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #5626 — the two review threads we didn't hold that PR open for. Test-only; no production code changes.
fetchWithTimeout-level spy down to acallProviderAISimple-level one, because the transport moved toaiProvider. That's one export, but the AGENTS.md contract is broader — "PortOS must never queue up AI provider calls a user hasn't knowingly triggered", by any route. A future read path reaching a provider through a directfetch, a streaming helper, or a second transport would slip past the export-level spy while it stayed green. Restores a globalfetchspy alongside it.result.erroris unset, so a transport that misclassifies a failed call as a successful empty completion overwrites a real cached theme/narrative with nothing. Until now that was only proven transitively — the shared transport returns an error, and you read theif (result.error) returnguard at each call site. Now asserted directly: an errored transport result surfaces as{ available: false, reason }and leavesatomicWriteuntouched, for bothgenerateThemeAnalysisandrefreshCrossDomainNarrative— plus the positive case, so the guard can't pass by never writing at all.Test plan
cd server && npx vitest run services/insightsService.test.js services/aiProvider.test.js— 39 passif (result.error)guards ininsightsService.jsfails exactly the two persist-guard tests and nothing else; restoring them goes green againfileUtilsis partial-mocked viaimportOriginal—PATHSis read atinsightsServicemodule scope, so a mock omitting it breaks the import outright