Skip to content

Commit 43fd79e

Browse files
committed
fix(cli): loop-compute table widths; surface real errors in collections list
Address PR review feedback: - printTable computed column widths with Math.max(c.length, ...rows.map(...)), spreading a per-column array into one call — a large table risks a max-arguments RangeError. Compute widths in a plain loop. - `collections list` caught every error from getCollection()/count() while the comment claimed it only skipped a delete race, so real provider failures vanished from the listing. Narrow the catch to CollectionNotFoundError and rethrow anything else; add tests for both paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015DvW7ANd1KemvLhUBoKCcF
1 parent a770b25 commit 43fd79e

3 files changed

Lines changed: 53 additions & 5 deletions

File tree

packages/cli/src/commands/collections.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,13 @@ export function registerCollectionCommands(program: Command): void {
7373
workflow:
7474
(await workflowName(metadata["workflow"])) ?? ""
7575
});
76-
} catch {
77-
// Skip a collection that races a delete between list and get.
76+
} catch (err) {
77+
// A collection can race a delete between listCollections() and
78+
// getCollection(); skip only that. Surface anything else (corrupt
79+
// collection, provider failure) instead of hiding it.
80+
if (!(err instanceof CollectionNotFoundError)) {
81+
throw err;
82+
}
7883
}
7984
}
8085
if (opts.json) {

packages/cli/src/commands/output.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,19 @@ export function printTable(
1919
return;
2020
}
2121
const cols = columns ?? Object.keys(rows[0]!);
22-
const widths = cols.map((c) =>
23-
Math.max(c.length, ...rows.map((r) => String(r[c] ?? "").length))
24-
);
22+
// Compute widths in a loop rather than Math.max(...spread): a large table
23+
// would spread thousands of args into one call, risking a max-arguments
24+
// RangeError and extra allocations.
25+
const widths = cols.map((c) => {
26+
let width = c.length;
27+
for (const r of rows) {
28+
const len = String(r[c] ?? "").length;
29+
if (len > width) {
30+
width = len;
31+
}
32+
}
33+
return width;
34+
});
2535
const sep = widths.map((w) => "─".repeat(w + 2)).join("┼");
2636
const header = cols.map((c, i) => ` ${c.padEnd(widths[i]!)} `).join("│");
2737
console.log(header);

packages/cli/tests/collections-action.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,39 @@ beforeEach(() => {
103103
readFile.mockReset();
104104
});
105105

106+
describe("collections list", () => {
107+
it("skips a collection that raced a delete but keeps healthy ones", async () => {
108+
listCollections.mockResolvedValueOnce([
109+
{ name: "good", metadata: { embedding_model: "m" } },
110+
{ name: "gone", metadata: {} }
111+
]);
112+
getCollection.mockReset().mockImplementation(async ({ name }) => {
113+
if (name === "gone") {
114+
throw new CollectionNotFoundError("gone");
115+
}
116+
return { name, metadata: {}, count: async () => 3, query, upsert };
117+
});
118+
const program = await buildProgram();
119+
const { stdout } = await captureOutput(() =>
120+
program.parseAsync(["node", "cli", "collections", "list", "--json"])
121+
);
122+
const rows = JSON.parse(stdout.trim()) as Array<{ name: string }>;
123+
expect(rows).toHaveLength(1);
124+
expect(rows[0]!.name).toBe("good");
125+
});
126+
127+
it("surfaces a non-not-found error instead of hiding it (exit 1)", async () => {
128+
listCollections.mockResolvedValueOnce([{ name: "bad", metadata: {} }]);
129+
getCollection.mockReset().mockRejectedValueOnce(new Error("provider boom"));
130+
const program = await buildProgram();
131+
const { exitCode, stderr } = await captureOutput(() =>
132+
program.parseAsync(["node", "cli", "collections", "list"])
133+
);
134+
expect(exitCode).toBe(1);
135+
expect(stderr).toContain("provider boom");
136+
});
137+
});
138+
106139
describe("collections create", () => {
107140
it("creates with embedding metadata", async () => {
108141
createCollection.mockResolvedValueOnce({ name: "docs", metadata: {} });

0 commit comments

Comments
 (0)