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.
This commit is contained in:
iliya 2026-03-15 10:07:22 +02:00
parent e60229622d
commit cb0a13bbf5
10 changed files with 1202 additions and 66 deletions

View file

@ -1,5 +1,7 @@
import { getHomeDir } from '@main/utils/pathDecoder'; import { getHomeDir } from '@main/utils/pathDecoder';
import { createLogger } from '@shared/utils/logger'; import { createLogger } from '@shared/utils/logger';
import { normalizePathForComparison } from '@shared/utils/platformPath';
import { createHash } from 'crypto';
import { diffLines } from 'diff'; import { diffLines } from 'diff';
import { createReadStream } from 'fs'; import { createReadStream } from 'fs';
import { access, readFile } from 'fs/promises'; import { access, readFile } from 'fs/promises';
@ -17,6 +19,7 @@ interface ContentCacheEntry {
original: string | null; original: string | null;
modified: string | null; modified: string | null;
source: FileChangeWithContent['contentSource']; source: FileChangeWithContent['contentSource'];
validationFingerprint: string;
expiresAt: number; expiresAt: number;
} }
@ -30,7 +33,7 @@ interface ContentCacheEntry {
*/ */
export class FileContentResolver { export class FileContentResolver {
private cache = new Map<string, ContentCacheEntry>(); private cache = new Map<string, ContentCacheEntry>();
private readonly cacheTtl = 30 * 1000; // 30 сек — shorter TTL to reduce stale data risk private readonly provisionalCacheTtl = 5 * 1000;
constructor( constructor(
private readonly logsFinder: TeamMemberLogsFinder, private readonly logsFinder: TeamMemberLogsFinder,
@ -60,12 +63,6 @@ export class FileContentResolver {
modified: string | null; modified: string | null;
source: FileChangeWithContent['contentSource']; 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) // Read current file from disk (= modified state after agent's changes)
let currentContent: string | null = null; let currentContent: string | null = null;
try { try {
@ -74,6 +71,21 @@ export class FileContentResolver {
logger.debug(`Файл недоступен на диске: ${filePath}`); 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, // 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. // the original content is definitely empty, so skip expensive history lookup.
const hasWriteNew = snippets.some((s) => !s.isError && s.type === 'write-new'); const hasWriteNew = snippets.some((s) => !s.isError && s.type === 'write-new');
@ -83,7 +95,7 @@ export class FileContentResolver {
modified: currentContent, modified: currentContent,
source: 'snippet-reconstruction' as const, source: 'snippet-reconstruction' as const,
}; };
this.cacheResult(cacheKey, result); this.cacheResult(cacheKey, validationFingerprint, result);
return result; return result;
} }
@ -95,7 +107,7 @@ export class FileContentResolver {
modified: currentContent, modified: currentContent,
source: 'file-history' as const, source: 'file-history' as const,
}; };
this.cacheResult(cacheKey, result); this.cacheResult(cacheKey, validationFingerprint, result);
return result; return result;
} }
@ -107,7 +119,7 @@ export class FileContentResolver {
modified: currentContent, modified: currentContent,
source: 'snippet-reconstruction' as const, source: 'snippet-reconstruction' as const,
}; };
this.cacheResult(cacheKey, result); this.cacheResult(cacheKey, validationFingerprint, result);
return result; return result;
} }
@ -120,7 +132,7 @@ export class FileContentResolver {
modified: currentContent, modified: currentContent,
source: 'git-fallback' as const, source: 'git-fallback' as const,
}; };
this.cacheResult(cacheKey, result); this.cacheResult(cacheKey, validationFingerprint, result);
return result; return result;
} }
} }
@ -132,12 +144,14 @@ export class FileContentResolver {
modified: currentContent, modified: currentContent,
source: 'disk-current' as const, source: 'disk-current' as const,
}; };
this.cacheResult(cacheKey, result); this.cacheResult(cacheKey, validationFingerprint, result);
return result; return result;
} }
// Nothing available // 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: 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( private cacheResult(
key: string, key: string,
validationFingerprint: string,
result: { result: {
original: string | null; original: string | null;
modified: string | null; modified: string | null;
@ -550,7 +618,8 @@ export class FileContentResolver {
original: result.original, original: result.original,
modified: result.modified, modified: result.modified,
source: result.source, source: result.source,
expiresAt: Date.now() + this.cacheTtl, validationFingerprint,
expiresAt: Date.now() + this.getCacheTtlForSource(result.source),
}); });
} }
} }

View file

@ -1,7 +1,10 @@
import { FileReadTimeoutError, readFileUtf8WithTimeout } from '@main/utils/fsRead'; import { FileReadTimeoutError, readFileUtf8WithTimeout } from '@main/utils/fsRead';
import { getTeamsBasePath } from '@main/utils/pathDecoder'; import { getTeamsBasePath } from '@main/utils/pathDecoder';
import { createLogger } from '@shared/utils/logger'; 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 fs from 'fs';
import * as path from 'path'; import * as path from 'path';
@ -248,8 +251,10 @@ export class TeamConfigReader {
// Defense: drop CLI auto-suffixed duplicates (alice-2) when base name exists. // Defense: drop CLI auto-suffixed duplicates (alice-2) when base name exists.
const allNames = Array.from(memberMap.values()).map((m) => m.name); const allNames = Array.from(memberMap.values()).map((m) => m.name);
const keepName = createCliAutoSuffixNameGuard(allNames); 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())) { for (const [key, member] of Array.from(memberMap.entries())) {
if (!keepName(member.name)) { if (!keepName(member.name) || !keepProvisioner(member.name)) {
memberMap.delete(key); memberMap.delete(key);
} }
} }

View file

@ -1203,26 +1203,74 @@ export class TeamDataService {
].join('\n'); ].join('\n');
} }
private logTaskCommentNotificationSkip(
teamName: string,
task: Pick<TeamTask, 'id' | 'displayId'>,
reason: string,
comment?: Pick<TaskComment, 'id'>
): void {
const commentSuffix = comment ? `:${comment.id}` : '';
logger.info(
`[TeamDataService] Skipped task comment notification for ${teamName}#${this.getTaskLabel(task)}${commentSuffix} (${reason})`
);
}
private getEligibleTaskCommentNotifications( private getEligibleTaskCommentNotifications(
teamName: string, teamName: string,
task: TeamTask, task: TeamTask,
leadName: string, leadName: string,
leadSessionId?: string leadSessionId?: string
): EligibleTaskCommentNotification[] { ): EligibleTaskCommentNotification[] {
if (task.status === 'deleted') return []; if (task.status === 'deleted') {
this.logTaskCommentNotificationSkip(teamName, task, 'task deleted');
return [];
}
const owner = task.owner?.trim() ?? ''; 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 taskRef = this.buildTaskRef(teamName, task);
const comments = Array.isArray(task.comments) ? task.comments : []; const comments = Array.isArray(task.comments) ? task.comments : [];
const out: EligibleTaskCommentNotification[] = []; const out: EligibleTaskCommentNotification[] = [];
for (const comment of comments) { 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() ?? ''; const author = comment.author?.trim() ?? '';
if (!author || author.toLowerCase() === 'user') continue; if (!author) {
if (this.isLeadOwner(author, leadName)) continue; this.logTaskCommentNotificationSkip(teamName, task, 'comment author missing', comment);
if (comment.id.startsWith('msg-')) continue; 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); const key = this.buildTaskCommentNotificationKey(task, comment);
out.push({ out.push({
@ -1234,7 +1282,7 @@ export class TeamDataService {
leadSessionId, leadSessionId,
taskRef, taskRef,
text: this.buildTaskCommentNotificationText(task, comment), text: this.buildTaskCommentNotificationText(task, comment),
summary: `**Comment on** #${taskRef.displayId}`, summary: `Comment on #${taskRef.displayId}`,
}); });
} }

View file

@ -1,4 +1,7 @@
import { createCliAutoSuffixNameGuard } from '@shared/utils/teamMemberName'; import {
createCliAutoSuffixNameGuard,
createCliProvisionerNameGuard,
} from '@shared/utils/teamMemberName';
import type { import type {
InboxMessage, InboxMessage,
@ -149,8 +152,10 @@ export class TeamMemberResolver {
// Defense: hide CLI auto-suffixed duplicates (alice-2) when base name (alice) exists. // Defense: hide CLI auto-suffixed duplicates (alice-2) when base name (alice) exists.
const keepName = createCliAutoSuffixNameGuard(names); 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)) { for (const name of Array.from(names)) {
if (!keepName(name)) { if (!keepName(name) || !keepProvisioner(name)) {
names.delete(name); names.delete(name);
} }
} }

View file

@ -6,6 +6,7 @@ import {
} from '@renderer/utils/taskChangeRequest'; } from '@renderer/utils/taskChangeRequest';
import { computeDiffContextHash } from '@shared/utils/diffContextHash'; import { computeDiffContextHash } from '@shared/utils/diffContextHash';
import { createLogger } from '@shared/utils/logger'; import { createLogger } from '@shared/utils/logger';
import { normalizePathForComparison } from '@shared/utils/platformPath';
import { structuredPatch } from 'diff'; import { structuredPatch } from 'diff';
/** Tracks in-flight checkTaskHasChanges calls to avoid duplicate requests */ /** Tracks in-flight checkTaskHasChanges calls to avoid duplicate requests */
@ -32,6 +33,7 @@ import type {
FileChangeWithContent, FileChangeWithContent,
FileReviewDecision, FileReviewDecision,
HunkDecision, HunkDecision,
SnippetDiff,
TaskChangeSet, TaskChangeSet,
TaskChangeSetV2, TaskChangeSetV2,
} from '@shared/types'; } from '@shared/types';
@ -45,6 +47,8 @@ interface DecisionSnapshot {
fileDecisions: Record<string, HunkDecision>; fileDecisions: Record<string, HunkDecision>;
} }
type ReviewChangeSet = AgentChangeSet | TaskChangeSet | TaskChangeSetV2;
const MAX_REVIEW_UNDO_DEPTH = 10; const MAX_REVIEW_UNDO_DEPTH = 10;
/** /**
@ -89,6 +93,8 @@ export interface ChangeReviewSlice {
hunkContextHashesByFile: Record<string, Record<number, string>>; hunkContextHashesByFile: Record<string, Record<number, string>>;
fileContents: Record<string, FileChangeWithContent>; fileContents: Record<string, FileChangeWithContent>;
fileContentsLoading: Record<string, boolean>; fileContentsLoading: Record<string, boolean>;
changeSetEpoch: number;
fileContentVersionByPath: Record<string, number>;
collapseUnchanged: boolean; collapseUnchanged: boolean;
applyError: string | null; applyError: string | null;
applying: boolean; applying: boolean;
@ -270,10 +276,83 @@ function buildHunkContextHashesForFile(
return out; 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<AppState, [], [], ChangeReviewSlice> = ( export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeReviewSlice> = (
set, set,
get get
) => { ) => {
const installActiveChangeSetForLoad = (
data: ReviewChangeSet,
extraState?: Partial<ChangeReviewSlice>
): 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 ( const revalidateTaskChangePresence = async (
teamName: string, teamName: string,
taskId: string, taskId: string,
@ -326,6 +405,8 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
hunkContextHashesByFile: {}, hunkContextHashesByFile: {},
fileContents: {}, fileContents: {},
fileContentsLoading: {}, fileContentsLoading: {},
changeSetEpoch: 0,
fileContentVersionByPath: {},
collapseUnchanged: true, collapseUnchanged: true,
applyError: null, applyError: null,
applying: false, applying: false,
@ -339,11 +420,7 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
set({ changeSetLoading: true, changeSetError: null }); set({ changeSetLoading: true, changeSetError: null });
try { try {
const data = await api.review.getAgentChanges(teamName, memberName); const data = await api.review.getAgentChanges(teamName, memberName);
set({ installActiveChangeSetForLoad(data, { activeTaskChangeRequestOptions: null });
activeChangeSet: data,
changeSetLoading: false,
selectedReviewFilePath: data.files[0]?.filePath ?? null,
});
} catch (error) { } catch (error) {
const message = error instanceof Error ? error.message : 'Failed to fetch agent changes'; const message = error instanceof Error ? error.message : 'Failed to fetch agent changes';
logger.error('fetchAgentChanges error:', message); logger.error('fetchAgentChanges error:', message);
@ -379,13 +456,10 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
const data = await api.review.getTaskChanges(teamName, taskId, options); const data = await api.review.getTaskChanges(teamName, taskId, options);
if (requestToken !== latestTaskChangesRequestToken) return; if (requestToken !== latestTaskChangesRequestToken) return;
const cacheKey = buildTaskChangePresenceKey(teamName, taskId, options); const cacheKey = buildTaskChangePresenceKey(teamName, taskId, options);
set((s) => ({ installActiveChangeSetForLoad(data, {
activeChangeSet: data,
activeTaskChangeRequestOptions: options, activeTaskChangeRequestOptions: options,
changeSetLoading: false, taskHasChanges: { ...get().taskHasChanges, [cacheKey]: data.files.length > 0 },
selectedReviewFilePath: data.files[0]?.filePath ?? null, });
taskHasChanges: { ...s.taskHasChanges, [cacheKey]: data.files.length > 0 },
}));
if (data.files.length > 0) { if (data.files.length > 0) {
taskChangesNegativeCache.delete(cacheKey); taskChangesNegativeCache.delete(cacheKey);
} else { } else {
@ -405,7 +479,7 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
clearChangeReview: () => { clearChangeReview: () => {
latestTaskChangesRequestToken++; latestTaskChangesRequestToken++;
set({ set((s) => ({
activeChangeSet: null, activeChangeSet: null,
changeSetLoading: false, changeSetLoading: false,
changeSetError: null, changeSetError: null,
@ -418,15 +492,17 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
hunkContextHashesByFile: {}, hunkContextHashesByFile: {},
fileContents: {}, fileContents: {},
fileContentsLoading: {}, fileContentsLoading: {},
changeSetEpoch: s.changeSetEpoch + 1,
fileContentVersionByPath: {},
applyError: null, applyError: null,
applying: false, applying: false,
editedContents: {}, editedContents: {},
}); }));
}, },
clearChangeReviewCache: () => { clearChangeReviewCache: () => {
latestTaskChangesRequestToken++; latestTaskChangesRequestToken++;
set({ set((s) => ({
activeChangeSet: null, activeChangeSet: null,
changeSetLoading: false, changeSetLoading: false,
changeSetError: null, changeSetError: null,
@ -437,15 +513,17 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
hunkContextHashesByFile: {}, hunkContextHashesByFile: {},
fileContents: {}, fileContents: {},
fileContentsLoading: {}, fileContentsLoading: {},
changeSetEpoch: s.changeSetEpoch + 1,
fileContentVersionByPath: {},
applyError: null, applyError: null,
applying: false, applying: false,
editedContents: {}, editedContents: {},
}); }));
}, },
resetAllReviewState: () => { resetAllReviewState: () => {
latestTaskChangesRequestToken++; latestTaskChangesRequestToken++;
set({ set((s) => ({
activeChangeSet: null, activeChangeSet: null,
changeSetLoading: false, changeSetLoading: false,
changeSetError: null, changeSetError: null,
@ -458,10 +536,12 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
hunkContextHashesByFile: {}, hunkContextHashesByFile: {},
fileContents: {}, fileContents: {},
fileContentsLoading: {}, fileContentsLoading: {},
changeSetEpoch: s.changeSetEpoch + 1,
fileContentVersionByPath: {},
applyError: null, applyError: null,
applying: false, applying: false,
editedContents: {}, editedContents: {},
}); }));
}, },
// ── Decision persistence ── // ── Decision persistence ──
@ -693,6 +773,8 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
const state = get(); const state = get();
// Skip if already loaded or loading // Skip if already loaded or loading
if (state.fileContents[filePath] || state.fileContentsLoading[filePath]) return; if (state.fileContents[filePath] || state.fileContentsLoading[filePath]) return;
const changeSetEpoch = state.changeSetEpoch;
const fileVersion = state.fileContentVersionByPath[filePath] ?? 0;
set((s) => ({ set((s) => ({
fileContentsLoading: { ...s.fileContentsLoading, [filePath]: true }, fileContentsLoading: { ...s.fileContentsLoading, [filePath]: true },
@ -705,6 +787,9 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
const snippets = fileEntry?.snippets ?? []; const snippets = fileEntry?.snippets ?? [];
const content = await api.review.getFileContent(teamName, memberName, filePath, snippets); const content = await api.review.getFileContent(teamName, memberName, filePath, snippets);
const latest = get();
if (changeSetEpoch !== latest.changeSetEpoch) return;
if ((latest.fileContentVersionByPath[filePath] ?? 0) !== fileVersion) return;
set((s) => { set((s) => {
const result: Partial<ChangeReviewSlice> = { const result: Partial<ChangeReviewSlice> = {
fileContents: { ...s.fileContents, [filePath]: content }, fileContents: { ...s.fileContents, [filePath]: content },
@ -735,6 +820,9 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
return result; return result;
}); });
} catch (error) { } catch (error) {
const latest = get();
if (changeSetEpoch !== latest.changeSetEpoch) return;
if ((latest.fileContentVersionByPath[filePath] ?? 0) !== fileVersion) return;
logger.error('fetchFileContent error:', error); logger.error('fetchFileContent error:', error);
set((s) => ({ set((s) => ({
fileContentsLoading: { ...s.fileContentsLoading, [filePath]: false }, fileContentsLoading: { ...s.fileContentsLoading, [filePath]: false },
@ -749,20 +837,16 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
// Stale check: re-fetch changes and compare content fingerprint // Stale check: re-fetch changes and compare content fingerprint
const state = get(); const state = get();
const current = state.activeChangeSet; const current = state.activeChangeSet;
// Fingerprint uses file count + file paths only (not line counts) const currentFingerprint = current
// because line counts may be corrected by lazy-loaded content resolution ? fingerprintChangeSet(current as ReviewChangeSet)
const fingerprint = (cs: { totalFiles: number; files: { filePath: string }[] }): string => : null;
`${cs.totalFiles}:${cs.files.map((f) => f.filePath).join(',')}`; const staleMessage =
'Changes have been updated since you started reviewing. Please re-review.';
if (memberName && current) { if (memberName && current) {
const fresh = await api.review.getAgentChanges(teamName, memberName); const fresh = await api.review.getAgentChanges(teamName, memberName);
if (fingerprint(fresh) !== fingerprint(current)) { if (currentFingerprint !== fingerprintChangeSet(fresh)) {
set({ replaceActiveChangeSetAfterStaleRefresh(fresh, staleMessage);
activeChangeSet: fresh,
applying: false,
applyError:
'Changes have been updated since you started reviewing. Please re-review.',
});
return; return;
} }
} else if (taskId && current) { } else if (taskId && current) {
@ -770,13 +854,8 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
...(state.activeTaskChangeRequestOptions ?? {}), ...(state.activeTaskChangeRequestOptions ?? {}),
forceFresh: true, forceFresh: true,
}); });
if (fingerprint(fresh) !== fingerprint(current)) { if (currentFingerprint !== fingerprintChangeSet(fresh)) {
set({ replaceActiveChangeSetAfterStaleRefresh(fresh, staleMessage);
activeChangeSet: fresh,
applying: false,
applyError:
'Changes have been updated since you started reviewing. Please re-review.',
});
return; return;
} }
} }
@ -952,6 +1031,11 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
const nextHashes = { ...s.hunkContextHashesByFile }; const nextHashes = { ...s.hunkContextHashesByFile };
delete nextHashes[filePath]; delete nextHashes[filePath];
const nextFileContentVersionByPath = {
...s.fileContentVersionByPath,
[filePath]: (s.fileContentVersionByPath[filePath] ?? 0) + 1,
};
const nextSelected = const nextSelected =
s.selectedReviewFilePath === filePath s.selectedReviewFilePath === filePath
? (nextFiles[0]?.filePath ?? null) ? (nextFiles[0]?.filePath ?? null)
@ -973,6 +1057,7 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
fileContentsLoading: nextFileContentsLoading, fileContentsLoading: nextFileContentsLoading,
editedContents: nextEditedContents, editedContents: nextEditedContents,
hunkContextHashesByFile: nextHashes, hunkContextHashesByFile: nextHashes,
fileContentVersionByPath: nextFileContentVersionByPath,
}; };
}); });
}, },
@ -1004,6 +1089,11 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
? { ...s.fileContentsLoading, [file.filePath]: false } ? { ...s.fileContentsLoading, [file.filePath]: false }
: s.fileContentsLoading; : s.fileContentsLoading;
const nextFileContentVersionByPath = {
...s.fileContentVersionByPath,
[file.filePath]: s.fileContentVersionByPath[file.filePath] ?? 0,
};
return { return {
activeChangeSet: { activeChangeSet: {
...s.activeChangeSet, ...s.activeChangeSet,
@ -1015,6 +1105,7 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
selectedReviewFilePath: s.selectedReviewFilePath ?? file.filePath, selectedReviewFilePath: s.selectedReviewFilePath ?? file.filePath,
fileContents: nextFileContents, fileContents: nextFileContents,
fileContentsLoading: nextFileContentsLoading, fileContentsLoading: nextFileContentsLoading,
fileContentVersionByPath: nextFileContentVersionByPath,
}; };
}); });
}, },
@ -1046,6 +1137,14 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
const nextEditedContents = { ...s.editedContents }; const nextEditedContents = { ...s.editedContents };
delete nextEditedContents[filePath]; delete nextEditedContents[filePath];
const nextHunkContextHashesByFile = { ...s.hunkContextHashesByFile };
delete nextHunkContextHashesByFile[filePath];
const nextFileContentVersionByPath = {
...s.fileContentVersionByPath,
[filePath]: (s.fileContentVersionByPath[filePath] ?? 0) + 1,
};
return { return {
hunkDecisions: nextHunkDecisions, hunkDecisions: nextHunkDecisions,
fileDecisions: nextFileDecisions, fileDecisions: nextFileDecisions,
@ -1053,6 +1152,8 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
fileContents: nextFileContents, fileContents: nextFileContents,
fileContentsLoading: nextFileContentsLoading, fileContentsLoading: nextFileContentsLoading,
editedContents: nextEditedContents, editedContents: nextEditedContents,
hunkContextHashesByFile: nextHunkContextHashesByFile,
fileContentVersionByPath: nextFileContentVersionByPath,
}; };
}); });
}, },
@ -1078,12 +1179,27 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
saveEditedFile: async (filePath: string, projectPath?: string) => { saveEditedFile: async (filePath: string, projectPath?: string) => {
const content = get().editedContents[filePath]; const content = get().editedContents[filePath];
if (!(filePath in get().editedContents)) return; 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 { try {
await api.review.saveEditedFile(filePath, content, projectPath); await api.review.saveEditedFile(filePath, content, projectPath);
set((s) => { set((s) => {
const nextEdited = { ...s.editedContents }; const nextEdited = { ...s.editedContents };
delete nextEdited[filePath]; 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. // Update cached content in-place to avoid skeleton flash.
// Replace modifiedFullContent with saved version so CodeMirror // Replace modifiedFullContent with saved version so CodeMirror
// reflects the new baseline without a full re-fetch cycle. // reflects the new baseline without a full re-fetch cycle.
@ -1096,7 +1212,13 @@ export const createChangeReviewSlice: StateCreator<AppState, [], [], ChangeRevie
contentSource: 'disk-current', contentSource: 'disk-current',
}; };
} }
return { editedContents: nextEdited, fileContents: nextContents, applying: false }; return {
editedContents: nextEdited,
fileChunkCounts: nextFileChunkCounts,
hunkContextHashesByFile: nextHunkContextHashesByFile,
fileContents: nextContents,
applying: false,
};
}); });
} catch (error) { } catch (error) {
set({ applying: false, applyError: mapReviewError(error) }); set({ applying: false, applyError: mapReviewError(error) });

View file

@ -38,3 +38,32 @@ export function createCliAutoSuffixNameGuard(
return !allLower.has(info.base.toLowerCase()); return !allLower.has(info.base.toLowerCase());
}; };
} }
const PROVISIONER_SUFFIX = '-provisioner';
/**
* Claude CLI creates temporary "{name}-provisioner" agents during team provisioning
* to spawn real teammates. These are internal artifacts and should be hidden when
* the real base member (e.g. "alice") also exists.
*
* Only removes "alice-provisioner" if "alice" is present if the base is missing,
* the provisioner entry is kept for visibility.
*/
export function createCliProvisionerNameGuard(
allNames: Iterable<string>
): (name: string) => boolean {
const allLower = new Set<string>();
for (const n of allNames) {
if (typeof n !== 'string') continue;
const t = n.trim().toLowerCase();
if (t) allLower.add(t);
}
return (name: string): boolean => {
const lower = name.trim().toLowerCase();
if (!lower.endsWith(PROVISIONER_SUFFIX)) return true;
const base = lower.slice(0, -PROVISIONER_SUFFIX.length);
if (!base) return true;
return !allLower.has(base);
};
}

View file

@ -1,4 +1,4 @@
import { describe, expect, it, vi } from 'vitest'; import { beforeEach, describe, expect, it, vi } from 'vitest';
import type { SnippetDiff } from '@shared/types'; import type { SnippetDiff } from '@shared/types';
@ -16,6 +16,12 @@ vi.mock('fs/promises', async (importOriginal) => {
}); });
describe('FileContentResolver', () => { describe('FileContentResolver', () => {
beforeEach(() => {
vi.clearAllMocks();
vi.resetModules();
vi.useRealTimers();
});
it('treats empty on-disk content as valid for write-new reconstruction', async () => { it('treats empty on-disk content as valid for write-new reconstruction', async () => {
const fsPromises = await import('fs/promises'); const fsPromises = await import('fs/promises');
const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>; const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>;
@ -49,4 +55,179 @@ describe('FileContentResolver', () => {
expect(content.modifiedFullContent).toBe(''); expect(content.modifiedFullContent).toBe('');
expect(content.contentSource).toBe('snippet-reconstruction'); expect(content.contentSource).toBe('snippet-reconstruction');
}); });
it('reuses cached content only when disk bytes and snippets are unchanged', async () => {
const fsPromises = await import('fs/promises');
const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>;
readFile.mockResolvedValue('alpha');
const { FileContentResolver } = await import('@main/services/team/FileContentResolver');
const logsFinder = {
findMemberLogPaths: vi.fn().mockResolvedValue([]),
};
const resolver = new FileContentResolver(logsFinder as any);
await resolver.resolveFileContent('team', 'member', '/tmp/cache-hit.txt', []);
await resolver.resolveFileContent('team', 'member', '/tmp/cache-hit.txt', []);
expect(logsFinder.findMemberLogPaths).toHaveBeenCalledTimes(1);
});
it('misses cache when disk content changes even if snippets stay the same', async () => {
const fsPromises = await import('fs/promises');
const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>;
readFile.mockResolvedValueOnce('alpha').mockResolvedValueOnce('beta');
const { FileContentResolver } = await import('@main/services/team/FileContentResolver');
const logsFinder = {
findMemberLogPaths: vi.fn().mockResolvedValue([]),
};
const resolver = new FileContentResolver(logsFinder as any);
await resolver.resolveFileContent('team', 'member', '/tmp/disk-change.txt', []);
await resolver.resolveFileContent('team', 'member', '/tmp/disk-change.txt', []);
expect(logsFinder.findMemberLogPaths).toHaveBeenCalledTimes(2);
});
it('misses cache when snippet fingerprint changes even if disk bytes stay the same', async () => {
const fsPromises = await import('fs/promises');
const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>;
readFile.mockResolvedValue('alpha');
const { FileContentResolver } = await import('@main/services/team/FileContentResolver');
const logsFinder = {
findMemberLogPaths: vi.fn().mockResolvedValue([]),
};
const resolver = new FileContentResolver(logsFinder as any);
const firstSnippets: SnippetDiff[] = [];
const secondSnippets: SnippetDiff[] = [
{
toolUseId: 't-edit',
filePath: '/tmp/snippet-change.txt',
toolName: 'Edit',
type: 'edit',
oldString: 'before',
newString: 'after',
replaceAll: false,
timestamp: '2026-03-01T10:00:00.000Z',
isError: false,
},
];
await resolver.resolveFileContent('team', 'member', '/tmp/snippet-change.txt', firstSnippets);
await resolver.resolveFileContent('team', 'member', '/tmp/snippet-change.txt', secondSnippets);
expect(logsFinder.findMemberLogPaths).toHaveBeenCalledTimes(2);
});
it('misses cache when snippet order changes even if snippet content stays the same', async () => {
const fsPromises = await import('fs/promises');
const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>;
readFile.mockResolvedValue('alpha');
const { FileContentResolver } = await import('@main/services/team/FileContentResolver');
const logsFinder = {
findMemberLogPaths: vi.fn().mockResolvedValue([]),
};
const resolver = new FileContentResolver(logsFinder as any);
const firstSnippets: SnippetDiff[] = [
{
toolUseId: 't-1',
filePath: '/tmp/snippet-order.txt',
toolName: 'Edit',
type: 'edit',
oldString: 'a',
newString: 'b',
replaceAll: false,
timestamp: '2026-03-01T10:00:00.000Z',
isError: false,
},
{
toolUseId: 't-2',
filePath: '/tmp/snippet-order.txt',
toolName: 'Edit',
type: 'edit',
oldString: 'c',
newString: 'd',
replaceAll: false,
timestamp: '2026-03-01T10:01:00.000Z',
isError: false,
},
];
const reversedSnippets = [...firstSnippets].reverse();
await resolver.resolveFileContent('team', 'member', '/tmp/snippet-order.txt', firstSnippets);
await resolver.resolveFileContent('team', 'member', '/tmp/snippet-order.txt', reversedSnippets);
expect(logsFinder.findMemberLogPaths).toHaveBeenCalledTimes(2);
});
it('distinguishes missing-file fingerprints from empty-file fingerprints', async () => {
const fsPromises = await import('fs/promises');
const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>;
readFile.mockRejectedValueOnce(new Error('ENOENT')).mockResolvedValueOnce('');
const { FileContentResolver } = await import('@main/services/team/FileContentResolver');
const logsFinder = {
findMemberLogPaths: vi.fn().mockResolvedValue([]),
};
const resolver = new FileContentResolver(logsFinder as any);
const missing = await resolver.resolveFileContent('team', 'member', '/tmp/missing-vs-empty.txt', []);
const empty = await resolver.resolveFileContent('team', 'member', '/tmp/missing-vs-empty.txt', []);
expect(missing.source).toBe('unavailable');
expect(empty.source).toBe('disk-current');
expect(logsFinder.findMemberLogPaths).toHaveBeenCalledTimes(2);
});
it('uses the same provisional TTL for all content sources in this pass', async () => {
const { FileContentResolver } = await import('@main/services/team/FileContentResolver');
const resolver = new FileContentResolver({ findMemberLogPaths: vi.fn() } as any);
const getCacheTtlForSource = (
resolver as unknown as {
getCacheTtlForSource: (source: string) => number;
}
).getCacheTtlForSource.bind(resolver);
expect(getCacheTtlForSource('file-history')).toBe(5_000);
expect(getCacheTtlForSource('snippet-reconstruction')).toBe(5_000);
expect(getCacheTtlForSource('git-fallback')).toBe(5_000);
expect(getCacheTtlForSource('disk-current')).toBe(5_000);
expect(getCacheTtlForSource('unavailable')).toBe(5_000);
});
it('expires provisional cache entries after the short TTL window', async () => {
vi.useFakeTimers();
const fsPromises = await import('fs/promises');
const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>;
readFile.mockResolvedValue('alpha');
const { FileContentResolver } = await import('@main/services/team/FileContentResolver');
const logsFinder = {
findMemberLogPaths: vi.fn().mockResolvedValue([]),
};
const resolver = new FileContentResolver(logsFinder as any);
await resolver.resolveFileContent('team', 'member', '/tmp/ttl-expiry.txt', []);
vi.advanceTimersByTime(5_001);
await resolver.resolveFileContent('team', 'member', '/tmp/ttl-expiry.txt', []);
expect(logsFinder.findMemberLogPaths).toHaveBeenCalledTimes(2);
});
}); });

View file

@ -5,6 +5,65 @@ import { TASK_COMMENT_FORWARDING_ENV } from '../../../../src/main/services/team/
import type { TeamTask } from '../../../../src/shared/types/team'; import type { TeamTask } from '../../../../src/shared/types/team';
function createForwardingJournalStore(initialEntries: Array<Record<string, unknown>> = []) {
const journalEntries = initialEntries;
const journal = {
exists: vi.fn(async () => true),
ensureFile: vi.fn(async () => undefined),
withEntries: vi.fn(async (_teamName: string, fn: (entries: unknown[]) => Promise<{ result: unknown }>) => {
const outcome = await fn(journalEntries);
return outcome.result;
}),
};
return { journalEntries, journal };
}
function createTaskCommentForwardingService(options: {
tasks: TeamTask[];
inboxWriter?: { sendMessage: ReturnType<typeof vi.fn> };
inboxMessagesForLead?: Array<Record<string, unknown>>;
journal?: {
exists: ReturnType<typeof vi.fn>;
ensureFile: ReturnType<typeof vi.fn>;
withEntries: ReturnType<typeof vi.fn>;
};
members?: Array<{ name: string; role?: string }>;
}) {
const inboxWriter = options.inboxWriter ?? { sendMessage: vi.fn(async () => ({ deliveredToInbox: true, messageId: 'msg-1' })) };
const journal = options.journal ?? createForwardingJournalStore().journal;
const service = new TeamDataService(
{
listTeams: vi.fn(),
getConfig: vi.fn(async () => ({
name: 'My team',
members: options.members ?? [{ name: 'team-lead', role: 'Lead' }],
leadSessionId: 'lead-1',
})),
} as never,
{
getTasks: vi.fn(async () => options.tasks),
} as never,
{
listInboxNames: vi.fn(async () => []),
getMessages: vi.fn(async () => []),
getMessagesFor: vi.fn(async () => options.inboxMessagesForLead ?? []),
} as never,
inboxWriter as never,
{} as never,
{} as never,
{} as never,
{} as never,
{} as never,
{} as never,
(() => ({}) as never) as never,
journal as never
);
return { service, inboxWriter, journal };
}
describe('TeamDataService', () => { describe('TeamDataService', () => {
it('keeps getTeamData read-only and skips kanban garbage-collect', async () => { it('keeps getTeamData read-only and skips kanban garbage-collect', async () => {
const order: string[] = []; const order: string[] = [];
@ -681,7 +740,7 @@ describe('TeamDataService', () => {
expect.objectContaining({ expect.objectContaining({
member: 'team-lead', member: 'team-lead',
from: 'alice', from: 'alice',
summary: '**Comment on** #abcd1234', summary: 'Comment on #abcd1234',
source: 'system_notification', source: 'system_notification',
leadSessionId: 'lead-1', leadSessionId: 'lead-1',
taskRefs: [{ taskId: 'task-1', displayId: 'abcd1234', teamName: 'my-team' }], taskRefs: [{ taskId: 'task-1', displayId: 'abcd1234', teamName: 'my-team' }],
@ -1480,7 +1539,7 @@ describe('TeamDataService', () => {
'my-team', 'my-team',
expect.objectContaining({ expect.objectContaining({
from: 'bob', from: 'bob',
summary: '**Comment on** #abcd1234', summary: 'Comment on #abcd1234',
messageId: 'task-comment-forward:my-team:task-1:comment-2', messageId: 'task-comment-forward:my-team:task-1:comment-2',
}) })
); );
@ -1490,6 +1549,165 @@ describe('TeamDataService', () => {
} }
}); });
it('does not forward user-authored, lead-authored, mirrored, or non-regular comments', async () => {
const previous = process.env[TASK_COMMENT_FORWARDING_ENV];
process.env[TASK_COMMENT_FORWARDING_ENV] = 'on';
try {
const { journalEntries, journal } = createForwardingJournalStore();
const { service, inboxWriter } = createTaskCommentForwardingService({
journal,
tasks: [
{
id: 'task-1',
displayId: 'abcd1234',
subject: 'Investigate',
status: 'pending',
owner: 'alice',
comments: [
{
id: 'comment-user',
author: 'user',
text: 'User comment should not notify.',
createdAt: '2026-03-14T10:00:00.000Z',
type: 'regular',
},
{
id: 'comment-lead',
author: 'team-lead',
text: 'Lead already knows this.',
createdAt: '2026-03-14T10:01:00.000Z',
type: 'regular',
},
{
id: 'msg-legacy',
author: 'alice',
text: 'Mirrored inbox artifact.',
createdAt: '2026-03-14T10:02:00.000Z',
type: 'regular',
},
{
id: 'comment-review-request',
author: 'alice',
text: 'Please review.',
createdAt: '2026-03-14T10:03:00.000Z',
type: 'review_request',
},
{
id: 'comment-review-approved',
author: 'alice',
text: 'Approved.',
createdAt: '2026-03-14T10:04:00.000Z',
type: 'review_approved',
},
],
},
],
});
await service.notifyLeadOnTeammateTaskComment('my-team', 'task-1');
expect(inboxWriter.sendMessage).not.toHaveBeenCalled();
expect(journalEntries).toEqual([]);
} finally {
if (previous === undefined) delete process.env[TASK_COMMENT_FORWARDING_ENV];
else process.env[TASK_COMMENT_FORWARDING_ENV] = previous;
}
});
it('does not forward comments for lead-owned tasks', async () => {
const previous = process.env[TASK_COMMENT_FORWARDING_ENV];
process.env[TASK_COMMENT_FORWARDING_ENV] = 'on';
try {
const { journalEntries, journal } = createForwardingJournalStore();
const { service, inboxWriter } = createTaskCommentForwardingService({
journal,
tasks: [
{
id: 'task-1',
displayId: 'abcd1234',
subject: 'Lead-owned task',
status: 'pending',
owner: 'team-lead',
comments: [
{
id: 'comment-1',
author: 'alice',
text: 'Should not create a second lead notification.',
createdAt: '2026-03-14T10:00:00.000Z',
type: 'regular',
},
],
},
],
});
await service.notifyLeadOnTeammateTaskComment('my-team', 'task-1');
expect(inboxWriter.sendMessage).not.toHaveBeenCalled();
expect(journalEntries).toEqual([]);
} finally {
if (previous === undefined) delete process.env[TASK_COMMENT_FORWARDING_ENV];
else process.env[TASK_COMMENT_FORWARDING_ENV] = previous;
}
});
it('does not replay historical comment notifications after lead rename because the journal key is team-level', async () => {
const previous = process.env[TASK_COMMENT_FORWARDING_ENV];
process.env[TASK_COMMENT_FORWARDING_ENV] = 'on';
try {
const { journalEntries, journal } = createForwardingJournalStore([
{
key: 'task-1:comment-1',
taskId: 'task-1',
commentId: 'comment-1',
author: 'alice',
messageId: 'task-comment-forward:my-team:task-1:comment-1',
state: 'sent',
createdAt: '2026-03-14T10:00:00.000Z',
updatedAt: '2026-03-14T10:00:00.000Z',
sentAt: '2026-03-14T10:00:00.000Z',
},
]);
const { service, inboxWriter } = createTaskCommentForwardingService({
journal,
members: [{ name: 'new-lead', role: 'Lead' }],
tasks: [
{
id: 'task-1',
displayId: 'abcd1234',
subject: 'Investigate',
status: 'pending',
owner: 'alice',
comments: [
{
id: 'comment-1',
author: 'alice',
text: 'Already forwarded before lead rename.',
createdAt: '2026-03-14T10:00:00.000Z',
type: 'regular',
},
],
},
],
});
await service.notifyLeadOnTeammateTaskComment('my-team', 'task-1');
expect(inboxWriter.sendMessage).not.toHaveBeenCalled();
expect(journalEntries).toHaveLength(1);
expect(journalEntries[0]).toMatchObject({
key: 'task-1:comment-1',
state: 'sent',
});
} finally {
if (previous === undefined) delete process.env[TASK_COMMENT_FORWARDING_ENV];
else process.env[TASK_COMMENT_FORWARDING_ENV] = previous;
}
});
it('waits for startup initialization before processing watcher-driven comment notifications', async () => { it('waits for startup initialization before processing watcher-driven comment notifications', async () => {
const previous = process.env[TASK_COMMENT_FORWARDING_ENV]; const previous = process.env[TASK_COMMENT_FORWARDING_ENV];
process.env[TASK_COMMENT_FORWARDING_ENV] = 'on'; process.env[TASK_COMMENT_FORWARDING_ENV] = 'on';

View file

@ -55,6 +55,83 @@ async function flushAsyncWork(): Promise<void> {
await Promise.resolve(); await Promise.resolve();
} }
function makeSnippet(
overrides: Partial<{
toolUseId: string;
filePath: string;
toolName: string;
type: 'edit' | 'multi-edit' | 'write-new' | 'write-update';
oldString: string;
newString: string;
replaceAll: boolean;
timestamp: string;
isError: boolean;
contextHash: string;
}> = {}
) {
return {
toolUseId: 'tool-1',
filePath: '/repo/file.ts',
toolName: 'Edit',
type: 'edit' as const,
oldString: 'before',
newString: 'after',
replaceAll: false,
timestamp: '2026-03-01T10:00:00.000Z',
isError: false,
...overrides,
};
}
function makeFile(filePath = '/repo/file.ts', snippetOverrides = {}) {
return {
filePath,
relativePath: filePath.split('/').pop() ?? 'file.ts',
snippets: [makeSnippet({ filePath, ...snippetOverrides })],
linesAdded: 1,
linesRemoved: 1,
isNewFile: false,
};
}
function makeAgentChangeSet(filePath = '/repo/file.ts', snippetOverrides = {}) {
const file = makeFile(filePath, snippetOverrides);
return {
memberName: 'alice',
teamName: 'team-a',
files: [file],
totalFiles: 1,
totalLinesAdded: file.linesAdded,
totalLinesRemoved: file.linesRemoved,
};
}
function makeTaskChangeSet(taskId = 'task-1', filePath = '/repo/file.ts', snippetOverrides = {}) {
const file = makeFile(filePath, snippetOverrides);
return {
teamName: 'team-a',
taskId,
files: [file],
totalFiles: 1,
totalLinesAdded: file.linesAdded,
totalLinesRemoved: file.linesRemoved,
confidence: 'fallback',
computedAt: '2026-03-01T12:00:00.000Z',
scope: {
taskId,
memberName: 'alice',
startLine: 0,
endLine: 0,
startTimestamp: '',
endTimestamp: '',
toolUseIds: [],
filePaths: [filePath],
confidence: { tier: 4, label: 'fallback', reason: 'test fixture' },
},
warnings: [],
};
}
const OPTIONS_A = { const OPTIONS_A = {
owner: 'alice', owner: 'alice',
status: 'completed', status: 'completed',
@ -392,4 +469,340 @@ describe('changeReviewSlice task changes', () => {
}); });
expect(store.getState().taskHasChanges[buildTaskChangePresenceKey(teamName, taskId, OPTIONS_A)]).toBe(false); expect(store.getState().taskHasChanges[buildTaskChangePresenceKey(teamName, taskId, OPTIONS_A)]).toBe(false);
}); });
it('clears resolved file content state when fetchAgentChanges installs a new change set', async () => {
const store = createSliceStore();
const data = makeAgentChangeSet('/repo/new.ts');
hoisted.getAgentChanges.mockResolvedValue(data);
store.setState({
hunkDecisions: { '/repo/file.ts:0': 'rejected' },
fileContents: {
'/repo/file.ts': {
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'after',
contentSource: 'snippet-reconstruction',
},
},
fileContentsLoading: { '/repo/file.ts': true },
fileChunkCounts: { '/repo/file.ts': 3 },
hunkContextHashesByFile: { '/repo/file.ts': { 0: 'ctx' } },
changeSetEpoch: 4,
fileContentVersionByPath: { '/repo/file.ts': 2 },
});
await store.getState().fetchAgentChanges('team-a', 'alice');
expect(store.getState().activeChangeSet).toEqual(data);
expect(store.getState().selectedReviewFilePath).toBe('/repo/new.ts');
expect(store.getState().fileContents).toEqual({});
expect(store.getState().fileContentsLoading).toEqual({});
expect(store.getState().fileChunkCounts).toEqual({});
expect(store.getState().hunkContextHashesByFile).toEqual({});
expect(store.getState().hunkDecisions).toEqual({ '/repo/file.ts:0': 'rejected' });
expect(store.getState().changeSetEpoch).toBe(5);
expect(store.getState().fileContentVersionByPath).toEqual({});
});
it('clears resolved file content state when fetchTaskChanges installs a new change set', async () => {
const store = createSliceStore();
const data = makeTaskChangeSet('task-2', '/repo/task.ts');
hoisted.getTaskChanges.mockResolvedValue(data);
store.setState({
hunkDecisions: { '/repo/file.ts:0': 'accepted' },
fileContents: {
'/repo/file.ts': {
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'after',
contentSource: 'snippet-reconstruction',
},
},
fileContentsLoading: { '/repo/file.ts': true },
fileChunkCounts: { '/repo/file.ts': 2 },
hunkContextHashesByFile: { '/repo/file.ts': { 0: 'ctx' } },
changeSetEpoch: 1,
fileContentVersionByPath: { '/repo/file.ts': 7 },
});
await store.getState().fetchTaskChanges('team-a', 'task-2', OPTIONS_A);
expect(store.getState().activeChangeSet).toEqual(data);
expect(store.getState().activeTaskChangeRequestOptions).toEqual(OPTIONS_A);
expect(store.getState().selectedReviewFilePath).toBe('/repo/task.ts');
expect(store.getState().fileContents).toEqual({});
expect(store.getState().fileContentsLoading).toEqual({});
expect(store.getState().fileChunkCounts).toEqual({});
expect(store.getState().hunkContextHashesByFile).toEqual({});
expect(store.getState().hunkDecisions).toEqual({ '/repo/file.ts:0': 'accepted' });
expect(store.getState().changeSetEpoch).toBe(2);
expect(store.getState().fileContentVersionByPath).toEqual({});
});
it('re-fetches visible file content after change-set replacement instead of silently reusing stale content', async () => {
const store = createSliceStore();
const refreshed = makeAgentChangeSet('/repo/file.ts', { newString: 'after-v2' });
hoisted.getAgentChanges.mockResolvedValueOnce(refreshed);
hoisted.getFileContent.mockResolvedValueOnce({
...makeFile('/repo/file.ts', { newString: 'after-v2' }),
originalFullContent: 'before',
modifiedFullContent: 'after-v2',
contentSource: 'snippet-reconstruction',
});
store.setState({
activeChangeSet: makeAgentChangeSet('/repo/file.ts'),
fileContents: {
'/repo/file.ts': {
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'after',
contentSource: 'snippet-reconstruction',
},
},
fileContentsLoading: {},
changeSetEpoch: 0,
fileContentVersionByPath: {},
});
await store.getState().fetchAgentChanges('team-a', 'alice');
expect(store.getState().fileContents).toEqual({});
await store.getState().fetchFileContent('team-a', 'alice', '/repo/file.ts');
expect(hoisted.getFileContent).toHaveBeenCalledTimes(1);
expect(hoisted.getFileContent).toHaveBeenCalledWith(
'team-a',
'alice',
'/repo/file.ts',
refreshed.files[0]?.snippets ?? []
);
expect(store.getState().fileContents['/repo/file.ts']?.modifiedFullContent).toBe('after-v2');
});
it('ignores stale fetchFileContent responses after change-set replacement', async () => {
const store = createSliceStore();
const pending = deferred<any>();
hoisted.getFileContent.mockReturnValueOnce(pending.promise);
hoisted.getAgentChanges.mockResolvedValueOnce(makeAgentChangeSet('/repo/next.ts'));
store.setState({
activeChangeSet: makeAgentChangeSet('/repo/file.ts'),
hunkContextHashesByFile: { '/repo/file.ts': { 0: 'ctx' } },
changeSetEpoch: 0,
fileContentVersionByPath: {},
});
const fetchPromise = store.getState().fetchFileContent('team-a', 'alice', '/repo/file.ts');
await flushAsyncWork();
await store.getState().fetchAgentChanges('team-a', 'alice');
pending.resolve({
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'after',
contentSource: 'snippet-reconstruction',
});
await fetchPromise;
await flushAsyncWork();
expect(store.getState().selectedReviewFilePath).toBe('/repo/next.ts');
expect(store.getState().fileContents).toEqual({});
expect(store.getState().fileContentsLoading).toEqual({});
});
it('ignores stale fetchFileContent responses after per-file invalidation', async () => {
const store = createSliceStore();
const pending = deferred<any>();
hoisted.getFileContent.mockReturnValueOnce(pending.promise);
store.setState({
activeChangeSet: makeAgentChangeSet('/repo/file.ts'),
changeSetEpoch: 0,
fileContentVersionByPath: {},
});
const fetchPromise = store.getState().fetchFileContent('team-a', 'alice', '/repo/file.ts');
await flushAsyncWork();
store.getState().clearReviewStateForFile('/repo/file.ts');
pending.resolve({
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'after',
contentSource: 'snippet-reconstruction',
});
await fetchPromise;
await flushAsyncWork();
expect(store.getState().fileContents).toEqual({});
expect(store.getState().fileContentsLoading).toEqual({});
expect(store.getState().hunkContextHashesByFile).toEqual({});
expect(store.getState().fileContentVersionByPath['/repo/file.ts']).toBe(1);
});
it('ignores stale fetchFileContent responses after removing a review file', async () => {
const store = createSliceStore();
const pending = deferred<any>();
hoisted.getFileContent.mockReturnValueOnce(pending.promise);
store.setState({
activeChangeSet: makeAgentChangeSet('/repo/file.ts'),
changeSetEpoch: 0,
fileContentVersionByPath: {},
});
const fetchPromise = store.getState().fetchFileContent('team-a', 'alice', '/repo/file.ts');
await flushAsyncWork();
store.getState().removeReviewFile('/repo/file.ts');
pending.resolve({
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'after',
contentSource: 'snippet-reconstruction',
});
await fetchPromise;
await flushAsyncWork();
expect(store.getState().activeChangeSet?.files).toEqual([]);
expect(store.getState().fileContents).toEqual({});
expect(store.getState().fileContentsLoading).toEqual({});
expect(store.getState().fileContentVersionByPath['/repo/file.ts']).toBe(1);
});
it('keeps restored file content when a stale fetch resolves after remove and re-add', async () => {
const store = createSliceStore();
const pending = deferred<any>();
hoisted.getFileContent.mockReturnValueOnce(pending.promise);
store.setState({
activeChangeSet: makeAgentChangeSet('/repo/file.ts'),
changeSetEpoch: 0,
fileContentVersionByPath: {},
});
const fetchPromise = store.getState().fetchFileContent('team-a', 'alice', '/repo/file.ts');
await flushAsyncWork();
store.getState().removeReviewFile('/repo/file.ts');
store.getState().addReviewFile(makeFile('/repo/file.ts'), {
index: 0,
content: {
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'restored',
contentSource: 'snippet-reconstruction',
},
});
pending.resolve({
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'stale',
contentSource: 'snippet-reconstruction',
});
await fetchPromise;
await flushAsyncWork();
expect(store.getState().activeChangeSet?.files).toHaveLength(1);
expect(store.getState().fileContents['/repo/file.ts']?.modifiedFullContent).toBe('restored');
expect(store.getState().fileContentVersionByPath['/repo/file.ts']).toBe(1);
});
it('ignores stale fetchFileContent responses that resolve after saveEditedFile', async () => {
const store = createSliceStore();
const fetchPending = deferred<any>();
const savePending = deferred<void>();
hoisted.getFileContent.mockReturnValueOnce(fetchPending.promise);
hoisted.saveEditedFile.mockReturnValueOnce(savePending.promise);
store.setState({
activeChangeSet: makeAgentChangeSet('/repo/file.ts'),
fileContents: {
'/repo/file.ts': {
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'draft-before-save',
contentSource: 'snippet-reconstruction',
},
},
fileContentsLoading: { '/repo/file.ts': true },
fileChunkCounts: { '/repo/file.ts': 3 },
hunkContextHashesByFile: { '/repo/file.ts': { 0: 'ctx' } },
editedContents: { '/repo/file.ts': 'saved-content' },
changeSetEpoch: 0,
fileContentVersionByPath: {},
});
const fetchPromise = store.getState().fetchFileContent('team-a', 'alice', '/repo/file.ts');
await flushAsyncWork();
const savePromise = store.getState().saveEditedFile('/repo/file.ts');
await flushAsyncWork();
savePending.resolve();
await savePromise;
fetchPending.resolve({
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'stale-after-save',
contentSource: 'snippet-reconstruction',
});
await fetchPromise;
await flushAsyncWork();
expect(store.getState().editedContents).toEqual({});
expect(store.getState().fileContents['/repo/file.ts']?.modifiedFullContent).toBe('saved-content');
expect(store.getState().fileContentsLoading['/repo/file.ts']).toBe(false);
expect(store.getState().fileChunkCounts).toEqual({});
expect(store.getState().hunkContextHashesByFile).toEqual({});
expect(store.getState().fileContentVersionByPath['/repo/file.ts']).toBe(1);
});
it('forces re-review when snippets change even if file paths stay the same', async () => {
const store = createSliceStore();
const current = makeAgentChangeSet('/repo/file.ts', { newString: 'after' });
const fresh = makeAgentChangeSet('/repo/file.ts', { newString: 'after-v2' });
hoisted.getAgentChanges.mockResolvedValueOnce(fresh);
store.setState({
activeChangeSet: current,
hunkDecisions: { '/repo/file.ts:0': 'rejected' },
fileDecisions: { '/repo/file.ts': 'rejected' },
fileChunkCounts: { '/repo/file.ts': 1 },
reviewUndoStack: [{ hunkDecisions: { '/repo/file.ts:0': 'rejected' }, fileDecisions: { '/repo/file.ts': 'rejected' } }],
hunkContextHashesByFile: { '/repo/file.ts': { 0: 'ctx' } },
fileContents: {
'/repo/file.ts': {
...makeFile('/repo/file.ts'),
originalFullContent: 'before',
modifiedFullContent: 'after',
contentSource: 'snippet-reconstruction',
},
},
fileContentsLoading: { '/repo/file.ts': false },
editedContents: { '/repo/file.ts': 'draft' },
changeSetEpoch: 2,
fileContentVersionByPath: { '/repo/file.ts': 3 },
});
await store.getState().applyReview('team-a', undefined, 'alice');
expect(hoisted.applyDecisions).not.toHaveBeenCalled();
expect(store.getState().activeChangeSet).toEqual(fresh);
expect(store.getState().applyError).toBe(
'Changes have been updated since you started reviewing. Please re-review.'
);
expect(store.getState().hunkDecisions).toEqual({});
expect(store.getState().fileDecisions).toEqual({});
expect(store.getState().reviewUndoStack).toEqual([]);
expect(store.getState().hunkContextHashesByFile).toEqual({});
expect(store.getState().fileContents).toEqual({});
expect(store.getState().fileContentsLoading).toEqual({});
expect(store.getState().editedContents).toEqual({});
expect(store.getState().changeSetEpoch).toBe(3);
expect(store.getState().fileContentVersionByPath).toEqual({});
});
}); });

View file

@ -1,6 +1,10 @@
import { describe, expect, it } from 'vitest'; import { describe, expect, it } from 'vitest';
import { createCliAutoSuffixNameGuard, parseNumericSuffixName } from '@shared/utils/teamMemberName'; import {
createCliAutoSuffixNameGuard,
createCliProvisionerNameGuard,
parseNumericSuffixName,
} from '@shared/utils/teamMemberName';
describe('teamMemberName helpers', () => { describe('teamMemberName helpers', () => {
it('parses numeric suffix names', () => { it('parses numeric suffix names', () => {
@ -36,3 +40,45 @@ describe('teamMemberName helpers', () => {
expect(keepName('alice-2')).toBe(false); expect(keepName('alice-2')).toBe(false);
}); });
}); });
describe('createCliProvisionerNameGuard', () => {
it('drops provisioner names when the base member exists', () => {
const keep = createCliProvisionerNameGuard([
'alice',
'alice-provisioner',
'bob',
'bob-provisioner',
]);
expect(keep('alice')).toBe(true);
expect(keep('alice-provisioner')).toBe(false);
expect(keep('bob')).toBe(true);
expect(keep('bob-provisioner')).toBe(false);
});
it('keeps provisioner names when the base member is absent', () => {
const keep = createCliProvisionerNameGuard(['carol-provisioner']);
expect(keep('carol-provisioner')).toBe(true);
});
it('treats base-name collisions case-insensitively', () => {
const keep = createCliProvisionerNameGuard(['Alice', 'alice-provisioner']);
expect(keep('alice-provisioner')).toBe(false);
});
it('keeps non-provisioner names unchanged', () => {
const keep = createCliProvisionerNameGuard(['alice', 'alice-provisioner', 'dev-1']);
expect(keep('alice')).toBe(true);
expect(keep('dev-1')).toBe(true);
});
it('handles empty and edge-case names', () => {
const keep = createCliProvisionerNameGuard(['', '-provisioner']);
expect(keep('')).toBe(true);
expect(keep('-provisioner')).toBe(true);
});
});