From fb4a009041e6b3edeba472de938e5046772d6748 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 8 Sep 2026 11:09:39 +0000 Subject: [PATCH] fix(generation): do not overwrite real tool reports with placeholders Catalog fill, ensure-tool-report extraInputs, and overlapping generate/visit races all persist through persistOnDemandToolReports. A later success+placeholder payload could replace a stored palmistry (or other) reading. Match Stage B's keep-existing guard so a failed refresh cannot destroy a real report. Co-authored-by: Andy Oliver Rozario --- lib/onDemandToolReports.ts | 33 +++- .../unit/onDemandPlaceholderOverwrite.test.ts | 180 ++++++++++++++++++ 2 files changed, 210 insertions(+), 3 deletions(-) create mode 100644 tests/unit/onDemandPlaceholderOverwrite.test.ts diff --git a/lib/onDemandToolReports.ts b/lib/onDemandToolReports.ts index 4c059cf..c11b07a 100644 --- a/lib/onDemandToolReports.ts +++ b/lib/onDemandToolReports.ts @@ -23,6 +23,25 @@ export function isOnDemandToolSlug(slug: string): slug is OnDemandToolSlug { return (ALL_TOOL_SLUGS as readonly string[]).includes(slug); } +function isRealStoredReport(report: unknown): boolean { + if (!report || typeof report !== 'object') return false; + return (report as { placeholder?: boolean }).placeholder !== true; +} + +/** + * Stage B persist already refuses to clobber a real reading with a placeholder. + * Catalog fill, ensure-tool-report extraInputs, and overlapping generate/visit + * races all share persistOnDemandToolReports — apply the same guard here. + */ +export function shouldKeepExistingReportOverPlaceholder( + existing: unknown, + incoming: unknown, +): boolean { + if (!isRealStoredReport(existing)) return false; + if (!incoming || typeof incoming !== 'object') return false; + return (incoming as { placeholder?: boolean }).placeholder === true; +} + function mergeToolStatus( existing: PersistedToolStatusMap, slug: string, @@ -74,13 +93,21 @@ export async function persistOnDemandToolReports(params: { const failedSlugs: string[] = []; for (const [slug, entry] of Object.entries(toolReports)) { + const incomingData = + entry.status === 'success' && entry.data && typeof entry.data === 'object' + ? collapseDuplicateReportFields(entry.data as Record) + : null; + if (incomingData && shouldKeepExistingReportOverPlaceholder(existingProfile[slug], incomingData)) { + failedSlugs.push(slug); + continue; + } toolStatus = mergeToolStatus(toolStatus, slug, entry, now); - if (entry.status === 'success' && entry.data && typeof entry.data === 'object') { + if (incomingData) { profilePatch[slug] = { - ...collapseDuplicateReportFields(entry.data as Record), + ...incomingData, generationIdempotencyKey: profileHash, }; - if (isReadyToolReport(entry.data, slug)) readySlugs.push(slug); + if (isReadyToolReport(incomingData, slug)) readySlugs.push(slug); else failedSlugs.push(slug); } else { failedSlugs.push(slug); diff --git a/tests/unit/onDemandPlaceholderOverwrite.test.ts b/tests/unit/onDemandPlaceholderOverwrite.test.ts new file mode 100644 index 0000000..5d5d290 --- /dev/null +++ b/tests/unit/onDemandPlaceholderOverwrite.test.ts @@ -0,0 +1,180 @@ +/** + * Catalog / ensure-tool-report persist must not replace a real reading with a placeholder. + * @jest-environment node + */ + +const mockGetDocument = jest.fn(); +const mockSetDocument = jest.fn(); +const mockClearCachedDivinationData = jest.fn(); + +jest.mock('@/lib/firebase-admin', () => ({ + getDocument: (...args: unknown[]) => mockGetDocument(...args), + setDocument: (...args: unknown[]) => mockSetDocument(...args), +})); + +jest.mock('@/lib/universalDataAggregator', () => ({ + clearCachedDivinationData: (...args: unknown[]) => mockClearCachedDivinationData(...args), +})); + +import { + persistOnDemandToolReports, + shouldKeepExistingReportOverPlaceholder, +} from '@/lib/onDemandToolReports'; + +describe('shouldKeepExistingReportOverPlaceholder', () => { + const realPalmistry = { + palmistryContext: { lines: { lifeLine: 'long' } }, + analysis: { overview: 'Strong life line' }, + generationIdempotencyKey: 'hash-1', + }; + + it('keeps a real stored report when the incoming payload is a placeholder', () => { + expect( + shouldKeepExistingReportOverPlaceholder(realPalmistry, { + placeholder: true, + reason: 'Palm analysis failed. Try re-uploading a clearer image.', + }), + ).toBe(true); + }); + + it('does not keep a placeholder over another placeholder', () => { + expect( + shouldKeepExistingReportOverPlaceholder( + { placeholder: true, reason: 'Upload hand images' }, + { placeholder: true, reason: 'Palm analysis failed' }, + ), + ).toBe(false); + }); + + it('does not keep when incoming is a real report', () => { + expect( + shouldKeepExistingReportOverPlaceholder(realPalmistry, { + palmistryContext: { lines: { lifeLine: 'short' } }, + analysis: { overview: 'Updated' }, + }), + ).toBe(false); + }); + + it('does not keep when nothing is stored yet', () => { + expect( + shouldKeepExistingReportOverPlaceholder(undefined, { placeholder: true, reason: 'unavailable' }), + ).toBe(false); + }); +}); + +describe('persistOnDemandToolReports placeholder guard', () => { + const uid = 'user-1'; + const profileHash = 'hash-1'; + const realPalmistry = { + palmistryContext: { lines: { lifeLine: 'long' } }, + analysis: { overview: 'Strong life line' }, + generationIdempotencyKey: profileHash, + }; + + beforeEach(() => { + jest.clearAllMocks(); + mockSetDocument.mockResolvedValue(true); + }); + + it('does not overwrite a stored palmistry reading with a later placeholder persist', async () => { + mockGetDocument.mockImplementation((collection: string) => { + if (collection === 'comprehensiveMysticalProfiles') { + return Promise.resolve({ + palmistry: realPalmistry, + toolStatus: { palmistry: { state: 'ready', attempts: 1 } }, + }); + } + return Promise.resolve({}); + }); + + const result = await persistOnDemandToolReports({ + uid, + profileHash, + toolReports: { + palmistry: { + status: 'success', + data: { + placeholder: true, + reason: 'Palm analysis failed. Try re-uploading a clearer image.', + }, + generatedAt: new Date().toISOString(), + }, + }, + }); + + expect(result.failedSlugs).toContain('palmistry'); + expect(result.readySlugs).not.toContain('palmistry'); + + const profileWrite = mockSetDocument.mock.calls.find( + (call: unknown[]) => call[0] === 'comprehensiveMysticalProfiles', + ); + expect(profileWrite).toBeDefined(); + const patch = profileWrite?.[2] as Record; + expect(patch.palmistry).toBeUndefined(); + }); + + it('still writes a placeholder when the tool has no real report yet', async () => { + mockGetDocument.mockResolvedValue({}); + + await persistOnDemandToolReports({ + uid, + profileHash, + toolReports: { + faceReading: { + status: 'success', + data: { placeholder: true, reason: 'Physiognomy needs a readable photo.' }, + generatedAt: new Date().toISOString(), + }, + }, + }); + + const profileWrite = mockSetDocument.mock.calls.find( + (call: unknown[]) => call[0] === 'comprehensiveMysticalProfiles', + ); + const patch = profileWrite?.[2] as Record; + expect(patch.faceReading).toEqual( + expect.objectContaining({ + placeholder: true, + generationIdempotencyKey: profileHash, + }), + ); + }); + + it('still persists a sibling tool in the same batch when one slug is kept', async () => { + mockGetDocument.mockImplementation((collection: string) => { + if (collection === 'comprehensiveMysticalProfiles') { + return Promise.resolve({ palmistry: realPalmistry }); + } + return Promise.resolve({}); + }); + + await persistOnDemandToolReports({ + uid, + profileHash, + toolReports: { + palmistry: { + status: 'success', + data: { placeholder: true, reason: 'Palm analysis failed.' }, + generatedAt: new Date().toISOString(), + }, + tarot: { + status: 'success', + data: { cards: [{ name: 'The Fool' }], profile: { birthCard: { name: 'The Fool' } } }, + generatedAt: new Date().toISOString(), + }, + }, + }); + + const profileWrite = mockSetDocument.mock.calls.find( + (call: unknown[]) => call[0] === 'comprehensiveMysticalProfiles', + ); + const patch = profileWrite?.[2] as Record; + expect(patch.palmistry).toBeUndefined(); + expect(patch.tarot).toEqual( + expect.objectContaining({ + cards: [{ name: 'The Fool' }], + generationIdempotencyKey: profileHash, + }), + ); + }); +});