fix(changes): require strict ledger hunk rejects
This commit is contained in:
parent
642cea8857
commit
07a9f603de
2 changed files with 199 additions and 2 deletions
|
|
@ -293,6 +293,7 @@ export class ReviewApplierService {
|
||||||
decision.fileDecision === 'rejected',
|
decision.fileDecision === 'rejected',
|
||||||
allHunksRejected,
|
allHunksRejected,
|
||||||
rejectedHunkIndices,
|
rejectedHunkIndices,
|
||||||
|
decision.hunkContextHashes,
|
||||||
fileContent.snippets
|
fileContent.snippets
|
||||||
);
|
);
|
||||||
if (ledgerOutcome.handled) {
|
if (ledgerOutcome.handled) {
|
||||||
|
|
@ -450,6 +451,7 @@ export class ReviewApplierService {
|
||||||
fileRejected: boolean,
|
fileRejected: boolean,
|
||||||
allHunksRejected: boolean,
|
allHunksRejected: boolean,
|
||||||
rejectedHunkIndices: number[],
|
rejectedHunkIndices: number[],
|
||||||
|
hunkContextHashes: Record<number, string> | undefined,
|
||||||
snippets: SnippetDiff[]
|
snippets: SnippetDiff[]
|
||||||
): Promise<LedgerApplyOutcome> {
|
): Promise<LedgerApplyOutcome> {
|
||||||
const ledgerSnippets = snippets.filter((snippet) => snippet.ledger && !snippet.isError);
|
const ledgerSnippets = snippets.filter((snippet) => snippet.ledger && !snippet.isError);
|
||||||
|
|
@ -497,6 +499,20 @@ export class ReviewApplierService {
|
||||||
error: 'Ledger full text is unavailable; partial reject requires manual review.',
|
error: 'Ledger full text is unavailable; partial reject requires manual review.',
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
const strictHunks = mapRejectedHunkIndicesByHashStrict(
|
||||||
|
original,
|
||||||
|
modified,
|
||||||
|
rejectedHunkIndices,
|
||||||
|
hunkContextHashes
|
||||||
|
);
|
||||||
|
if (!strictHunks.ok) {
|
||||||
|
return {
|
||||||
|
handled: true,
|
||||||
|
status: strictHunks.code === 'conflict' ? 'conflict' : 'error',
|
||||||
|
code: strictHunks.code,
|
||||||
|
error: strictHunks.error,
|
||||||
|
};
|
||||||
|
}
|
||||||
const guard = await this.checkLedgerCurrentHash(
|
const guard = await this.checkLedgerCurrentHash(
|
||||||
filePath,
|
filePath,
|
||||||
lastLedger.afterState?.sha256 ?? lastLedger.afterHash ?? undefined
|
lastLedger.afterState?.sha256 ?? lastLedger.afterHash ?? undefined
|
||||||
|
|
@ -504,7 +520,7 @@ export class ReviewApplierService {
|
||||||
if (!guard.ok) {
|
if (!guard.ok) {
|
||||||
return guard.outcome;
|
return guard.outcome;
|
||||||
}
|
}
|
||||||
const patchResult = this.tryHunkLevelReject(original, modified, rejectedHunkIndices);
|
const patchResult = this.tryStrictHunkLevelReject(original, modified, strictHunks.indices);
|
||||||
if (!patchResult) {
|
if (!patchResult) {
|
||||||
return {
|
return {
|
||||||
handled: true,
|
handled: true,
|
||||||
|
|
@ -1035,6 +1051,46 @@ export class ReviewApplierService {
|
||||||
hadConflicts: false,
|
hadConflicts: false,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private tryStrictHunkLevelReject(
|
||||||
|
original: string,
|
||||||
|
modified: string,
|
||||||
|
hunkIndices: number[]
|
||||||
|
): RejectResult | null {
|
||||||
|
const patch = structuredPatch('file', 'file', original, modified);
|
||||||
|
|
||||||
|
if (!patch.hunks || patch.hunks.length === 0) return null;
|
||||||
|
|
||||||
|
const validIndices = hunkIndices.filter((idx) => idx >= 0 && idx < patch.hunks.length);
|
||||||
|
if (validIndices.length !== hunkIndices.length || validIndices.length === 0) return null;
|
||||||
|
|
||||||
|
const inversedHunks: StructuredPatchHunk[] = [];
|
||||||
|
for (const idx of validIndices) {
|
||||||
|
const hunk = patch.hunks[idx];
|
||||||
|
if (!hunk) return null;
|
||||||
|
inversedHunks.push(invertHunk(hunk));
|
||||||
|
}
|
||||||
|
|
||||||
|
const inversePatch = {
|
||||||
|
oldFileName: 'file',
|
||||||
|
newFileName: 'file',
|
||||||
|
oldHeader: undefined,
|
||||||
|
newHeader: undefined,
|
||||||
|
hunks: inversedHunks,
|
||||||
|
};
|
||||||
|
|
||||||
|
const result = applyPatch(modified, inversePatch, { fuzzFactor: 0 });
|
||||||
|
if (result === false) {
|
||||||
|
logger.debug('Strict ledger hunk-level inverse patch не удался');
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
|
return {
|
||||||
|
success: true,
|
||||||
|
newContent: result,
|
||||||
|
hadConflicts: false,
|
||||||
|
};
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
function buildHunkHashIndexMap(original: string, modified: string): Map<string, number[]> {
|
function buildHunkHashIndexMap(original: string, modified: string): Map<string, number[]> {
|
||||||
|
|
@ -1086,6 +1142,54 @@ function mapRejectedHunkIndicesByHash(
|
||||||
return [...out].sort((a, b) => a - b);
|
return [...out].sort((a, b) => a - b);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function mapRejectedHunkIndicesByHashStrict(
|
||||||
|
original: string,
|
||||||
|
modified: string,
|
||||||
|
rejectedIndices: number[],
|
||||||
|
hunkContextHashes: Record<number, string> | undefined
|
||||||
|
): { ok: true; indices: number[] } | { ok: false; code: ApplyErrorCode; error: string } {
|
||||||
|
if (rejectedIndices.length === 0) {
|
||||||
|
return { ok: true, indices: [] };
|
||||||
|
}
|
||||||
|
if (!hunkContextHashes || Object.keys(hunkContextHashes).length === 0) {
|
||||||
|
return {
|
||||||
|
ok: false,
|
||||||
|
code: 'manual-review-required',
|
||||||
|
error: 'Ledger partial reject requires stable hunk context hashes.',
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
const hashMap = buildHunkHashIndexMap(original, modified);
|
||||||
|
const out = new Set<number>();
|
||||||
|
for (const idx of rejectedIndices) {
|
||||||
|
const hash = hunkContextHashes[idx];
|
||||||
|
if (!hash) {
|
||||||
|
return {
|
||||||
|
ok: false,
|
||||||
|
code: 'manual-review-required',
|
||||||
|
error: 'Ledger partial reject is missing a hunk context hash.',
|
||||||
|
};
|
||||||
|
}
|
||||||
|
const candidates = hashMap.get(hash);
|
||||||
|
if (!candidates || candidates.length === 0) {
|
||||||
|
return {
|
||||||
|
ok: false,
|
||||||
|
code: 'conflict',
|
||||||
|
error: 'Ledger partial reject hunk context changed; please re-review.',
|
||||||
|
};
|
||||||
|
}
|
||||||
|
if (candidates.length > 1) {
|
||||||
|
return {
|
||||||
|
ok: false,
|
||||||
|
code: 'conflict',
|
||||||
|
error: 'Ledger partial reject hunk context is ambiguous; please re-review.',
|
||||||
|
};
|
||||||
|
}
|
||||||
|
out.add(candidates[0]!);
|
||||||
|
}
|
||||||
|
return { ok: true, indices: [...out].sort((a, b) => a - b) };
|
||||||
|
}
|
||||||
|
|
||||||
// ── Module-level helpers ──
|
// ── Module-level helpers ──
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
|
||||||
|
|
@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||||
|
|
||||||
import { createHash } from 'crypto';
|
import { createHash } from 'crypto';
|
||||||
import { structuredPatch } from 'diff';
|
import { structuredPatch } from 'diff';
|
||||||
|
import { computeDiffContextHash } from '@shared/utils/diffContextHash';
|
||||||
|
|
||||||
import type { SnippetDiff } from '@shared/types';
|
import type { SnippetDiff } from '@shared/types';
|
||||||
|
|
||||||
|
|
@ -985,7 +986,8 @@ describe('ReviewApplierService', () => {
|
||||||
{
|
{
|
||||||
filePath,
|
filePath,
|
||||||
fileDecision: 'pending',
|
fileDecision: 'pending',
|
||||||
hunkDecisions: { 0: 'rejected' },
|
hunkDecisions: { 0: 'rejected', 1: 'pending' },
|
||||||
|
hunkContextHashes: buildHunkContextHashes(original, modified),
|
||||||
},
|
},
|
||||||
],
|
],
|
||||||
},
|
},
|
||||||
|
|
@ -1034,8 +1036,99 @@ describe('ReviewApplierService', () => {
|
||||||
expect(res).toMatchObject({ applied: 1, conflicts: 0 });
|
expect(res).toMatchObject({ applied: 1, conflicts: 0 });
|
||||||
expect(writeFile).toHaveBeenCalledWith(filePath, original, 'utf8');
|
expect(writeFile).toHaveBeenCalledWith(filePath, original, 'utf8');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('ledger partial reject refuses stale hunk context instead of falling back to index', async () => {
|
||||||
|
const fsPromises = await import('fs/promises');
|
||||||
|
const readFile = fsPromises.readFile as unknown as ReturnType<typeof vi.fn>;
|
||||||
|
const writeFile = fsPromises.writeFile as unknown as ReturnType<typeof vi.fn>;
|
||||||
|
|
||||||
|
const filePath = '/tmp/stale-ledger.ts';
|
||||||
|
const original = 'const value = 1;\nconst keep = true;\n';
|
||||||
|
const modified = 'const value = 2;\nconst keep = true;\n';
|
||||||
|
readFile.mockResolvedValue(modified);
|
||||||
|
|
||||||
|
const { ReviewApplierService } = await import('@main/services/team/ReviewApplierService');
|
||||||
|
const svc = new ReviewApplierService();
|
||||||
|
|
||||||
|
const res = await svc.applyReviewDecisions(
|
||||||
|
{
|
||||||
|
teamName: 'team',
|
||||||
|
decisions: [
|
||||||
|
{
|
||||||
|
filePath,
|
||||||
|
fileDecision: 'pending',
|
||||||
|
hunkDecisions: { 0: 'rejected', 1: 'pending' },
|
||||||
|
hunkContextHashes: { 0: 'stale-context-hash' },
|
||||||
|
},
|
||||||
|
],
|
||||||
|
},
|
||||||
|
new Map([
|
||||||
|
[
|
||||||
|
filePath,
|
||||||
|
{
|
||||||
|
filePath,
|
||||||
|
relativePath: 'stale-ledger.ts',
|
||||||
|
snippets: [
|
||||||
|
{
|
||||||
|
toolUseId: 'ledger-1',
|
||||||
|
filePath,
|
||||||
|
toolName: 'Edit',
|
||||||
|
type: 'edit',
|
||||||
|
oldString: 'const value = 1;\n',
|
||||||
|
newString: 'const value = 2;\n',
|
||||||
|
replaceAll: false,
|
||||||
|
timestamp: '2026-03-01T10:00:00.000Z',
|
||||||
|
isError: false,
|
||||||
|
ledger: {
|
||||||
|
eventId: 'event-1',
|
||||||
|
source: 'ledger-exact',
|
||||||
|
confidence: 'exact',
|
||||||
|
originalFullContent: original,
|
||||||
|
modifiedFullContent: modified,
|
||||||
|
beforeHash: sha(original),
|
||||||
|
afterHash: sha(modified),
|
||||||
|
operation: 'modify',
|
||||||
|
beforeState: { exists: true, sha256: sha(original) },
|
||||||
|
afterState: { exists: true, sha256: sha(modified) },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
],
|
||||||
|
linesAdded: 1,
|
||||||
|
linesRemoved: 1,
|
||||||
|
isNewFile: false,
|
||||||
|
originalFullContent: original,
|
||||||
|
modifiedFullContent: modified,
|
||||||
|
contentSource: 'ledger-exact',
|
||||||
|
},
|
||||||
|
],
|
||||||
|
])
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(res.applied).toBe(0);
|
||||||
|
expect(res.conflicts).toBe(1);
|
||||||
|
expect(res.errors[0]?.code).toBe('conflict');
|
||||||
|
expect(writeFile).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
function sha(content: string): string {
|
function sha(content: string): string {
|
||||||
return createHash('sha256').update(content).digest('hex');
|
return createHash('sha256').update(content).digest('hex');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function buildHunkContextHashes(original: string, modified: string): Record<number, string> {
|
||||||
|
const patch = structuredPatch('file', 'file', original, modified);
|
||||||
|
const out: Record<number, string> = {};
|
||||||
|
for (let i = 0; i < patch.hunks.length; i++) {
|
||||||
|
const hunk = patch.hunks[i]!;
|
||||||
|
const oldSideContent = hunk.lines
|
||||||
|
.filter((line) => !line.startsWith('+'))
|
||||||
|
.map((line) => line.slice(1))
|
||||||
|
.join('\n');
|
||||||
|
const newSideContent = hunk.lines
|
||||||
|
.filter((line) => !line.startsWith('-'))
|
||||||
|
.map((line) => line.slice(1))
|
||||||
|
.join('\n');
|
||||||
|
out[i] = computeDiffContextHash(oldSideContent, newSideContent);
|
||||||
|
}
|
||||||
|
return out;
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue