From cb0a13bbf5c18b832a5c2b08674ac588f410e441 Mon Sep 17 00:00:00 2001 From: iliya Date: Sun, 15 Mar 2026 10:07:22 +0200 Subject: [PATCH] feat: enhance file content resolution caching and validation - Introduced a validation fingerprint mechanism in FileContentResolver to ensure cached content is reused only when both disk content and snippets remain unchanged. - Reduced cache TTL to 5 seconds for provisional entries to minimize stale data risks. - Added utility functions for generating fingerprints based on disk content and snippet details. - Updated cache handling logic to incorporate validation checks, improving efficiency and accuracy in content retrieval. - Enhanced unit tests to cover new caching behavior and fingerprint validation scenarios. --- src/main/services/team/FileContentResolver.ts | 97 +++- src/main/services/team/TeamConfigReader.ts | 9 +- src/main/services/team/TeamDataService.ts | 62 ++- src/main/services/team/TeamMemberResolver.ts | 9 +- .../store/slices/changeReviewSlice.ts | 196 +++++++-- src/shared/utils/teamMemberName.ts | 29 ++ .../services/team/FileContentResolver.test.ts | 183 +++++++- .../services/team/TeamDataService.test.ts | 222 +++++++++- test/renderer/store/changeReviewSlice.test.ts | 413 ++++++++++++++++++ test/shared/utils/teamMemberName.test.ts | 48 +- 10 files changed, 1202 insertions(+), 66 deletions(-) diff --git a/src/main/services/team/FileContentResolver.ts b/src/main/services/team/FileContentResolver.ts index 0795b417..5cbdf65b 100644 --- a/src/main/services/team/FileContentResolver.ts +++ b/src/main/services/team/FileContentResolver.ts @@ -1,5 +1,7 @@ import { getHomeDir } from '@main/utils/pathDecoder'; import { createLogger } from '@shared/utils/logger'; +import { normalizePathForComparison } from '@shared/utils/platformPath'; +import { createHash } from 'crypto'; import { diffLines } from 'diff'; import { createReadStream } from 'fs'; import { access, readFile } from 'fs/promises'; @@ -17,6 +19,7 @@ interface ContentCacheEntry { original: string | null; modified: string | null; source: FileChangeWithContent['contentSource']; + validationFingerprint: string; expiresAt: number; } @@ -30,7 +33,7 @@ interface ContentCacheEntry { */ export class FileContentResolver { private cache = new Map(); - private readonly cacheTtl = 30 * 1000; // 30 сек — shorter TTL to reduce stale data risk + private readonly provisionalCacheTtl = 5 * 1000; constructor( private readonly logsFinder: TeamMemberLogsFinder, @@ -60,12 +63,6 @@ export class FileContentResolver { modified: string | null; source: FileChangeWithContent['contentSource']; }> { - const cacheKey = `${teamName}:${memberName}:${filePath}`; - const cached = this.cache.get(cacheKey); - if (cached && cached.expiresAt > Date.now()) { - return { original: cached.original, modified: cached.modified, source: cached.source }; - } - // Read current file from disk (= modified state after agent's changes) let currentContent: string | null = null; try { @@ -74,6 +71,21 @@ export class FileContentResolver { logger.debug(`Файл недоступен на диске: ${filePath}`); } + const cacheKey = `${teamName}:${memberName}:${filePath}`; + const validationFingerprint = this.buildValidationFingerprint( + filePath, + currentContent, + snippets + ); + const cached = this.cache.get(cacheKey); + if ( + cached && + cached.expiresAt > Date.now() && + cached.validationFingerprint === validationFingerprint + ) { + return { original: cached.original, modified: cached.modified, source: cached.source }; + } + // Fast path: if the agent created the file and it still exists on disk, // the original content is definitely empty, so skip expensive history lookup. const hasWriteNew = snippets.some((s) => !s.isError && s.type === 'write-new'); @@ -83,7 +95,7 @@ export class FileContentResolver { modified: currentContent, source: 'snippet-reconstruction' as const, }; - this.cacheResult(cacheKey, result); + this.cacheResult(cacheKey, validationFingerprint, result); return result; } @@ -95,7 +107,7 @@ export class FileContentResolver { modified: currentContent, source: 'file-history' as const, }; - this.cacheResult(cacheKey, result); + this.cacheResult(cacheKey, validationFingerprint, result); return result; } @@ -107,7 +119,7 @@ export class FileContentResolver { modified: currentContent, source: 'snippet-reconstruction' as const, }; - this.cacheResult(cacheKey, result); + this.cacheResult(cacheKey, validationFingerprint, result); return result; } @@ -120,7 +132,7 @@ export class FileContentResolver { modified: currentContent, source: 'git-fallback' as const, }; - this.cacheResult(cacheKey, result); + this.cacheResult(cacheKey, validationFingerprint, result); return result; } } @@ -132,12 +144,14 @@ export class FileContentResolver { modified: currentContent, source: 'disk-current' as const, }; - this.cacheResult(cacheKey, result); + this.cacheResult(cacheKey, validationFingerprint, result); return result; } // Nothing available - return { original: null, modified: null, source: 'unavailable' }; + const unavailable = { original: null, modified: null, source: 'unavailable' as const }; + this.cacheResult(cacheKey, validationFingerprint, unavailable); + return unavailable; } /** @@ -538,8 +552,62 @@ export class FileContentResolver { // ── Private: Cache helpers ── + private normalizeResolverPath(filePath: string): string { + return normalizePathForComparison(filePath); + } + + private hashString(input: string): string { + return createHash('sha256').update(input).digest('hex'); + } + + private buildDiskFingerprint(currentContent: string | null): string { + if (currentContent === null) return 'missing'; + return this.hashString(`present:${currentContent}`); + } + + private buildSnippetFingerprint(snippets: SnippetDiff[]): string { + const hash = createHash('sha256'); + for (const snippet of snippets) { + hash.update('\u0000snippet\u0000'); + hash.update(this.normalizeResolverPath(snippet.filePath)); + hash.update('\u0000'); + hash.update(snippet.toolUseId); + hash.update('\u0000'); + hash.update(snippet.type); + hash.update('\u0000'); + hash.update(snippet.oldString); + hash.update('\u0000'); + hash.update(snippet.newString); + hash.update('\u0000'); + hash.update(snippet.replaceAll ? '1' : '0'); + hash.update('\u0000'); + hash.update(snippet.timestamp); + hash.update('\u0000'); + hash.update(snippet.isError ? '1' : '0'); + hash.update('\u0000'); + hash.update(snippet.contextHash ?? ''); + } + return hash.digest('hex'); + } + + private buildValidationFingerprint( + filePath: string, + currentContent: string | null, + snippets: SnippetDiff[] + ): string { + const normalizedPath = this.normalizeResolverPath(filePath); + const diskFingerprint = this.buildDiskFingerprint(currentContent); + const snippetFingerprint = this.buildSnippetFingerprint(snippets); + return this.hashString(`${normalizedPath}|${diskFingerprint}|${snippetFingerprint}`); + } + + private getCacheTtlForSource(_source: FileChangeWithContent['contentSource']): number { + return this.provisionalCacheTtl; + } + private cacheResult( key: string, + validationFingerprint: string, result: { original: string | null; modified: string | null; @@ -550,7 +618,8 @@ export class FileContentResolver { original: result.original, modified: result.modified, source: result.source, - expiresAt: Date.now() + this.cacheTtl, + validationFingerprint, + expiresAt: Date.now() + this.getCacheTtlForSource(result.source), }); } } diff --git a/src/main/services/team/TeamConfigReader.ts b/src/main/services/team/TeamConfigReader.ts index 8cc26468..d4d684b3 100644 --- a/src/main/services/team/TeamConfigReader.ts +++ b/src/main/services/team/TeamConfigReader.ts @@ -1,7 +1,10 @@ import { FileReadTimeoutError, readFileUtf8WithTimeout } from '@main/utils/fsRead'; import { getTeamsBasePath } from '@main/utils/pathDecoder'; import { createLogger } from '@shared/utils/logger'; -import { createCliAutoSuffixNameGuard } from '@shared/utils/teamMemberName'; +import { + createCliAutoSuffixNameGuard, + createCliProvisionerNameGuard, +} from '@shared/utils/teamMemberName'; import * as fs from 'fs'; import * as path from 'path'; @@ -248,8 +251,10 @@ export class TeamConfigReader { // Defense: drop CLI auto-suffixed duplicates (alice-2) when base name exists. const allNames = Array.from(memberMap.values()).map((m) => m.name); const keepName = createCliAutoSuffixNameGuard(allNames); + // Defense: drop CLI provisioner artifacts (alice-provisioner) when base name exists. + const keepProvisioner = createCliProvisionerNameGuard(allNames); for (const [key, member] of Array.from(memberMap.entries())) { - if (!keepName(member.name)) { + if (!keepName(member.name) || !keepProvisioner(member.name)) { memberMap.delete(key); } } diff --git a/src/main/services/team/TeamDataService.ts b/src/main/services/team/TeamDataService.ts index fb8ab7c7..d4b62460 100644 --- a/src/main/services/team/TeamDataService.ts +++ b/src/main/services/team/TeamDataService.ts @@ -1203,26 +1203,74 @@ export class TeamDataService { ].join('\n'); } + private logTaskCommentNotificationSkip( + teamName: string, + task: Pick, + reason: string, + comment?: Pick + ): void { + const commentSuffix = comment ? `:${comment.id}` : ''; + logger.info( + `[TeamDataService] Skipped task comment notification for ${teamName}#${this.getTaskLabel(task)}${commentSuffix} (${reason})` + ); + } + private getEligibleTaskCommentNotifications( teamName: string, task: TeamTask, leadName: string, leadSessionId?: string ): EligibleTaskCommentNotification[] { - if (task.status === 'deleted') return []; + if (task.status === 'deleted') { + this.logTaskCommentNotificationSkip(teamName, task, 'task deleted'); + return []; + } const owner = task.owner?.trim() ?? ''; - if (!owner || this.isLeadOwner(owner, leadName)) return []; + if (!owner) { + this.logTaskCommentNotificationSkip(teamName, task, 'task has no owner'); + return []; + } + if (this.isLeadOwner(owner, leadName)) { + this.logTaskCommentNotificationSkip(teamName, task, 'task owner is lead'); + return []; + } const taskRef = this.buildTaskRef(teamName, task); const comments = Array.isArray(task.comments) ? task.comments : []; const out: EligibleTaskCommentNotification[] = []; for (const comment of comments) { - if (comment.type !== 'regular') continue; + if (comment.type !== 'regular') { + this.logTaskCommentNotificationSkip( + teamName, + task, + `comment type ${comment.type}`, + comment + ); + continue; + } const author = comment.author?.trim() ?? ''; - if (!author || author.toLowerCase() === 'user') continue; - if (this.isLeadOwner(author, leadName)) continue; - if (comment.id.startsWith('msg-')) continue; + if (!author) { + this.logTaskCommentNotificationSkip(teamName, task, 'comment author missing', comment); + continue; + } + if (author.toLowerCase() === 'user') { + this.logTaskCommentNotificationSkip(teamName, task, 'comment author is user', comment); + continue; + } + if (this.isLeadOwner(author, leadName)) { + this.logTaskCommentNotificationSkip(teamName, task, 'comment author is lead', comment); + continue; + } + if (comment.id.startsWith('msg-')) { + this.logTaskCommentNotificationSkip( + teamName, + task, + 'comment is mirrored inbox artifact', + comment + ); + continue; + } const key = this.buildTaskCommentNotificationKey(task, comment); out.push({ @@ -1234,7 +1282,7 @@ export class TeamDataService { leadSessionId, taskRef, text: this.buildTaskCommentNotificationText(task, comment), - summary: `**Comment on** #${taskRef.displayId}`, + summary: `Comment on #${taskRef.displayId}`, }); } diff --git a/src/main/services/team/TeamMemberResolver.ts b/src/main/services/team/TeamMemberResolver.ts index af5f3873..f6c9830d 100644 --- a/src/main/services/team/TeamMemberResolver.ts +++ b/src/main/services/team/TeamMemberResolver.ts @@ -1,4 +1,7 @@ -import { createCliAutoSuffixNameGuard } from '@shared/utils/teamMemberName'; +import { + createCliAutoSuffixNameGuard, + createCliProvisionerNameGuard, +} from '@shared/utils/teamMemberName'; import type { InboxMessage, @@ -149,8 +152,10 @@ export class TeamMemberResolver { // Defense: hide CLI auto-suffixed duplicates (alice-2) when base name (alice) exists. const keepName = createCliAutoSuffixNameGuard(names); + // Defense: hide CLI provisioner artifacts (alice-provisioner) when base name (alice) exists. + const keepProvisioner = createCliProvisionerNameGuard(names); for (const name of Array.from(names)) { - if (!keepName(name)) { + if (!keepName(name) || !keepProvisioner(name)) { names.delete(name); } } diff --git a/src/renderer/store/slices/changeReviewSlice.ts b/src/renderer/store/slices/changeReviewSlice.ts index 48a8152b..4f9ebc42 100644 --- a/src/renderer/store/slices/changeReviewSlice.ts +++ b/src/renderer/store/slices/changeReviewSlice.ts @@ -6,6 +6,7 @@ import { } from '@renderer/utils/taskChangeRequest'; import { computeDiffContextHash } from '@shared/utils/diffContextHash'; import { createLogger } from '@shared/utils/logger'; +import { normalizePathForComparison } from '@shared/utils/platformPath'; import { structuredPatch } from 'diff'; /** Tracks in-flight checkTaskHasChanges calls to avoid duplicate requests */ @@ -32,6 +33,7 @@ import type { FileChangeWithContent, FileReviewDecision, HunkDecision, + SnippetDiff, TaskChangeSet, TaskChangeSetV2, } from '@shared/types'; @@ -45,6 +47,8 @@ interface DecisionSnapshot { fileDecisions: Record; } +type ReviewChangeSet = AgentChangeSet | TaskChangeSet | TaskChangeSetV2; + const MAX_REVIEW_UNDO_DEPTH = 10; /** @@ -89,6 +93,8 @@ export interface ChangeReviewSlice { hunkContextHashesByFile: Record>; fileContents: Record; fileContentsLoading: Record; + changeSetEpoch: number; + fileContentVersionByPath: Record; collapseUnchanged: boolean; applyError: string | null; applying: boolean; @@ -270,10 +276,83 @@ function buildHunkContextHashesForFile( return out; } +function encodeFingerprintField(value: string): string { + return `${value.length}:${value}`; +} + +function fingerprintSnippet(snippet: SnippetDiff): string { + return [ + encodeFingerprintField(normalizePathForComparison(snippet.filePath)), + encodeFingerprintField(snippet.toolUseId), + encodeFingerprintField(snippet.timestamp), + encodeFingerprintField(snippet.type), + encodeFingerprintField(snippet.oldString), + encodeFingerprintField(snippet.newString), + encodeFingerprintField(snippet.replaceAll ? '1' : '0'), + encodeFingerprintField(snippet.isError ? '1' : '0'), + encodeFingerprintField(snippet.contextHash ?? ''), + ].join('|'); +} + +function fingerprintChangeSet(changeSet: ReviewChangeSet): string { + return [...changeSet.files] + .sort((a, b) => + normalizePathForComparison(a.filePath).localeCompare(normalizePathForComparison(b.filePath)) + ) + .map((file) => + [ + encodeFingerprintField(normalizePathForComparison(file.filePath)), + ...file.snippets.map(fingerprintSnippet), + ].join('|') + ) + .join('||'); +} + export const createChangeReviewSlice: StateCreator = ( set, get ) => { + const installActiveChangeSetForLoad = ( + data: ReviewChangeSet, + extraState?: Partial + ): void => { + set((s) => ({ + activeChangeSet: data, + changeSetLoading: false, + selectedReviewFilePath: data.files[0]?.filePath ?? null, + fileContents: {}, + fileContentsLoading: {}, + fileChunkCounts: {}, + hunkContextHashesByFile: {}, + applyError: null, + changeSetEpoch: s.changeSetEpoch + 1, + fileContentVersionByPath: {}, + ...extraState, + })); + }; + + const replaceActiveChangeSetAfterStaleRefresh = ( + fresh: ReviewChangeSet, + applyError: string + ): void => { + set((s) => ({ + activeChangeSet: fresh, + applying: false, + applyError, + selectedReviewFilePath: fresh.files[0]?.filePath ?? null, + hunkDecisions: {}, + fileDecisions: {}, + fileChunkCounts: {}, + reviewUndoStack: [], + hunkContextHashesByFile: {}, + fileContents: {}, + fileContentsLoading: {}, + editedContents: {}, + changeSetEpoch: s.changeSetEpoch + 1, + fileContentVersionByPath: {}, + })); + }; + const revalidateTaskChangePresence = async ( teamName: string, taskId: string, @@ -326,6 +405,8 @@ export const createChangeReviewSlice: StateCreator ({ - activeChangeSet: data, + installActiveChangeSetForLoad(data, { activeTaskChangeRequestOptions: options, - changeSetLoading: false, - selectedReviewFilePath: data.files[0]?.filePath ?? null, - taskHasChanges: { ...s.taskHasChanges, [cacheKey]: data.files.length > 0 }, - })); + taskHasChanges: { ...get().taskHasChanges, [cacheKey]: data.files.length > 0 }, + }); if (data.files.length > 0) { taskChangesNegativeCache.delete(cacheKey); } else { @@ -405,7 +479,7 @@ export const createChangeReviewSlice: StateCreator { latestTaskChangesRequestToken++; - set({ + set((s) => ({ activeChangeSet: null, changeSetLoading: false, changeSetError: null, @@ -418,15 +492,17 @@ export const createChangeReviewSlice: StateCreator { latestTaskChangesRequestToken++; - set({ + set((s) => ({ activeChangeSet: null, changeSetLoading: false, changeSetError: null, @@ -437,15 +513,17 @@ export const createChangeReviewSlice: StateCreator { latestTaskChangesRequestToken++; - set({ + set((s) => ({ activeChangeSet: null, changeSetLoading: false, changeSetError: null, @@ -458,10 +536,12 @@ export const createChangeReviewSlice: StateCreator ({ fileContentsLoading: { ...s.fileContentsLoading, [filePath]: true }, @@ -705,6 +787,9 @@ export const createChangeReviewSlice: StateCreator { const result: Partial = { fileContents: { ...s.fileContents, [filePath]: content }, @@ -735,6 +820,9 @@ export const createChangeReviewSlice: StateCreator ({ fileContentsLoading: { ...s.fileContentsLoading, [filePath]: false }, @@ -749,20 +837,16 @@ export const createChangeReviewSlice: StateCreator - `${cs.totalFiles}:${cs.files.map((f) => f.filePath).join(',')}`; + const currentFingerprint = current + ? fingerprintChangeSet(current as ReviewChangeSet) + : null; + const staleMessage = + 'Changes have been updated since you started reviewing. Please re-review.'; if (memberName && current) { const fresh = await api.review.getAgentChanges(teamName, memberName); - if (fingerprint(fresh) !== fingerprint(current)) { - set({ - activeChangeSet: fresh, - applying: false, - applyError: - 'Changes have been updated since you started reviewing. Please re-review.', - }); + if (currentFingerprint !== fingerprintChangeSet(fresh)) { + replaceActiveChangeSetAfterStaleRefresh(fresh, staleMessage); return; } } else if (taskId && current) { @@ -770,13 +854,8 @@ export const createChangeReviewSlice: StateCreator { const content = get().editedContents[filePath]; if (!(filePath in get().editedContents)) return; - set({ applying: true, applyError: null }); + set((s) => ({ + fileContentsLoading: { ...s.fileContentsLoading, [filePath]: false }, + applying: true, + applyError: null, + fileContentVersionByPath: { + ...s.fileContentVersionByPath, + [filePath]: (s.fileContentVersionByPath[filePath] ?? 0) + 1, + }, + })); try { await api.review.saveEditedFile(filePath, content, projectPath); set((s) => { const nextEdited = { ...s.editedContents }; delete nextEdited[filePath]; + + const nextFileChunkCounts = { ...s.fileChunkCounts }; + delete nextFileChunkCounts[filePath]; + + const nextHunkContextHashesByFile = { ...s.hunkContextHashesByFile }; + delete nextHunkContextHashesByFile[filePath]; + // Update cached content in-place to avoid skeleton flash. // Replace modifiedFullContent with saved version so CodeMirror // reflects the new baseline without a full re-fetch cycle. @@ -1096,7 +1212,13 @@ export const createChangeReviewSlice: StateCreator