mirror of
https://github.com/Kaelio/ktx.git
synced 2026-06-22 08:38:08 +02:00
fix(ingest): verify repair outcomes and reject dangling join targets (#292)
One ingest integration hiccup no longer discards a whole source: - Replace the duplicated gate-repair and textual-resolver loops with one shared constrained-repair loop whose success criterion is re-running the failed check (verify), not whether the agent edited files. Verify failures feed the retry prompt; maxAttempts is 2. - Let the resolver declare a conflicting patch redundant: a verified no-change resolution is accepted as subsumed instead of failing the source (duplicate wiki-page creation from parallel work units). - Carry per-source validation errors through validateWuTouchedSources into gate messages and work-unit failure reasons instead of discarding them. - Move join-neighbor expansion into the shared validation path so work-unit validation and integration gates check the same set. - Reject joins whose target resolves to no source, at sl_write time and in the gates, attributed to the declaring source. Resolution mirrors the Python engine exactly (case-sensitive name within the connection), with a case-mismatch hint for the writing agent.
This commit is contained in:
Andrey Avtomonov
GitHub
parent
00cdf2de90
commit
a278d2f7d0
17 changed files with 1119 additions and 705 deletions
|
|
@ -108,57 +108,39 @@ describe('artifact gates', () => {
|
|||
).rejects.toThrow(/unknown sl_refs entity mart_account_segments\.total_contract_arr_cents/);
|
||||
});
|
||||
|
||||
it('validates direct declared-join neighbors of touched semantic-layer sources', async () => {
|
||||
it('passes touched sources to the shared validation path and surfaces its reasons', async () => {
|
||||
// Join-neighbor expansion lives inside validateTouchedSources (the same
|
||||
// path work units use); the gate hands over the raw touched set and must
|
||||
// carry the per-source reasons into the failure it throws.
|
||||
const semanticLayerService = {
|
||||
loadAllSources: vi.fn().mockResolvedValue({
|
||||
sources: [
|
||||
{
|
||||
name: 'orders',
|
||||
grain: ['order_id'],
|
||||
columns: [
|
||||
{ name: 'order_id', type: 'string' },
|
||||
{ name: 'account_id', type: 'string' },
|
||||
],
|
||||
joins: [{ to: 'accounts', on: 'orders.account_id = accounts.account_id', relationship: 'many_to_one' }],
|
||||
measures: [{ name: 'order_count', expr: 'count(*)' }],
|
||||
},
|
||||
{
|
||||
name: 'accounts',
|
||||
grain: ['account_id'],
|
||||
columns: [{ name: 'account_id', type: 'string' }],
|
||||
joins: [],
|
||||
measures: [{ name: 'account_count', expr: 'count(*)' }],
|
||||
},
|
||||
{
|
||||
name: 'segments',
|
||||
grain: ['segment_id'],
|
||||
columns: [
|
||||
{ name: 'segment_id', type: 'string' },
|
||||
{ name: 'account_id', type: 'string' },
|
||||
],
|
||||
joins: [{ to: 'accounts', on: 'segments.account_id = accounts.account_id', relationship: 'many_to_one' }],
|
||||
measures: [],
|
||||
},
|
||||
],
|
||||
loadErrors: [],
|
||||
}),
|
||||
loadAllSources: vi.fn().mockResolvedValue({ sources: [], loadErrors: [] }),
|
||||
};
|
||||
const validateTouchedSources = vi.fn().mockResolvedValue({ invalidSources: [], validSources: [] });
|
||||
|
||||
await validateFinalIngestArtifacts({
|
||||
connectionIds: ['warehouse'],
|
||||
changedWikiPageKeys: [],
|
||||
touchedSlSources: [{ connectionId: 'warehouse', sourceName: 'accounts' }],
|
||||
wikiService: { readPage: vi.fn() } as never,
|
||||
semanticLayerService: semanticLayerService as never,
|
||||
validateTouchedSources,
|
||||
tableExists: async () => true,
|
||||
const validateTouchedSources = vi.fn().mockResolvedValue({
|
||||
validSources: [],
|
||||
invalidSources: [
|
||||
{
|
||||
source: 'warehouse:mart_account_segments',
|
||||
errors: ['join target "accounts" does not exist'],
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
await expect(
|
||||
validateFinalIngestArtifacts({
|
||||
connectionIds: ['warehouse'],
|
||||
changedWikiPageKeys: [],
|
||||
touchedSlSources: [{ connectionId: 'warehouse', sourceName: 'mart_account_segments' }],
|
||||
wikiService: { readPage: vi.fn() } as never,
|
||||
semanticLayerService: semanticLayerService as never,
|
||||
validateTouchedSources,
|
||||
tableExists: async () => true,
|
||||
}),
|
||||
).rejects.toThrow(
|
||||
/semantic-layer validation failed for warehouse:mart_account_segments: join target "accounts" does not exist/,
|
||||
);
|
||||
|
||||
expect(validateTouchedSources).toHaveBeenCalledWith([
|
||||
{ connectionId: 'warehouse', sourceName: 'accounts' },
|
||||
{ connectionId: 'warehouse', sourceName: 'orders' },
|
||||
{ connectionId: 'warehouse', sourceName: 'segments' },
|
||||
{ connectionId: 'warehouse', sourceName: 'mart_account_segments' },
|
||||
]);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -53,7 +53,7 @@ describe('finalGateRepairPaths', () => {
|
|||
});
|
||||
|
||||
describe('repairFinalGateFailure', () => {
|
||||
it('lets the repair agent read gate errors and edit only allowed files', async () => {
|
||||
it('lets the repair agent read gate errors, edit only allowed files, and verifies the gate', async () => {
|
||||
const { workdir, trace } = await makeHarness();
|
||||
const agentRunner = {
|
||||
runLoop: vi.fn(async (params: any) => {
|
||||
|
|
@ -70,7 +70,7 @@ describe('repairFinalGateFailure', () => {
|
|||
path: 'wiki/global/other.md',
|
||||
content: 'not allowed',
|
||||
}),
|
||||
).rejects.toThrow(/gate repair path not allowed/);
|
||||
).rejects.toThrow(/repair path not allowed/);
|
||||
|
||||
await params.toolSet.write_repair_file.execute({
|
||||
path: 'wiki/global/account-segments.md',
|
||||
|
|
@ -79,6 +79,7 @@ describe('repairFinalGateFailure', () => {
|
|||
return { stopReason: 'natural' as const };
|
||||
}),
|
||||
};
|
||||
const verify = vi.fn(async () => ({ ok: true as const }));
|
||||
|
||||
const result = await repairFinalGateFailure({
|
||||
agentRunner,
|
||||
|
|
@ -88,6 +89,7 @@ describe('repairFinalGateFailure', () => {
|
|||
allowedPaths: ['wiki/global/account-segments.md'],
|
||||
trace,
|
||||
repairKind: 'final_artifact_gate',
|
||||
verify,
|
||||
maxAttempts: 1,
|
||||
stepBudget: 8,
|
||||
});
|
||||
|
|
@ -97,6 +99,7 @@ describe('repairFinalGateFailure', () => {
|
|||
attempts: 1,
|
||||
changedPaths: ['wiki/global/account-segments.md'],
|
||||
});
|
||||
expect(verify).toHaveBeenCalledWith(['wiki/global/account-segments.md']);
|
||||
await expect(readFile(join(workdir, 'wiki/global/account-segments.md'), 'utf-8')).resolves.toContain(
|
||||
'total_contract_arr',
|
||||
);
|
||||
|
|
@ -115,6 +118,7 @@ describe('repairFinalGateFailure', () => {
|
|||
|
||||
it('returns failed when the repair agent edits no allowed file', async () => {
|
||||
const { workdir, trace } = await makeHarness();
|
||||
const verify = vi.fn(async () => ({ ok: true as const }));
|
||||
const result = await repairFinalGateFailure({
|
||||
agentRunner: { runLoop: vi.fn(async () => ({ stopReason: 'natural' as const })) },
|
||||
workdir,
|
||||
|
|
@ -122,6 +126,7 @@ describe('repairFinalGateFailure', () => {
|
|||
allowedPaths: ['wiki/global/account-segments.md'],
|
||||
trace,
|
||||
repairKind: 'final_artifact_gate',
|
||||
verify,
|
||||
maxAttempts: 1,
|
||||
stepBudget: 8,
|
||||
});
|
||||
|
|
@ -131,6 +136,52 @@ describe('repairFinalGateFailure', () => {
|
|||
attempts: 1,
|
||||
reason: 'gate repair completed without editing an allowed path',
|
||||
});
|
||||
expect(verify).not.toHaveBeenCalled();
|
||||
await expect(readFile(trace.tracePath, 'utf-8')).resolves.toContain('gate_repair_failed');
|
||||
});
|
||||
|
||||
it('does not report repaired when edits fail gate verification', async () => {
|
||||
// Regression: the repair agent edited allowed files but left a dangling
|
||||
// join in place. The old loop reported "repaired" because a file changed;
|
||||
// success must come from the gate re-check instead.
|
||||
const { workdir, trace } = await makeHarness();
|
||||
const agentRunner = {
|
||||
runLoop: vi.fn(async (params: any) => {
|
||||
await params.toolSet.write_repair_file.execute({
|
||||
path: 'wiki/global/account-segments.md',
|
||||
content: 'an edit that does not fix the gate\n',
|
||||
});
|
||||
return { stopReason: 'natural' as const };
|
||||
}),
|
||||
};
|
||||
const verify = vi
|
||||
.fn()
|
||||
.mockResolvedValueOnce({
|
||||
ok: false,
|
||||
reason: 'final artifact gates failed:\nsemantic-layer validation failed for warehouse:accounts',
|
||||
})
|
||||
.mockResolvedValueOnce({ ok: true });
|
||||
|
||||
const result = await repairFinalGateFailure({
|
||||
agentRunner,
|
||||
workdir,
|
||||
gateError: 'final artifact gates failed:\nsemantic-layer validation failed for warehouse:accounts',
|
||||
allowedPaths: ['wiki/global/account-segments.md'],
|
||||
trace,
|
||||
repairKind: 'patch_semantic_gate',
|
||||
verify,
|
||||
maxAttempts: 2,
|
||||
stepBudget: 8,
|
||||
});
|
||||
|
||||
expect(result).toEqual({
|
||||
status: 'repaired',
|
||||
attempts: 2,
|
||||
changedPaths: ['wiki/global/account-segments.md'],
|
||||
});
|
||||
expect(verify).toHaveBeenCalledTimes(2);
|
||||
const secondPrompt = agentRunner.runLoop.mock.calls[1][0].userPrompt as string;
|
||||
expect(secondPrompt).toContain('semantic-layer validation failed for warehouse:accounts');
|
||||
expect(secondPrompt).toContain('Previous attempt did not pass the gate');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1901,13 +1901,13 @@ describe('IngestBundleRunner isolated diff path', () => {
|
|||
});
|
||||
deps.agentRunner.runLoop = vi.fn(async (params: any) => {
|
||||
if (params.telemetryTags.operationName === 'ingest-isolated-diff-textual-resolver') {
|
||||
const current = await params.toolSet.read_integration_file.execute({
|
||||
const current = await params.toolSet.read_repair_file.execute({
|
||||
path: 'semantic-layer/warehouse/mart_account_segments.yaml',
|
||||
});
|
||||
expect(current.markdown).toContain('total_contract_arr_cents');
|
||||
const patch = await params.toolSet.read_failed_patch.execute({});
|
||||
expect(patch.markdown).toContain('account_count');
|
||||
await params.toolSet.write_integration_file.execute({
|
||||
await params.toolSet.write_repair_file.execute({
|
||||
path: 'semantic-layer/warehouse/mart_account_segments.yaml',
|
||||
content:
|
||||
'name: mart_account_segments\n' +
|
||||
|
|
@ -2105,7 +2105,6 @@ describe('IngestBundleRunner isolated diff path', () => {
|
|||
});
|
||||
const trace = await readFile(join(runtime.configDir, '.ktx/ingest-traces/job-final-gate-repair/trace.jsonl'), 'utf-8');
|
||||
expect(trace).toContain('gate_repair_repaired');
|
||||
expect(trace).toContain('final_artifact_gates_after_gate_repair_finished');
|
||||
expect(trace).toContain('final_gate_repair_committed');
|
||||
} finally {
|
||||
await rm(runtime.homeDir, { recursive: true, force: true });
|
||||
|
|
@ -2191,7 +2190,8 @@ describe('IngestBundleRunner isolated diff path', () => {
|
|||
const reportCreate = vi.mocked(deps.reports.create).mock.calls.at(-1)?.[0] as any;
|
||||
expect(reportCreate.body.status).toBe('failed');
|
||||
expect(reportCreate.body.isolatedDiff).toMatchObject({
|
||||
gateRepairAttempts: 1,
|
||||
// Both attempts of the verify-based repair loop ran without an edit.
|
||||
gateRepairAttempts: 2,
|
||||
gateRepairs: 0,
|
||||
gateRepairFailures: 1,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -221,6 +221,7 @@ describe('integrateWorkUnitPatch', () => {
|
|||
touchedPaths: ['wiki/global/a.md'],
|
||||
});
|
||||
await writeFile(join(configDir, 'wiki/global/a.md'), 'accepted\nproposal\n', 'utf-8');
|
||||
await expect(context.verify(['wiki/global/a.md'])).resolves.toEqual({ ok: true });
|
||||
return {
|
||||
status: 'repaired' as const,
|
||||
attempts: 1,
|
||||
|
|
@ -336,6 +337,7 @@ describe('integrateWorkUnitPatch', () => {
|
|||
touchedPaths: ['wiki/global/a.md'],
|
||||
});
|
||||
await writeFile(join(configDir, 'wiki/global/a.md'), 'repaired semantic ref\n', 'utf-8');
|
||||
await expect(context.verify(['wiki/global/a.md'])).resolves.toEqual({ ok: true });
|
||||
return {
|
||||
status: 'repaired' as const,
|
||||
attempts: 1,
|
||||
|
|
@ -402,71 +404,56 @@ describe('integrateWorkUnitPatch', () => {
|
|||
await expect(readFile(join(configDir, 'wiki/global/a.md'), 'utf-8')).resolves.toBe('old\n');
|
||||
});
|
||||
|
||||
it('repairs a semantic gate failure after a textual conflict is resolved', async () => {
|
||||
const { homeDir, configDir, git } = await makeRepo();
|
||||
await mkdir(join(configDir, 'wiki/global'), { recursive: true });
|
||||
await writeFile(join(configDir, 'wiki/global/a.md'), 'base\n', 'utf-8');
|
||||
await git.commitFiles(['wiki/global/a.md'], 'base page', 'System User', 'system@example.com');
|
||||
const conflictBase = await git.revParseHead();
|
||||
it('accepts a redundant duplicate-creation patch as subsumed without committing', async () => {
|
||||
// Regression: two work units each emitted a creation patch for the same
|
||||
// wiki page. The second creation patch conflicts with the page already in
|
||||
// the tree; the resolver verifies a no-change resolution and the source
|
||||
// must not fail.
|
||||
const { homeDir, configDir, git, baseSha } = await makeRepo();
|
||||
await writeFile(join(configDir, 'wiki/global/b.md'), 'page from the first work unit\n', 'utf-8');
|
||||
await git.commitFiles(['wiki/global/b.md'], 'first creation', 'System User', 'system@example.com');
|
||||
const acceptedHead = await git.revParseHead();
|
||||
|
||||
await writeFile(join(configDir, 'wiki/global/a.md'), 'accepted\n', 'utf-8');
|
||||
await git.commitFiles(['wiki/global/a.md'], 'accepted edit', 'System User', 'system@example.com');
|
||||
|
||||
const childDir = join(homeDir, 'child-conflict-repair');
|
||||
await git.addWorktree(childDir, 'child-conflict-repair', conflictBase);
|
||||
const childDir = join(homeDir, 'child-duplicate');
|
||||
await git.addWorktree(childDir, 'child-duplicate', baseSha);
|
||||
const childGit = git.forWorktree(childDir);
|
||||
await writeFile(join(childDir, 'wiki/global/a.md'), 'proposal\n', 'utf-8');
|
||||
await childGit.commitFiles(['wiki/global/a.md'], 'proposal edit', 'System User', 'system@example.com');
|
||||
const patchPath = join(homeDir, 'proposal-repair.patch');
|
||||
await childGit.writeBinaryNoRenamePatch(conflictBase, 'HEAD', patchPath);
|
||||
await writeFile(join(childDir, 'wiki/global/b.md'), 'duplicate page from the second work unit\n', 'utf-8');
|
||||
await childGit.commitFiles(['wiki/global/b.md'], 'second creation', 'System User', 'system@example.com');
|
||||
const patchPath = join(homeDir, 'duplicate-creation.patch');
|
||||
await childGit.writeBinaryNoRenamePatch(baseSha, 'HEAD', patchPath);
|
||||
|
||||
const trace = new FileIngestTraceWriter({
|
||||
tracePath: join(homeDir, '.ktx/ingest-traces/job-resolver-repair/trace.jsonl'),
|
||||
jobId: 'job-resolver-repair',
|
||||
tracePath: join(homeDir, '.ktx/ingest-traces/job-subsumed/trace.jsonl'),
|
||||
jobId: 'job-subsumed',
|
||||
connectionId: 'warehouse',
|
||||
sourceKey: 'metabase',
|
||||
sourceKey: 'notion',
|
||||
level: 'trace',
|
||||
});
|
||||
|
||||
// Gate fails on the resolver's merged tree, then passes after the repair edit.
|
||||
const validateAppliedTree = vi
|
||||
.fn()
|
||||
.mockRejectedValueOnce(
|
||||
new Error('final artifact gates failed:\narr-definition: unknown sl_refs entity mart_arr_daily.arr_dollars'),
|
||||
)
|
||||
.mockResolvedValueOnce(undefined);
|
||||
|
||||
const repairGateFailure = vi.fn(async (context: { unitKey: string; touchedPaths: string[] }) => {
|
||||
expect(context).toMatchObject({ unitKey: 'wu-conflict-repair', touchedPaths: ['wiki/global/a.md'] });
|
||||
await writeFile(join(configDir, 'wiki/global/a.md'), 'accepted\nproposal repaired\n', 'utf-8');
|
||||
return { status: 'repaired' as const, attempts: 1, changedPaths: ['wiki/global/a.md'] };
|
||||
});
|
||||
|
||||
const result = await integrateWorkUnitPatch({
|
||||
unitKey: 'wu-conflict-repair',
|
||||
unitKey: 'wu-duplicate',
|
||||
patchPath,
|
||||
integrationGit: git,
|
||||
trace,
|
||||
author: { name: 'System User', email: 'system@example.com' },
|
||||
slDisallowed: false,
|
||||
allowedTargetConnectionIds: new Set(['warehouse']),
|
||||
validateAppliedTree,
|
||||
resolveTextualConflict: vi.fn(async () => {
|
||||
await writeFile(join(configDir, 'wiki/global/a.md'), 'accepted\nproposal\n', 'utf-8');
|
||||
return { status: 'repaired' as const, attempts: 1, changedPaths: ['wiki/global/a.md'] };
|
||||
validateAppliedTree: vi.fn(async () => {}),
|
||||
resolveTextualConflict: vi.fn(async (context) => {
|
||||
await expect(context.verify([])).resolves.toEqual({ ok: true });
|
||||
return { status: 'repaired' as const, attempts: 1, changedPaths: [] };
|
||||
}),
|
||||
repairGateFailure,
|
||||
});
|
||||
|
||||
expect(result).toMatchObject({
|
||||
status: 'accepted',
|
||||
touchedPaths: ['wiki/global/a.md'],
|
||||
textualResolution: { status: 'repaired' },
|
||||
gateRepair: { status: 'repaired', attempts: 1, changedPaths: ['wiki/global/a.md'] },
|
||||
touchedPaths: [],
|
||||
textualResolution: { status: 'repaired', attempts: 1, changedPaths: [] },
|
||||
});
|
||||
expect(validateAppliedTree).toHaveBeenCalledTimes(2);
|
||||
expect(repairGateFailure).toHaveBeenCalledOnce();
|
||||
await expect(readFile(join(configDir, 'wiki/global/a.md'), 'utf-8')).resolves.toBe('accepted\nproposal repaired\n');
|
||||
await expect(readFile(trace.tracePath, 'utf-8')).resolves.toContain('patch_accepted_after_textual_resolution');
|
||||
expect(await git.revParseHead()).toBe(acceptedHead);
|
||||
await expect(readFile(join(configDir, 'wiki/global/b.md'), 'utf-8')).resolves.toBe(
|
||||
'page from the first work unit\n',
|
||||
);
|
||||
await expect(readFile(trace.tracePath, 'utf-8')).resolves.toContain('patch_subsumed_after_textual_resolution');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -42,7 +42,7 @@ describe('resolveTextualConflict', () => {
|
|||
const { workdir, patchPath, trace } = await makeHarness();
|
||||
const agentRunner = {
|
||||
runLoop: vi.fn(async (params: any) => {
|
||||
const current = await params.toolSet.read_integration_file.execute({ path: 'wiki/global/account.md' });
|
||||
const current = await params.toolSet.read_repair_file.execute({ path: 'wiki/global/account.md' });
|
||||
expect(current.structured).toEqual({ path: 'wiki/global/account.md', exists: true });
|
||||
expect(current.markdown).toContain('accepted line');
|
||||
|
||||
|
|
@ -50,19 +50,20 @@ describe('resolveTextualConflict', () => {
|
|||
expect(patch.markdown).toContain('proposal line');
|
||||
|
||||
await expect(
|
||||
params.toolSet.write_integration_file.execute({
|
||||
params.toolSet.write_repair_file.execute({
|
||||
path: 'wiki/global/not-allowed.md',
|
||||
content: 'bad\n',
|
||||
}),
|
||||
).rejects.toThrow(/resolver path not allowed/);
|
||||
).rejects.toThrow(/repair path not allowed/);
|
||||
|
||||
await params.toolSet.write_integration_file.execute({
|
||||
await params.toolSet.write_repair_file.execute({
|
||||
path: 'wiki/global/account.md',
|
||||
content: 'accepted line\nproposal line\n',
|
||||
});
|
||||
return { stopReason: 'natural' as const };
|
||||
}),
|
||||
};
|
||||
const verify = vi.fn(async () => ({ ok: true as const }));
|
||||
|
||||
const result = await resolveTextualConflict({
|
||||
agentRunner,
|
||||
|
|
@ -72,6 +73,7 @@ describe('resolveTextualConflict', () => {
|
|||
touchedPaths: ['wiki/global/account.md'],
|
||||
trace,
|
||||
reason: 'patch failed: wiki/global/account.md',
|
||||
verify,
|
||||
maxAttempts: 1,
|
||||
stepBudget: 8,
|
||||
});
|
||||
|
|
@ -81,6 +83,7 @@ describe('resolveTextualConflict', () => {
|
|||
attempts: 1,
|
||||
changedPaths: ['wiki/global/account.md'],
|
||||
});
|
||||
expect(verify).toHaveBeenCalledWith(['wiki/global/account.md']);
|
||||
await expect(readFile(join(workdir, 'wiki/global/account.md'), 'utf-8')).resolves.toBe(
|
||||
'accepted line\nproposal line\n',
|
||||
);
|
||||
|
|
@ -97,8 +100,9 @@ describe('resolveTextualConflict', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('fails when the repair agent completes without editing any touched path', async () => {
|
||||
it('fails when the repair agent neither edits nor declares the patch redundant', async () => {
|
||||
const { workdir, patchPath, trace } = await makeHarness();
|
||||
const verify = vi.fn(async () => ({ ok: true as const }));
|
||||
const result = await resolveTextualConflict({
|
||||
agentRunner: { runLoop: vi.fn(async () => ({ stopReason: 'natural' as const })) },
|
||||
workdir,
|
||||
|
|
@ -107,6 +111,7 @@ describe('resolveTextualConflict', () => {
|
|||
touchedPaths: ['wiki/global/account.md'],
|
||||
trace,
|
||||
reason: 'patch failed: wiki/global/account.md',
|
||||
verify,
|
||||
maxAttempts: 1,
|
||||
stepBudget: 8,
|
||||
});
|
||||
|
|
@ -114,7 +119,112 @@ describe('resolveTextualConflict', () => {
|
|||
expect(result).toEqual({
|
||||
status: 'failed',
|
||||
attempts: 1,
|
||||
reason: 'resolver completed without editing an allowed path',
|
||||
reason: 'resolver completed without editing an allowed path or declaring the patch redundant',
|
||||
});
|
||||
expect(verify).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('succeeds without edits when the agent declares the patch redundant and the gates verify', async () => {
|
||||
// Regression: two Notion pages produced creation patches for the same
|
||||
// wiki key. The second patch conflicts, the integration tree already
|
||||
// holds a complete page, and the correct resolution is no edit at all.
|
||||
const { workdir, patchPath, trace } = await makeHarness();
|
||||
const agentRunner = {
|
||||
runLoop: vi.fn(async (params: any) => {
|
||||
const declared = await params.toolSet.declare_patch_redundant.execute({
|
||||
reason: 'wiki/global/account.md already documents this page',
|
||||
});
|
||||
expect(declared.structured).toEqual({ reason: 'wiki/global/account.md already documents this page' });
|
||||
return { stopReason: 'natural' as const };
|
||||
}),
|
||||
};
|
||||
const verify = vi.fn(async () => ({ ok: true as const }));
|
||||
|
||||
const result = await resolveTextualConflict({
|
||||
agentRunner,
|
||||
workdir,
|
||||
unitKey: 'wu-duplicate',
|
||||
patchPath,
|
||||
touchedPaths: ['wiki/global/account.md'],
|
||||
trace,
|
||||
reason: 'patch failed: wiki/global/account.md',
|
||||
verify,
|
||||
maxAttempts: 1,
|
||||
stepBudget: 8,
|
||||
});
|
||||
|
||||
expect(result).toEqual({ status: 'repaired', attempts: 1, changedPaths: [] });
|
||||
expect(verify).toHaveBeenCalledWith([]);
|
||||
await expect(readFile(join(workdir, 'wiki/global/account.md'), 'utf-8')).resolves.toBe('accepted line\n');
|
||||
await expect(readFile(trace.tracePath, 'utf-8')).resolves.toContain('textual_conflict_resolver_repaired');
|
||||
});
|
||||
|
||||
it('retries with the gate failure when verification rejects the first resolution', async () => {
|
||||
const { workdir, patchPath, trace } = await makeHarness();
|
||||
const agentRunner = {
|
||||
runLoop: vi.fn(async (params: any) => {
|
||||
await params.toolSet.write_repair_file.execute({
|
||||
path: 'wiki/global/account.md',
|
||||
content: 'accepted line\nproposal line\n',
|
||||
});
|
||||
return { stopReason: 'natural' as const };
|
||||
}),
|
||||
};
|
||||
const verify = vi
|
||||
.fn()
|
||||
.mockResolvedValueOnce({ ok: false, reason: 'final artifact gates failed: stale sl_refs entry' })
|
||||
.mockResolvedValueOnce({ ok: true });
|
||||
|
||||
const result = await resolveTextualConflict({
|
||||
agentRunner,
|
||||
workdir,
|
||||
unitKey: 'wu-retry',
|
||||
patchPath,
|
||||
touchedPaths: ['wiki/global/account.md'],
|
||||
trace,
|
||||
reason: 'patch failed: wiki/global/account.md',
|
||||
verify,
|
||||
maxAttempts: 2,
|
||||
stepBudget: 8,
|
||||
});
|
||||
|
||||
expect(result).toEqual({
|
||||
status: 'repaired',
|
||||
attempts: 2,
|
||||
changedPaths: ['wiki/global/account.md'],
|
||||
});
|
||||
expect(agentRunner.runLoop).toHaveBeenCalledTimes(2);
|
||||
const secondPrompt = agentRunner.runLoop.mock.calls[1][0].userPrompt as string;
|
||||
expect(secondPrompt).toContain('final artifact gates failed: stale sl_refs entry');
|
||||
});
|
||||
|
||||
it('fails when edits never pass verification', async () => {
|
||||
const { workdir, patchPath, trace } = await makeHarness();
|
||||
const agentRunner = {
|
||||
runLoop: vi.fn(async (params: any) => {
|
||||
await params.toolSet.write_repair_file.execute({
|
||||
path: 'wiki/global/account.md',
|
||||
content: 'still wrong\n',
|
||||
});
|
||||
return { stopReason: 'natural' as const };
|
||||
}),
|
||||
};
|
||||
const verify = vi.fn(async () => ({ ok: false as const, reason: 'final artifact gates failed' }));
|
||||
|
||||
const result = await resolveTextualConflict({
|
||||
agentRunner,
|
||||
workdir,
|
||||
unitKey: 'wu-never-passes',
|
||||
patchPath,
|
||||
touchedPaths: ['wiki/global/account.md'],
|
||||
trace,
|
||||
reason: 'patch failed: wiki/global/account.md',
|
||||
verify,
|
||||
maxAttempts: 2,
|
||||
stepBudget: 8,
|
||||
});
|
||||
|
||||
expect(result).toEqual({ status: 'failed', attempts: 2, reason: 'final artifact gates failed' });
|
||||
expect(verify).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -85,12 +85,13 @@ describe('Stage 3 — executeWorkUnit', () => {
|
|||
addTouchedSlSource(deps.captureSession.touchedSlSources, 'c1', 'src_good');
|
||||
return Promise.resolve({ stopReason: 'natural' });
|
||||
});
|
||||
deps.validateTouchedSources = vi
|
||||
.fn()
|
||||
.mockResolvedValue({ validSources: ['c1:src_good'], invalidSources: ['c1:src_bad'] });
|
||||
deps.validateTouchedSources = vi.fn().mockResolvedValue({
|
||||
validSources: ['c1:src_good'],
|
||||
invalidSources: [{ source: 'c1:src_bad', errors: ['join target "accounts" does not exist'] }],
|
||||
});
|
||||
const outcome = await executeWorkUnit(deps, makeWu());
|
||||
expect(outcome.status).toBe('failed');
|
||||
expect(outcome.reason).toMatch(/src_bad/);
|
||||
expect(outcome.reason).toMatch(/src_bad \(join target "accounts" does not exist\)/);
|
||||
expect(outcome.actions).toEqual([]);
|
||||
expect(outcome.touchedSlSources).toEqual([]);
|
||||
expect(deps.resetHardTo).toHaveBeenCalledWith('pre');
|
||||
|
|
|
|||
|
|
@ -1,8 +1,17 @@
|
|||
import { describe, expect, it, vi } from 'vitest';
|
||||
import { validateWuTouchedSources } from '../../../../src/context/ingest/stages/validate-wu-sources.js';
|
||||
import { formatInvalidWuSources, validateWuTouchedSources } from '../../../../src/context/ingest/stages/validate-wu-sources.js';
|
||||
|
||||
function makeSemanticLayerService(sourcesByConnection: Record<string, Array<{ name: string; joins?: Array<{ to: string }> }>>) {
|
||||
return {
|
||||
loadAllSources: vi.fn(async (connectionId: string) => ({
|
||||
sources: sourcesByConnection[connectionId] ?? [],
|
||||
loadErrors: [],
|
||||
})),
|
||||
};
|
||||
}
|
||||
|
||||
describe('validateWuTouchedSources', () => {
|
||||
it('validates each touched source against its own connection', async () => {
|
||||
it('validates each touched source against its own connection and carries validator errors', async () => {
|
||||
const validateSingleSource = vi
|
||||
.fn()
|
||||
.mockImplementation((_deps: unknown, conn: string, name: string) =>
|
||||
|
|
@ -12,7 +21,13 @@ describe('validateWuTouchedSources', () => {
|
|||
: { errors: ['invalid measure'], warnings: [] },
|
||||
),
|
||||
);
|
||||
const deps = { slValidator: { validateSingleSource } } as any;
|
||||
const deps = {
|
||||
semanticLayerService: makeSemanticLayerService({
|
||||
'warehouse-a': [{ name: 'good' }],
|
||||
'warehouse-b': [{ name: 'bad' }],
|
||||
}),
|
||||
slValidator: { validateSingleSource },
|
||||
} as any;
|
||||
|
||||
const result = await validateWuTouchedSources(deps, [
|
||||
{ connectionId: 'warehouse-a', sourceName: 'good' },
|
||||
|
|
@ -20,16 +35,137 @@ describe('validateWuTouchedSources', () => {
|
|||
]);
|
||||
|
||||
expect(result.validSources).toEqual(['warehouse-a:good']);
|
||||
expect(result.invalidSources).toEqual(['warehouse-b:bad']);
|
||||
expect(validateSingleSource).toHaveBeenNthCalledWith(1, deps, 'warehouse-a', 'good');
|
||||
expect(validateSingleSource).toHaveBeenNthCalledWith(2, deps, 'warehouse-b', 'bad');
|
||||
expect(result.invalidSources).toEqual([{ source: 'warehouse-b:bad', errors: ['invalid measure'] }]);
|
||||
});
|
||||
|
||||
it('returns empty arrays when no sources are touched', async () => {
|
||||
const validateSingleSource = vi.fn();
|
||||
const deps = { slValidator: { validateSingleSource } } as any;
|
||||
const semanticLayerService = makeSemanticLayerService({});
|
||||
const deps = { semanticLayerService, slValidator: { validateSingleSource } } as any;
|
||||
const result = await validateWuTouchedSources(deps, []);
|
||||
expect(result).toEqual({ validSources: [], invalidSources: [] });
|
||||
expect(validateSingleSource).not.toHaveBeenCalled();
|
||||
expect(semanticLayerService.loadAllSources).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('expands the validated set with existing join neighbors in both directions', async () => {
|
||||
const validateSingleSource = vi.fn().mockResolvedValue({ errors: [], warnings: [] });
|
||||
const deps = {
|
||||
semanticLayerService: makeSemanticLayerService({
|
||||
warehouse: [
|
||||
{ name: 'accounts', joins: [] },
|
||||
{ name: 'orders', joins: [{ to: 'accounts' }] },
|
||||
{ name: 'segments', joins: [{ to: 'accounts' }] },
|
||||
{ name: 'unrelated', joins: [] },
|
||||
],
|
||||
}),
|
||||
slValidator: { validateSingleSource },
|
||||
} as any;
|
||||
|
||||
const result = await validateWuTouchedSources(deps, [{ connectionId: 'warehouse', sourceName: 'accounts' }]);
|
||||
|
||||
expect(result.validSources).toEqual(['warehouse:accounts', 'warehouse:orders', 'warehouse:segments']);
|
||||
expect(validateSingleSource.mock.calls.map((call) => call[2])).toEqual(['accounts', 'orders', 'segments']);
|
||||
});
|
||||
|
||||
it('reports a dangling join target as an error on the source that declares it', async () => {
|
||||
// Regression: a Metabase work unit wrote mart_account_segments with
|
||||
// `joins: [{to: accounts}]` while no `accounts` source exists anywhere.
|
||||
// The error must name the declaring source, not the phantom neighbor.
|
||||
const validateSingleSource = vi.fn().mockResolvedValue({ errors: [], warnings: [] });
|
||||
const deps = {
|
||||
semanticLayerService: makeSemanticLayerService({
|
||||
warehouse: [{ name: 'mart_account_segments', joins: [{ to: 'accounts' }] }],
|
||||
}),
|
||||
slValidator: { validateSingleSource },
|
||||
} as any;
|
||||
|
||||
const result = await validateWuTouchedSources(deps, [
|
||||
{ connectionId: 'warehouse', sourceName: 'mart_account_segments' },
|
||||
]);
|
||||
|
||||
expect(result.validSources).toEqual([]);
|
||||
expect(result.invalidSources).toEqual([
|
||||
{
|
||||
source: 'warehouse:mart_account_segments',
|
||||
errors: ['join target "accounts" does not exist'],
|
||||
},
|
||||
]);
|
||||
// The phantom target is not validated as a source of its own.
|
||||
expect(validateSingleSource.mock.calls.map((call) => call[2])).toEqual(['mart_account_segments']);
|
||||
});
|
||||
|
||||
it('reports a join left dangling by a deletion on the surviving source', async () => {
|
||||
const validateSingleSource = vi.fn().mockResolvedValue({ errors: [], warnings: [] });
|
||||
const deps = {
|
||||
semanticLayerService: makeSemanticLayerService({
|
||||
// `accounts` was deleted by this work unit: touched but absent from
|
||||
// the loaded sources. `orders` still joins to it.
|
||||
warehouse: [{ name: 'orders', joins: [{ to: 'accounts' }] }],
|
||||
}),
|
||||
slValidator: { validateSingleSource },
|
||||
} as any;
|
||||
|
||||
const result = await validateWuTouchedSources(deps, [{ connectionId: 'warehouse', sourceName: 'accounts' }]);
|
||||
|
||||
expect(result.invalidSources).toContainEqual({
|
||||
source: 'warehouse:orders',
|
||||
errors: ['join target "accounts" does not exist'],
|
||||
});
|
||||
});
|
||||
|
||||
it('rejects join targets that match a source name only case-insensitively', async () => {
|
||||
// The Python engine resolves joins[].to by exact name; a case mismatch
|
||||
// would pass a lenient gate and then fail every query as an orphan.
|
||||
const validateSingleSource = vi.fn().mockResolvedValue({ errors: [], warnings: [] });
|
||||
const deps = {
|
||||
semanticLayerService: makeSemanticLayerService({
|
||||
warehouse: [{ name: 'SIGNED_UP' }, { name: 'orders', joins: [{ to: 'signed_up' }] }],
|
||||
}),
|
||||
slValidator: { validateSingleSource },
|
||||
} as any;
|
||||
|
||||
const result = await validateWuTouchedSources(deps, [{ connectionId: 'warehouse', sourceName: 'orders' }]);
|
||||
|
||||
expect(result.invalidSources).toEqual([
|
||||
{
|
||||
source: 'warehouse:orders',
|
||||
errors: [
|
||||
'join target "signed_up" does not exist; join targets are case-sensitive — the source is named "SIGNED_UP"',
|
||||
],
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
it('ignores pre-existing dangling joins on sources unrelated to this change set', async () => {
|
||||
const validateSingleSource = vi.fn().mockResolvedValue({ errors: [], warnings: [] });
|
||||
const deps = {
|
||||
semanticLayerService: makeSemanticLayerService({
|
||||
warehouse: [
|
||||
{ name: 'touched_source', joins: [] },
|
||||
{ name: 'legacy', joins: [{ to: 'phantom' }] },
|
||||
],
|
||||
}),
|
||||
slValidator: { validateSingleSource },
|
||||
} as any;
|
||||
|
||||
const result = await validateWuTouchedSources(deps, [{ connectionId: 'warehouse', sourceName: 'touched_source' }]);
|
||||
|
||||
expect(result.invalidSources).toEqual([]);
|
||||
expect(result.validSources).toEqual(['warehouse:touched_source']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('formatInvalidWuSources', () => {
|
||||
it('joins each source with its reasons', () => {
|
||||
expect(
|
||||
formatInvalidWuSources([
|
||||
{ source: 'warehouse:mart_account_segments', errors: ['join target "accounts" does not exist'] },
|
||||
{ source: 'warehouse:bad', errors: ['invalid YAML', 'duplicate measure'] },
|
||||
]),
|
||||
).toBe(
|
||||
'warehouse:mart_account_segments (join target "accounts" does not exist), ' +
|
||||
'warehouse:bad (invalid YAML; duplicate measure)',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1191,17 +1191,11 @@ describe('validateWithProposedSource', () => {
|
|||
});
|
||||
|
||||
it('rejects join keys that are absent from matched physical sources', async () => {
|
||||
const schemaPath = 'semantic-layer/postgres-warehouse/_schema/orbit_analytics.yaml';
|
||||
const schemaPath = 'semantic-layer/dbt-main/_schema/orbit_analytics.yaml';
|
||||
configService.listFiles.mockImplementation((dir: string) => {
|
||||
if (dir === 'semantic-layer/dbt-main') {
|
||||
return Promise.resolve({ files: [] });
|
||||
}
|
||||
if (dir === 'semantic-layer') {
|
||||
if (dir === 'semantic-layer/dbt-main' || dir === 'semantic-layer/dbt-main/_schema' || dir === 'semantic-layer') {
|
||||
return Promise.resolve({ files: [schemaPath] });
|
||||
}
|
||||
if (dir === 'semantic-layer/dbt-main/_schema' || dir === 'semantic-layer/postgres-warehouse/_schema') {
|
||||
return Promise.resolve({ files: dir.endsWith('postgres-warehouse/_schema') ? [schemaPath] : [] });
|
||||
}
|
||||
return Promise.resolve({ files: [] });
|
||||
});
|
||||
configService.readFile.mockResolvedValue({
|
||||
|
|
@ -1233,6 +1227,103 @@ describe('validateWithProposedSource', () => {
|
|||
expect(result.errors.join('\n')).toMatch(/local column "account_name"/);
|
||||
expect(result.errors.join('\n')).toMatch(/target column "account_uuid"/);
|
||||
});
|
||||
|
||||
it('rejects joins whose target resolves to no source and no manifest entry anywhere', async () => {
|
||||
// Regression: a Metabase work unit wrote `joins: [{to: accounts}]` while
|
||||
// no `accounts` source or manifest table existed in the project. The
|
||||
// write tool must reject the source so the agent can fix its own join.
|
||||
configService.listFiles.mockResolvedValue({ files: [] });
|
||||
pythonPort.validateSources.mockResolvedValue({
|
||||
data: { errors: [], warnings: [] },
|
||||
});
|
||||
|
||||
const result = await service.validateWithProposedSource('conn-1', {
|
||||
name: 'mart_account_segments',
|
||||
table: 'orbit_analytics.mart_account_segments',
|
||||
grain: ['account_id'],
|
||||
columns: [{ name: 'account_id', type: 'string' }],
|
||||
joins: [
|
||||
{ to: 'accounts', on: 'mart_account_segments.account_id = accounts.account_id', relationship: 'many_to_one' },
|
||||
],
|
||||
measures: [],
|
||||
});
|
||||
|
||||
expect(result.errors.join('\n')).toMatch(/mart_account_segments: join target "accounts" does not exist/);
|
||||
expect(pythonPort.validateSources).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects join targets that differ from the source name only by case', async () => {
|
||||
// The Python engine resolves joins[].to by exact name
|
||||
// (engine._collect_orphan_join_target_errors), so a case-insensitive
|
||||
// acceptance here would let the source pass gates and fail every query.
|
||||
const schemaPath = 'semantic-layer/conn-1/_schema/core.yaml';
|
||||
configService.listFiles.mockImplementation((dir: string) => {
|
||||
if (dir === 'semantic-layer/conn-1' || dir === 'semantic-layer/conn-1/_schema' || dir === 'semantic-layer') {
|
||||
return Promise.resolve({ files: [schemaPath] });
|
||||
}
|
||||
return Promise.resolve({ files: [] });
|
||||
});
|
||||
configService.readFile.mockResolvedValue({
|
||||
content: ['tables:', ' SIGNED_UP:', ' table: analytics.SIGNED_UP', ' columns:', ' - { name: account_id, type: string }'].join(
|
||||
'\n',
|
||||
),
|
||||
});
|
||||
pythonPort.validateSources.mockResolvedValue({
|
||||
data: { errors: [], warnings: [] },
|
||||
});
|
||||
|
||||
const result = await service.validateWithProposedSource('conn-1', {
|
||||
name: 'orders',
|
||||
table: 'analytics.orders',
|
||||
grain: ['account_id'],
|
||||
columns: [{ name: 'account_id', type: 'string' }],
|
||||
joins: [{ to: 'signed_up', on: 'orders.account_id = signed_up.account_id', relationship: 'many_to_one' }],
|
||||
measures: [],
|
||||
});
|
||||
|
||||
expect(result.errors.join('\n')).toMatch(
|
||||
/orders: join target "signed_up" does not exist; join targets are case-sensitive — the source is named "SIGNED_UP"/,
|
||||
);
|
||||
expect(pythonPort.validateSources).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects join targets written as table refs even when a manifest table matches', async () => {
|
||||
// `joins[].to` must be the source NAME ("accounts"), not the physical
|
||||
// table ref ("orbit_analytics.accounts") — the engine keys sources by name.
|
||||
const schemaPath = 'semantic-layer/conn-1/_schema/core.yaml';
|
||||
configService.listFiles.mockImplementation((dir: string) => {
|
||||
if (dir === 'semantic-layer/conn-1' || dir === 'semantic-layer/conn-1/_schema' || dir === 'semantic-layer') {
|
||||
return Promise.resolve({ files: [schemaPath] });
|
||||
}
|
||||
return Promise.resolve({ files: [] });
|
||||
});
|
||||
configService.readFile.mockResolvedValue({
|
||||
content: ['tables:', ' accounts:', ' table: orbit_analytics.accounts', ' columns:', ' - { name: account_id, type: string }'].join(
|
||||
'\n',
|
||||
),
|
||||
});
|
||||
pythonPort.validateSources.mockResolvedValue({
|
||||
data: { errors: [], warnings: [] },
|
||||
});
|
||||
|
||||
const result = await service.validateWithProposedSource('conn-1', {
|
||||
name: 'orders',
|
||||
table: 'orbit_analytics.orders',
|
||||
grain: ['account_id'],
|
||||
columns: [{ name: 'account_id', type: 'string' }],
|
||||
joins: [
|
||||
{
|
||||
to: 'orbit_analytics.accounts',
|
||||
on: 'orders.account_id = orbit_analytics.accounts.account_id',
|
||||
relationship: 'many_to_one',
|
||||
},
|
||||
],
|
||||
measures: [],
|
||||
});
|
||||
|
||||
expect(result.errors.join('\n')).toMatch(/orders: join target "orbit_analytics.accounts" does not exist/);
|
||||
expect(pythonPort.validateSources).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('findDanglingSegmentRefs', () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue