diff --git a/src/CrestApps.Core.Docs/docs/changelog/2.0.0.md b/src/CrestApps.Core.Docs/docs/changelog/2.0.0.md index cda8227b..130c299c 100644 --- a/src/CrestApps.Core.Docs/docs/changelog/2.0.0.md +++ b/src/CrestApps.Core.Docs/docs/changelog/2.0.0.md @@ -1741,6 +1741,18 @@ See [Stacked Image Carousel](../core/chat.md#stacked-image-carousel). ## Change Logs +- **Fixed: a PDF the agent composed showed its title twice and a chart marker as text.** Asked for a report + with the charts of the conversation, gpt-4.1 sent the title heading with `create_pdf` and again at the start + of `add_pdf_content`, and wrote `[chart:Refer to chart marker above]` as a paragraph under each chart's + heading. The file opened with the title twice and printed that line where each chart should be. + - a heading that repeats the heading right before it, with nothing between, is left out — within a call and + against the block the new content follows — and the agent is told to send only what is new. A heading + used again further on is kept + - a paragraph that is a whole `[chart:{…}]` marker is drawn as that chart; any other chart marker in a + paragraph, heading or list fails the call with the ways to add a chart, and the charts of the + conversation it can use. The PDF agent's instructions say never to write a marker in text + - content converted from a file is not changed by either rule + - **Fixed: a preview showed a file the reader could not download.** `preview_pdf`, `preview_word`, `preview_presentation` and `preview_tabular_data` registered only their pictures; a download existed only when the model also called the export tool, and the PDF agent's instructions told it to skip the export when diff --git a/src/CrestApps.Core.Docs/docs/core/ai-documents.md b/src/CrestApps.Core.Docs/docs/core/ai-documents.md index 83c19ce0..47ef860c 100644 --- a/src/CrestApps.Core.Docs/docs/core/ai-documents.md +++ b/src/CrestApps.Core.Docs/docs/core/ai-documents.md @@ -228,6 +228,11 @@ Without it, `sign_pdf` explains that signing is not configured; `verify_pdf_sign page shows no number but is counted, so the page after it reads "Page 2 of 3". - Positions on a page are points from its top-left, as `search_pdf` reports them; on a page with a `/Rotate` entry they are in the page's unrotated frame. +- A chart goes in as a chart block. A paragraph that is a whole `[chart:{…}]` marker is drawn as that chart; + any other chart marker in text — including a placeholder such as `[chart:Refer to chart marker above]` — + fails the call with the ways to add the chart, because text is printed as written. +- A heading that repeats the heading right before it, with nothing between, is left out, and the agent is + told. This covers a model that sends the title with `create_pdf` and again at the start of the content. - For the Orchard Core integration, see [CrestApps for Orchard Core](https://orchardcore.crestapps.com). ### Images diff --git a/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Templates/Prompts/pdf-agent.md b/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Templates/Prompts/pdf-agent.md index 641ce0b3..ee32c03f 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Templates/Prompts/pdf-agent.md +++ b/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Templates/Prompts/pdf-agent.md @@ -45,6 +45,8 @@ already listed, and never ask them to upload a file you produced — the working not in a document. Put the chart's fields inside the block's `chart` object: `{"type": "chart", "chart": {"conversation_chart": 1}}`. A chart with no data fails the call: read the error and fix it. + Never write a `[chart:...]` marker, or a placeholder such as "[chart: see above]", in a paragraph, + heading or list: text is printed as written, so the call fails. Add each chart as a chart block. `bar` and `column` draw upright bars; use `horizontal_bar` only for long category names. Use the categories and values the user gave — "a bar chart of the same numbers" plots exactly the table's rows. Pass `number_format` diff --git a/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Tools/AddPdfContentTool.cs b/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Tools/AddPdfContentTool.cs index 15a45ad0..044fe6a5 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Tools/AddPdfContentTool.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Tools/AddPdfContentTool.cs @@ -102,7 +102,7 @@ protected override async Task ExecuteAsync(PdfToolArguments arguments, P case "append": { var section = ResolveSection(definition, arguments); - warnings = await PdfBlockPreparer.PrepareAsync(definition, blocks, arguments.Services, cancellationToken); + warnings = await PdfBlockPreparer.PrepareAsync(definition, blocks, arguments.Services, cancellationToken, precedingBlock: LastBlockBefore(definition, section, section.Blocks.Count)); section.Blocks.AddRange(blocks); summary = $"Added {Describe(blocks)} to section {section.Id}."; @@ -113,8 +113,9 @@ protected override async Task ExecuteAsync(PdfToolArguments arguments, P case "insert_after": { var (section, index) = FindBlock(definition, target); - warnings = await PdfBlockPreparer.PrepareAsync(definition, blocks, arguments.Services, cancellationToken); - section.Blocks.InsertRange(operation == "insert_before" ? index : index + 1, blocks); + var position = operation == "insert_before" ? index : index + 1; + warnings = await PdfBlockPreparer.PrepareAsync(definition, blocks, arguments.Services, cancellationToken, precedingBlock: LastBlockBefore(definition, section, position)); + section.Blocks.InsertRange(position, blocks); summary = $"Inserted {Describe(blocks)} {(operation == "insert_before" ? "before" : "after")} {target}."; break; @@ -124,7 +125,7 @@ protected override async Task ExecuteAsync(PdfToolArguments arguments, P { var (section, index) = FindBlock(definition, target); var replacedId = section.Blocks[index].Id; - warnings = await PdfBlockPreparer.PrepareAsync(definition, blocks, arguments.Services, cancellationToken); + warnings = await PdfBlockPreparer.PrepareAsync(definition, blocks, arguments.Services, cancellationToken, precedingBlock: LastBlockBefore(definition, section, index)); // A one-for-one replacement keeps the id, so the block can still be referred to by the // name the model already knows. @@ -203,6 +204,34 @@ protected override async Task ExecuteAsync(PdfToolArguments arguments, P }, cancellationToken); } + /// + /// Gets the block that new blocks placed at a position in a section follow, looking back into earlier + /// sections when the position is the start of one. + /// + /// The document. + /// The section the blocks go into. + /// The index in the section the first new block takes. + /// The block before that position, or when there is none. + private static PdfBlockDefinition LastBlockBefore(PdfDocumentDefinition definition, PdfSectionDefinition section, int position) + { + if (position > 0) + { + return section.Blocks[position - 1]; + } + + var sectionIndex = definition.Sections.IndexOf(section); + + for (var index = sectionIndex - 1; index >= 0; index--) + { + if (definition.Sections[index].Blocks.Count > 0) + { + return definition.Sections[index].Blocks[^1]; + } + } + + return null; + } + private static PdfSectionDefinition ResolveSection(PdfDocumentDefinition definition, PdfToolArguments arguments) { if (arguments.TryGetElement("new_section", out var element) && element.ValueKind == System.Text.Json.JsonValueKind.Object) diff --git a/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Tools/PdfBlockPreparer.cs b/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Tools/PdfBlockPreparer.cs index d49fe003..071f244d 100644 --- a/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Tools/PdfBlockPreparer.cs +++ b/src/Primitives/CrestApps.Core.AI.Documents.Pdf/Tools/PdfBlockPreparer.cs @@ -17,6 +17,7 @@ internal static class PdfBlockPreparer { private const int DefaultMaxRows = 1000; private const int MaxRowsLimit = 5000; + private const string ChartMarkerPrefix = "[chart:"; /// /// Readies blocks for a document. @@ -26,17 +27,20 @@ internal static class PdfBlockPreparer /// The request services, used to reach the tabular workspace. /// The cancellation token. /// - /// Whether a chart block that ends up with nothing to plot fails the call. Blocks the model wrote do: a - /// chart left out with only a warning was reported to the user as "the charts are embedded" while the - /// file had headings over empty space. + /// Whether the blocks are the model's own writing, as opposed to content converted from a file. For them, a + /// chart block that ends up with nothing to plot fails the call — a chart left out with only a warning was + /// reported to the user as "the charts are embedded" while the file had headings over empty space — and so + /// does chart marker text, and a heading that repeats the one right before it is left out. /// + /// The block the new blocks follow in the document, when there is one. /// Notes about data that could not be read as asked. public static async Task> PrepareAsync( PdfDocumentDefinition definition, List blocks, IServiceProvider services, CancellationToken cancellationToken, - bool requireChartData = true) + bool requireChartData = true, + PdfBlockDefinition precedingBlock = null) { ArgumentNullException.ThrowIfNull(definition); @@ -49,6 +53,12 @@ public static async Task> PrepareAsync( ReplaceHandWrittenContents(definition, blocks, warnings); + if (requireChartData) + { + await ChartsFromMarkerTextAsync(blocks, services); + RemoveRepeatedHeadings(blocks, precedingBlock, warnings); + } + TabularToolRunner.PreparationResult? tabular = null; try @@ -291,6 +301,131 @@ internal static void LiftFlatObject(PdfBlockDefinition block) } } + /// + /// Turns a paragraph that is a whole [chart:…] marker into the chart, and refuses any other text that + /// carries one. + /// + /// + /// Text is printed as it is written, so a marker in it reached the reader as its characters. Asked to put + /// the charts of the conversation in a report, gpt-4.1 wrote [chart:Refer to chart marker above] as a + /// paragraph under each chart's heading — a placeholder for a chart it never named — and the file showed + /// that line where the chart should be. A marker that holds a chart is drawn; anything else names no chart + /// this host can find, so the call is refused with the ways to add one, rather than printing the marker. + /// + /// The blocks of the call, changed in place. + /// The request services, used to list the charts of the conversation. + private static async Task ChartsFromMarkerTextAsync(List blocks, IServiceProvider services) + { + for (var index = 0; index < blocks.Count; index++) + { + var block = blocks[index]; + + if (block is null) + { + continue; + } + + var type = PdfMigraDocBuilder.NormalizeType(block); + var text = block.Text?.Trim(); + + if (type is PdfBlockTypes.Paragraph or PdfBlockTypes.Markdown && + text is not null && + text.StartsWith(ChartMarkerPrefix, StringComparison.OrdinalIgnoreCase) && + text.EndsWith(']') && + PdfChartJsReader.FindMarkers(text).Count == 1 && + PdfChartJsReader.TryRead(text, out _)) + { + block.Type = PdfBlockTypes.Chart; + block.Chart = new PdfChartDefinition { ChartJs = text }; + block.Text = null; + + continue; + } + + if (type == PdfBlockTypes.Code) + { + // Code is shown as written on purpose; a marker in it is an example, not a chart. + continue; + } + + var marked = new[] { block.Text, block.Title } + .Concat(block.Items ?? []) + .FirstOrDefault(value => value?.Contains(ChartMarkerPrefix, StringComparison.OrdinalIgnoreCase) == true); + + if (marked is null) + { + continue; + } + + var listed = PdfConversationCharts.Describe(await PdfConversationCharts.GetAsync(services)); + + throw new PdfToolException( + $"Nothing was added: block {index + 1} ({type}) has a chart marker in its text — \"{Excerpt(marked)}\" — and text is printed as written, so the reader would see the marker instead of a chart. " + + "Add the chart as its own block: {\"type\": \"chart\", \"conversation_chart\": N} for a chart shown earlier in the conversation, labels and series with the real values, source over uploaded tabular data, or chart_js with the whole [chart:{…}] marker. Never write a marker in text." + + (listed is null ? string.Empty : Environment.NewLine + listed)); + } + } + + /// + /// Leaves out a heading that repeats the heading right before it. + /// + /// + /// gpt-4.1 put the document's title heading in create_pdf and then sent the content again starting + /// with the same heading, and the report opened with its title twice. Two headings with the same words and + /// nothing between them are never two sections, so the second is left out and the model is told. A heading + /// used again further on, with content between, is kept. + /// + /// The blocks of the call, changed in place. + /// The block the new blocks follow in the document, when there is one. + /// Receives a note for each heading left out. + internal static void RemoveRepeatedHeadings(List blocks, PdfBlockDefinition precedingBlock, List notes) + { + ArgumentNullException.ThrowIfNull(blocks); + ArgumentNullException.ThrowIfNull(notes); + + var previous = precedingBlock; + + for (var index = 0; index < blocks.Count; index++) + { + var block = blocks[index]; + + if (block is null) + { + continue; + } + + if (IsHeading(block) && IsHeading(previous) && string.Equals(HeadingKey(block), HeadingKey(previous), StringComparison.Ordinal)) + { + notes.Add($"Left out the heading \"{block.Text.Trim()}\": it is already the heading before it. Do not send content the document already has; append only what is new."); + blocks.RemoveAt(index); + index--; + + continue; + } + + previous = block; + } + } + + private static bool IsHeading(PdfBlockDefinition block) + { + return block is not null && + PdfMigraDocBuilder.NormalizeType(block) == PdfBlockTypes.Heading && + !string.IsNullOrWhiteSpace(block.Text); + } + + private static string HeadingKey(PdfBlockDefinition block) + { + return string.Join(' ', block.Text.Split((char[])null, StringSplitOptions.RemoveEmptyEntries)).ToUpperInvariant(); + } + + private static string Excerpt(string text) + { + var trimmed = text.Trim(); + + return trimmed.Length <= 80 ? trimmed : trimmed[..80] + "…"; + } + /// /// Turns an image block that points at a chart — a [chart:…] marker, Chart.js JSON, or a chart of /// the conversation — into the chart block it was meant to be. diff --git a/tests/CrestApps.Core.Tests/Core/Documents/Pdf/PdfRepeatedContentTests.cs b/tests/CrestApps.Core.Tests/Core/Documents/Pdf/PdfRepeatedContentTests.cs new file mode 100644 index 00000000..3e04901e --- /dev/null +++ b/tests/CrestApps.Core.Tests/Core/Documents/Pdf/PdfRepeatedContentTests.cs @@ -0,0 +1,153 @@ +using CrestApps.Core.AI.Documents.Pdf.Tools; +using UglyToad.PdfPig; + +namespace CrestApps.Core.Tests.Core.Documents.Pdf; + +public sealed class PdfRepeatedContentTests +{ + private const string Title = "2026 Project Tracker Issue Report"; + + // A chart marker as the chart tool writes it. Every value is invented. + private const string Marker = """[chart:{"type":"bar","data":{"labels":["Scoring form","Call list"],"datasets":[{"label":"Issues","data":[194,49]}]}}]"""; + + [Fact] + public async Task AddPdfContent_RepeatsTheTitleHeadingCreatePdfAdded_KeepsItOnce() + { + using var host = new PdfToolTestHost(); + + // What gpt-4.1 sent: the title heading with create_pdf, then the whole content again starting with it. + await host.InvokeAsync(new CreatePdfTool(), new + { + name = "report", + title = Title, + blocks = new object[] { new { type = "heading", text = Title, level = 1 } }, + }); + + var added = await host.InvokeAsync(new AddPdfContentTool(), new + { + blocks = new object[] + { + new { type = "heading", text = Title, level = 1 }, + new { type = "heading", text = "Cover Page & Introduction", level = 2 }, + new { type = "paragraph", text = "This presentation summarizes the top issues." }, + }, + }); + + Assert.Contains("already the heading before it", added, StringComparison.Ordinal); + Assert.Equal(1, await CountAsync(host, Title)); + } + + [Fact] + public async Task CreatePdf_SameHeadingTwiceInARow_KeepsItOnce() + { + using var host = new PdfToolTestHost(); + + await host.InvokeAsync(new CreatePdfTool(), new + { + name = "report", + blocks = new object[] + { + new { type = "title", text = Title }, + new { type = "heading", text = " 2026 project tracker issue report ", level = 1 }, + new { type = "paragraph", text = "Body." }, + }, + }); + + Assert.Equal(1, await CountAsync(host, Title)); + } + + [Fact] + public async Task CreatePdf_SameHeadingWithContentBetween_KeepsBoth() + { + using var host = new PdfToolTestHost(); + + // A heading used again for another part of the document is the author's choice, not a repeat. + await host.InvokeAsync(new CreatePdfTool(), new + { + name = "report", + blocks = new object[] + { + new { type = "heading", text = "Overview", level = 2 }, + new { type = "paragraph", text = "Frequency." }, + new { type = "heading", text = "Overview", level = 2 }, + new { type = "paragraph", text = "Hours." }, + }, + }); + + Assert.Equal(2, await CountAsync(host, "Overview")); + } + + [Fact] + public async Task AddPdfContent_ChartMarkerPlaceholderAsText_IsRefusedAndNamesTheWayToAddTheChart() + { + using var host = new PdfToolTestHost(); + + await host.InvokeAsync(new CreatePdfTool(), new { name = "report", title = Title }); + + // What the agent wrote under "Chart: Top 20 Issue Groups by Frequency"; the reader saw these characters. + var added = await host.InvokeAsync(new AddPdfContentTool(), new + { + blocks = new object[] + { + new { type = "heading", text = "Chart: Top 20 Issue Groups by Frequency", level = 2 }, + new { type = "paragraph", text = "[chart:Refer to chart marker above]" }, + }, + }); + + Assert.Contains("Nothing was added", added, StringComparison.Ordinal); + Assert.Contains("conversation_chart", added, StringComparison.Ordinal); + Assert.Equal(0, await CountAsync(host, "[chart:")); + } + + [Fact] + public async Task AddPdfContent_ChartMarkerAsParagraph_IsDrawnAsTheChart() + { + using var host = new PdfToolTestHost(); + + await host.InvokeAsync(new CreatePdfTool(), new { name = "report", title = Title }); + + var added = await host.InvokeAsync(new AddPdfContentTool(), new + { + blocks = new object[] { new { type = "paragraph", text = Marker } }, + }); + + Assert.DoesNotContain("Nothing was added", added, StringComparison.Ordinal); + Assert.Equal(0, await CountAsync(host, "[chart:")); + + var definition = (await host.LoadWorkspaceAsync()).Documents.Single().Definition; + var block = Assert.Single(definition.Sections.SelectMany(section => section.Blocks)); + + Assert.Equal("chart", block.Type); + Assert.Equal(["Scoring form", "Call list"], block.Chart.Labels); + } + + [Fact] + public async Task AddPdfContent_ChartMarkerInsideASentence_IsRefused() + { + using var host = new PdfToolTestHost(); + + await host.InvokeAsync(new CreatePdfTool(), new { name = "report", title = Title }); + + var added = await host.InvokeAsync(new AddPdfContentTool(), new + { + blocks = new object[] { new { type = "paragraph", text = $"The chart below shows it. {Marker}" } }, + }); + + Assert.Contains("Nothing was added", added, StringComparison.Ordinal); + Assert.Equal(0, await CountAsync(host, "[chart:")); + } + + private static async Task CountAsync(PdfToolTestHost host, string text) + { + using var pdf = PdfDocument.Open(await host.ReadWorkingPdfAsync("report")); + var all = string.Join('\n', pdf.GetPages().Select(page => string.Join(' ', page.GetWords().Select(word => word.Text)))); + var count = 0; + + for (var index = all.IndexOf(text, StringComparison.OrdinalIgnoreCase); index >= 0; index = all.IndexOf(text, index + text.Length, StringComparison.OrdinalIgnoreCase)) + { + count++; + } + + return count; + } +}