Skip to content

Commit e07a190

Browse files
authored
fix(tools): reject legacy binary .xls with an actionable error (#4374)
* fix(tools): reject legacy binary .xls with an actionable error DuckDB's excel extension only reads the ZIP-based OOXML container (.xlsx/.xlsm). A genuine binary BIFF .xls therefore fails deep inside read_xlsx with "No xl/workbook.xml found in xlsx file", which neither names the real cause nor says what to do; the multi-sheet .xls in #4239 surfaced exactly this way and left the agent with an empty table. Sniff the OLE2/CFB signature by content (not by extension, so a renamed .xlsx keeps working) and fail up front with a message that names the conversion to perform. The tool description is updated so the model is told the same thing before it calls the tool. Refs #4239 * fix(tools): wrap the legacy-xls warning under the line limit --------- Co-authored-by: Jay0130-a <Jay0130-a@users.noreply.github.com>
1 parent f0adfac commit e07a190

2 files changed

Lines changed: 151 additions & 2 deletions

File tree

‎internal/agent/tools/data_analysis.go‎

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package tools
22

33
import (
4+
"bytes"
45
"context"
56
"database/sql"
67
"encoding/json"
@@ -36,15 +37,58 @@ var dataAnalysisTool = BaseTool{
3637
"the single table \"" + DataAnalysisTableName + "\"; write SQL against that table name and never put " +
3738
"the document ID inside the SQL. For Excel files with multiple sheets, non-empty sheets are loaded into " +
3839
"the same table and the source sheet name is exposed as a '__sheet_name' column so you can " +
39-
"filter/aggregate per sheet. If the user's question requires data statistics, convert the " +
40-
"question into SQL and execute it.",
40+
"filter/aggregate per sheet. Only the .xlsx container is readable: a legacy binary .xls must be " +
41+
"re-saved as .xlsx (or exported as .csv) before it can be analysed. If the user's question requires " +
42+
"data statistics, convert the question into SQL and execute it.",
4143
schema: utils.GenerateSchema[DataAnalysisInput](),
4244
}
4345

4446
// excelSheetNameColumn is the name of the synthetic column that identifies
4547
// which Excel sheet a row came from when multiple sheets are unioned together.
4648
const excelSheetNameColumn = "__sheet_name"
4749

50+
// legacyXLSMagic is the OLE2/CFB compound-document signature that binary BIFF
51+
// .xls workbooks start with ("D0 CF 11 E0 A1 B1 1A E1").
52+
//
53+
// DuckDB's excel extension only reads the ZIP-based OOXML container
54+
// (.xlsx/.xlsm), so a genuine .xls has to be rejected up front: handed to
55+
// read_xlsx it fails with the opaque "No xl/workbook.xml found in xlsx file",
56+
// which says nothing about the real cause or what to do about it.
57+
var legacyXLSMagic = []byte{0xD0, 0xCF, 0x11, 0xE0, 0xA1, 0xB1, 0x1A, 0xE1}
58+
59+
// hasLegacyXLSContainer reports whether path holds an OLE2 compound document,
60+
// i.e. a binary BIFF .xls workbook.
61+
//
62+
// The sniff is by content, not by extension: a .xls that is really a renamed
63+
// .xlsx is a ZIP archive and returns false, so it keeps loading through the
64+
// normal read_xlsx path. Unreadable or truncated files also return false and
65+
// are left to DuckDB to report.
66+
func hasLegacyXLSContainer(path string) bool {
67+
f, err := os.Open(path)
68+
if err != nil {
69+
return false
70+
}
71+
defer func() { _ = f.Close() }()
72+
73+
head := make([]byte, len(legacyXLSMagic))
74+
if _, err := io.ReadFull(f, head); err != nil {
75+
return false
76+
}
77+
return bytes.Equal(head, legacyXLSMagic)
78+
}
79+
80+
// errLegacyBinaryXLS is the actionable failure returned for a binary .xls
81+
// workbook. The data analysis tool cannot read it, and silently reporting an
82+
// empty table (or leaking DuckDB's xlsx-internals error) leaves the model
83+
// guessing; naming the conversion to perform is what the caller can act on.
84+
func errLegacyBinaryXLS() error {
85+
return fmt.Errorf(
86+
"legacy binary .xls workbooks are not supported by data analysis: " +
87+
"only the ZIP-based .xlsx format can be read; " +
88+
"re-save the workbook as .xlsx (or export the sheet as .csv) and upload it again",
89+
)
90+
}
91+
4892
// sqlSingleQuoteEscape escapes single quotes in a string so it can be safely
4993
// embedded inside a single-quoted SQL literal.
5094
func sqlSingleQuoteEscape(s string) string {
@@ -547,9 +591,23 @@ func (t *DataAnalysisTool) LoadFromCSV(ctx context.Context, filename string, tab
547591
//
548592
// Note: requires the DuckDB 'excel' extension (for read_xlsx) and the
549593
// 'spatial' extension (for st_read_meta used to enumerate sheets).
594+
//
595+
// Binary BIFF .xls workbooks are rejected with an actionable error before
596+
// DuckDB is touched: the excel extension only reads the ZIP-based OOXML
597+
// container, so a real .xls would otherwise surface as "No xl/workbook.xml
598+
// found in xlsx file".
550599
func (t *DataAnalysisTool) LoadFromExcel(ctx context.Context, filename string, tableName string) (*TableSchema, error) {
551600
logger.Infof(ctx, "[Tool][DataAnalysis] Loading Excel file '%s' into table '%s' for session %s", filename, tableName, t.sessionID)
552601

602+
if hasLegacyXLSContainer(filename) {
603+
logger.Warnf(ctx,
604+
"[Tool][DataAnalysis] Rejecting legacy binary .xls workbook '%s' (session=%s): "+
605+
"DuckDB can only read the ZIP-based .xlsx container",
606+
filename, t.sessionID,
607+
)
608+
return nil, errLegacyBinaryXLS()
609+
}
610+
553611
if !t.loadedTables[tableName] {
554612
t.recordCreatedTable(tableName)
555613
sheetNames, enumErr := t.listExcelSheets(ctx, filename)
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
package tools
2+
3+
import (
4+
"context"
5+
"os"
6+
"path/filepath"
7+
"strings"
8+
"testing"
9+
)
10+
11+
// ole2Header is the compound-document signature a binary BIFF .xls starts with.
12+
var ole2Header = []byte{0xD0, 0xCF, 0x11, 0xE0, 0xA1, 0xB1, 0x1A, 0xE1}
13+
14+
// writeLegacyXLS writes a file that looks like a binary BIFF .xls: the OLE2
15+
// signature followed by opaque payload bytes. DuckDB's read_xlsx cannot open
16+
// this (there is no xl/workbook.xml inside), which is exactly the case #4239
17+
// reports.
18+
func writeLegacyXLS(t *testing.T, path string) {
19+
t.Helper()
20+
payload := append([]byte{}, ole2Header...)
21+
payload = append(payload, make([]byte, 512)...)
22+
if err := os.WriteFile(path, payload, 0o600); err != nil {
23+
t.Fatalf("write legacy xls fixture: %v", err)
24+
}
25+
}
26+
27+
func TestHasLegacyXLSContainerDetectsOLE2Workbook(t *testing.T) {
28+
path := filepath.Join(t.TempDir(), "legacy.xls")
29+
writeLegacyXLS(t, path)
30+
31+
if !hasLegacyXLSContainer(path) {
32+
t.Fatalf("expected %s to be detected as a binary BIFF workbook", path)
33+
}
34+
}
35+
36+
func TestHasLegacyXLSContainerIgnoresZipBasedWorkbooks(t *testing.T) {
37+
path := filepath.Join(t.TempDir(), "modern.xlsx")
38+
writeWorkbook(t, path, map[string][][]any{
39+
"Data": {{"id", "amount"}, {1, 100}},
40+
}, []string{"Data"})
41+
42+
// A .xls that is really a renamed .xlsx is a ZIP archive and must keep
43+
// loading through the normal read_xlsx path.
44+
if hasLegacyXLSContainer(path) {
45+
t.Fatalf("expected the ZIP-based workbook %s not to be flagged as legacy", path)
46+
}
47+
}
48+
49+
func TestHasLegacyXLSContainerLeavesShortAndMissingFilesAlone(t *testing.T) {
50+
dir := t.TempDir()
51+
52+
empty := filepath.Join(dir, "empty.xls")
53+
if err := os.WriteFile(empty, nil, 0o600); err != nil {
54+
t.Fatalf("write empty fixture: %v", err)
55+
}
56+
if hasLegacyXLSContainer(empty) {
57+
t.Fatalf("an empty file is not a legacy workbook")
58+
}
59+
60+
if hasLegacyXLSContainer(filepath.Join(dir, "missing.xls")) {
61+
t.Fatalf("a missing file must not be reported as a legacy workbook")
62+
}
63+
}
64+
65+
func TestLoadFromExcelRejectsLegacyBinaryXLSWithActionableError(t *testing.T) {
66+
path := filepath.Join(t.TempDir(), "legacy.xls")
67+
writeLegacyXLS(t, path)
68+
69+
// No DuckDB handle: the rejection has to happen before the tool touches
70+
// the database, otherwise a nil db would panic instead of failing cleanly.
71+
tool := &DataAnalysisTool{sessionID: "legacy-xls"}
72+
73+
_, err := tool.LoadFromExcel(context.Background(), path, "dataset")
74+
if err == nil {
75+
t.Fatal("expected a binary .xls workbook to be rejected")
76+
}
77+
78+
message := err.Error()
79+
for _, want := range []string{"legacy binary .xls", ".xlsx", ".csv"} {
80+
if !strings.Contains(message, want) {
81+
t.Errorf("error %q does not mention %q", message, want)
82+
}
83+
}
84+
}
85+
86+
func TestDataAnalysisToolDescriptionNamesTheXLSConversion(t *testing.T) {
87+
description := dataAnalysisTool.description
88+
if !strings.Contains(description, ".xlsx") || !strings.Contains(description, "binary .xls") {
89+
t.Fatalf("tool description must tell the model how to handle a legacy .xls, got: %s", description)
90+
}
91+
}

0 commit comments

Comments
 (0)