Merge pull request #1066 from buster-so/dallin-bus-1885-chat-is-returning-a-report-the-individual-metric-the-one

Add in more tests and catch multiple report metric files
This commit is contained in:
dal 2025-09-23 09:48:57 -06:00 committed by GitHub
commit 86a0834731
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 307 additions and 37 deletions

View File

@ -507,6 +507,276 @@ describe('done-tool-file-selection - report filtering functionality', () => {
});
});
describe('BUG-1885: Metrics used in reports appearing in response messages', () => {
it('should filter out metric when using new single-file report structure (user reported bug)', () => {
// This test reproduces the exact bug scenario reported by the user
const messages: ModelMessage[] = [
// Create a metric first
{
role: 'tool',
content: [
{
type: 'tool-result',
toolName: 'createMetrics',
toolCallId: 'metric-call',
output: {
type: 'json',
value: {
files: [
{
id: 'e774e254-7ccd-4f03-b28d-6d91b5331b8a',
name: 'Top 10 Customers by Lifetime Value',
version_number: 1,
},
],
},
},
},
],
},
// Create a report using new single-file structure that references the metric
{
role: 'assistant',
content: [
{
type: 'tool-call',
toolName: 'createReports',
toolCallId: 'report-call',
input: {
name: 'Top Customers Analysis',
content: `# Top Customers Analysis\n\n<metric metricId="e774e254-7ccd-4f03-b28d-6d91b5331b8a"/>\n\nAnalysis of top customers...`,
},
},
],
},
{
role: 'tool',
content: [
{
type: 'tool-result',
toolName: 'createReports',
toolCallId: 'report-call',
output: {
type: 'json',
value: {
file: {
id: '1a3bf75a-a4b9-415e-b121-d26d1873a45e',
name: 'Top Customers Analysis',
version_number: 1,
},
},
},
},
],
},
];
const files = extractFilesFromToolCalls(messages);
// The metric should be filtered out because it's referenced in the report
// The report should also be filtered out (all reports are filtered)
expect(files).toHaveLength(0);
expect(files.find((f) => f.id === 'e774e254-7ccd-4f03-b28d-6d91b5331b8a')).toBeUndefined();
expect(files.find((f) => f.id === '1a3bf75a-a4b9-415e-b121-d26d1873a45e')).toBeUndefined();
});
it('should filter metrics referenced in ANY report, not just the last one', () => {
const messages: ModelMessage[] = [
// Create three metrics
{
role: 'tool',
content: [
{
type: 'tool-result',
toolName: 'createMetrics',
toolCallId: 'metrics-call',
output: {
type: 'json',
value: {
files: [
{ id: 'metric-1', name: 'Metric 1', version_number: 1 },
{ id: 'metric-2', name: 'Metric 2', version_number: 1 },
{ id: 'metric-3', name: 'Metric 3', version_number: 1 },
],
},
},
},
],
},
// First report referencing metric-1
{
role: 'assistant',
content: [
{
type: 'tool-call',
toolName: 'createReports',
toolCallId: 'report-1-call',
input: {
name: 'Report 1',
content: '<metric metricId="metric-1"/>',
},
},
],
},
{
role: 'tool',
content: [
{
type: 'tool-result',
toolName: 'createReports',
toolCallId: 'report-1-call',
output: {
type: 'json',
value: {
file: { id: 'report-1', name: 'Report 1', version_number: 1 },
},
},
},
],
},
// Second report referencing metric-2
{
role: 'assistant',
content: [
{
type: 'tool-call',
toolName: 'createReports',
toolCallId: 'report-2-call',
input: {
name: 'Report 2',
content: '<metric metricId="metric-2"/>',
},
},
],
},
{
role: 'tool',
content: [
{
type: 'tool-result',
toolName: 'createReports',
toolCallId: 'report-2-call',
output: {
type: 'json',
value: {
file: { id: 'report-2', name: 'Report 2', version_number: 1 },
},
},
},
],
},
];
const files = extractFilesFromToolCalls(messages);
// Only metric-3 should remain (not referenced in any report)
expect(files).toHaveLength(1);
expect(files[0]?.id).toBe('metric-3');
expect(files.find((f) => f.id === 'metric-1')).toBeUndefined(); // In first report
expect(files.find((f) => f.id === 'metric-2')).toBeUndefined(); // In second report
});
it('should handle both legacy array and new single-file report structures', () => {
const messages: ModelMessage[] = [
// Create metrics
{
role: 'tool',
content: [
{
type: 'tool-result',
toolName: 'createMetrics',
toolCallId: 'metrics-mixed',
output: {
type: 'json',
value: {
files: [
{ id: 'metric-legacy', name: 'Legacy Metric', version_number: 1 },
{ id: 'metric-new', name: 'New Metric', version_number: 1 },
{ id: 'metric-standalone', name: 'Standalone Metric', version_number: 1 },
],
},
},
},
],
},
// Report using legacy array structure
{
role: 'assistant',
content: [
{
type: 'tool-call',
toolName: 'createReports',
toolCallId: 'legacy-report',
input: {
files: [
{
name: 'Legacy Report',
content: '<metric metricId="metric-legacy"/>',
},
],
},
},
],
},
{
role: 'tool',
content: [
{
type: 'tool-result',
toolName: 'createReports',
toolCallId: 'legacy-report',
output: {
type: 'json',
value: {
files: [{ id: 'report-legacy', name: 'Legacy Report', version_number: 1 }],
},
},
},
],
},
// Report using new single-file structure
{
role: 'assistant',
content: [
{
type: 'tool-call',
toolName: 'createReports',
toolCallId: 'new-report',
input: {
name: 'New Report',
content: '<metric metricId="metric-new"/>',
},
},
],
},
{
role: 'tool',
content: [
{
type: 'tool-result',
toolName: 'createReports',
toolCallId: 'new-report',
output: {
type: 'json',
value: {
file: { id: 'report-new', name: 'New Report', version_number: 1 },
},
},
},
],
},
];
const files = extractFilesFromToolCalls(messages);
// Only the standalone metric should remain
expect(files).toHaveLength(1);
expect(files[0]?.id).toBe('metric-standalone');
expect(files.find((f) => f.id === 'metric-legacy')).toBeUndefined();
expect(files.find((f) => f.id === 'metric-new')).toBeUndefined();
});
});
describe('createFileResponseMessages', () => {
it('should create response messages for selected files', () => {
const files = [

View File

@ -348,18 +348,16 @@ export function extractFilesFromToolCalls(messages: ModelMessage[]): ExtractedFi
let filteredFiles = filterOutDashboardMetrics(deduplicatedFiles);
// Filter out metrics and dashboards that are referenced in reports
filteredFiles = filterOutReportContainedFiles(filteredFiles, lastReportInfo);
// Pass all files so the function can check ALL reports, not just the last one
filteredFiles = filterOutReportContainedFiles(filteredFiles);
// Filter out reports that don't have metrics (they won't be in responseMessages anyway)
// Keep reports that have metrics since they'll be in the responseMessages
// Filter out ALL reports (they are already in responseMessages)
filteredFiles = filteredFiles.filter((file) => {
if (file.fileType !== 'report_file') {
return true; // Keep all non-report files that passed previous filters
}
// For reports, only keep them if they contain metrics
// Reports with metrics will be in responseMessages already, so we filter them out here
// to avoid duplication
// Filter out ALL reports - they are handled separately and already in responseMessages
return false;
});
@ -733,49 +731,51 @@ function filterOutDashboardMetrics(files: ExtractedFile[]): ExtractedFile[] {
/**
* Filter out metrics and dashboards that are referenced in reports
* Any metric that appears in a report should not be selected
* Any metric that appears in ANY report should not be selected
*/
function filterOutReportContainedFiles(
files: ExtractedFile[],
lastReportInfo?: ReportInfo
): ExtractedFile[] {
if (!lastReportInfo || !lastReportInfo.content) {
// No report content to check against
function filterOutReportContainedFiles(files: ExtractedFile[]): ExtractedFile[] {
// Extract all report files from the files array
const reportFiles = files.filter((f) => f.fileType === 'report_file' && f.content);
if (reportFiles.length === 0) {
// No reports to check against
console.info('[done-tool-file-selection] No report content to filter against');
return files;
}
const reportContent = lastReportInfo.content;
// Extract metric IDs from the last report content
// Extract metric IDs and dashboard IDs from ALL reports
const metricsInReports = new Set<string>();
const dashboardsInReports = new Set<string>();
// Extract metric IDs from report content using regex pattern
// Match any alphanumeric ID with hyphens (UUIDs, simple IDs like "metric-1", etc.)
const metricIdPattern = /metricId[="']+([a-zA-Z0-9-]+)["']/gi;
const matches = reportContent.matchAll(metricIdPattern);
// Process each report's content
for (const report of reportFiles) {
if (!report.content) continue;
for (const match of matches) {
if (match[1]) {
metricsInReports.add(match[1]);
// Extract metric IDs from report content using regex pattern
// Match any alphanumeric ID with hyphens (UUIDs, simple IDs like "metric-1", etc.)
const metricIdPattern = /metricId[="']+([a-zA-Z0-9-]+)["']/gi;
const matches = report.content.matchAll(metricIdPattern);
for (const match of matches) {
if (match[1]) {
metricsInReports.add(match[1]);
}
}
// Also check for dashboard IDs in reports
const dashboardIdPattern = /dashboardId[="']+([a-zA-Z0-9-]+)["']/gi;
const dashboardMatches = report.content.matchAll(dashboardIdPattern);
for (const match of dashboardMatches) {
if (match[1]) {
dashboardsInReports.add(match[1]);
}
}
}
// Also check for dashboard IDs in reports
const dashboardIdPattern = /dashboardId[="']+([a-zA-Z0-9-]+)["']/gi;
const dashboardMatches = reportContent.matchAll(dashboardIdPattern);
for (const match of dashboardMatches) {
if (match[1]) {
dashboardsInReports.add(match[1]);
}
}
console.info('[done-tool-file-selection] Checking for files absorbed by last report', {
reportId: lastReportInfo.id,
reportOperation: lastReportInfo.operation,
contentLength: reportContent.length,
console.info('[done-tool-file-selection] Checking for files absorbed by reports', {
reportCount: reportFiles.length,
reportIds: reportFiles.map((r) => r.id),
metricsInReports: metricsInReports.size,
dashboardsInReports: dashboardsInReports.size,
metricIds: Array.from(metricsInReports),