From 6fa1d85a1739b64fe06dda9eda3779dac4266871 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 23:26:40 +0000 Subject: [PATCH] fix(registry): a failed version discovery must not abandon the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit publish-all called discoverVersions outside the try that guards the rest of the loop, so anything it threw escaped and ended the process. Definitions are walked in sorted order, so one third-party registry API returning 404, 410 or 429 silently abandoned every package after it and printed no summary at all — the run just died. Discovery talks to an external API, which fails for reasons unrelated to the package: a renamed module, a rate limit, an upstream blip. Those now record a failure for that definition and continue, exactly like a failed build. Found while reviewing #126, which adds a Go registry. That PR made it easy to hit — go/github.com/spf13/cobra sorts second of 141, so a single proxy hiccup would have taken down the other 139. The fragility is ours and predates it. The new tests run the real CLI against a scratch registry whose definitions use a registry with no version fetcher, so discovery throws before any network call. Both fail without the guard and pass with it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NBQQpA86yYzwJUiVz8ph2R --- packages/registry/src/cli.ts | 25 +++++-- packages/registry/src/publish-all.test.ts | 88 +++++++++++++++++++++++ 2 files changed, 108 insertions(+), 5 deletions(-) create mode 100644 packages/registry/src/publish-all.test.ts diff --git a/packages/registry/src/cli.ts b/packages/registry/src/cli.ts index 0855d3b..a956772 100644 --- a/packages/registry/src/cli.ts +++ b/packages/registry/src/cli.ts @@ -20,7 +20,7 @@ import { listDefinitions, } from "./definition.js"; import { checkPackageExists, publishPackage } from "./publish.js"; -import { discoverVersions } from "./version-check.js"; +import { type AvailableVersion, discoverVersions } from "./version-check.js"; const DEFAULT_REGISTRY_DIR = resolve( import.meta.dirname, @@ -225,10 +225,25 @@ program const failures: { id: string; error: string }[] = []; for (const def of definitions) { - const versions = await discoverVersions(def, { - since: opts.since ? Number(opts.since) : undefined, - latest: opts.latest ? Number(opts.latest) : undefined, - }); + // Discovery talks to a third-party registry API, so it can fail for + // reasons that have nothing to do with this package — a 404 for a renamed + // module, a 429, an upstream outage. Record it like a build failure + // instead of letting it throw out of the loop: definitions are processed + // in sorted order, so an unguarded throw here silently abandons every + // package after this one and prints no summary at all. + let versions: AvailableVersion[]; + try { + versions = await discoverVersions(def, { + since: opts.since ? Number(opts.since) : undefined, + latest: opts.latest ? Number(opts.latest) : undefined, + }); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + const id = `${def.registry}/${def.name}`; + console.error(` FAILED ${id} (version discovery): ${message}`); + failures.push({ id, error: message }); + continue; + } for (const ver of versions) { const id = `${def.registry}/${def.name}@${ver.version}`; diff --git a/packages/registry/src/publish-all.test.ts b/packages/registry/src/publish-all.test.ts new file mode 100644 index 0000000..be69f17 --- /dev/null +++ b/packages/registry/src/publish-all.test.ts @@ -0,0 +1,88 @@ +import { execFileSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +/** + * `publish-all` walks every definition in sorted order. A throw that escapes the + * loop abandons every package after it, so these run the real CLI and assert on + * what the nightly job would actually see. + * + * Definitions use a registry with no version fetcher, which makes discovery + * throw before any network call. + */ +describe("publish-all — a failing definition must not abandon the rest", () => { + let dir: string; + let out: string; + + const define = (registry: string, name: string): void => { + mkdirSync(join(dir, registry), { recursive: true }); + writeFileSync( + join(dir, registry, `${name}.yaml`), + [ + `name: ${name}`, + `description: "test"`, + `versions:`, + ` - min_version: "1.0.0"`, + ` source:`, + ` type: git`, + ` url: https://example.invalid/${name}`, + ` docs_path: docs`, + "", + ].join("\n"), + ); + }; + + const runPublishAll = (): { status: number; output: string } => { + try { + const stdout = execFileSync( + "npx", + ["tsx", "src/cli.ts", "publish-all", "--dir", dir, "--output", out], + { cwd: process.cwd(), encoding: "utf8", stdio: "pipe" }, + ); + return { status: 0, output: stdout }; + } catch (err) { + const e = err as { status?: number; stdout?: string; stderr?: string }; + return { + status: e.status ?? 1, + output: `${e.stdout ?? ""}${e.stderr ?? ""}`, + }; + } + }; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "registry-defs-")); + out = mkdtempSync(join(tmpdir(), "registry-out-")); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + rmSync(out, { recursive: true, force: true }); + }); + + it("records a discovery failure and still reaches the summary", () => { + define("unsupported", "aaa"); + define("unsupported", "zzz"); + + const { status, output } = runPublishAll(); + + // Both are recorded, not thrown — and the run reports rather than crashing. + expect(output).toContain("--- Summary ---"); + expect(output).toContain("Failed: 2"); + expect(output).toContain("unsupported/aaa"); + expect(output).toContain("unsupported/zzz"); + expect(status).not.toBe(0); + }, 60_000); + + it("does not abandon definitions sorted after a failing one", () => { + define("unsupported", "aaa"); + define("unsupported", "mmm"); + define("unsupported", "zzz"); + + const { output } = runPublishAll(); + + // The bug this guards: `aaa` throwing meant `mmm` and `zzz` were never seen. + expect(output).toContain("Failed: 3"); + }, 60_000); +});