From eeb0ecd932ba0cb57fedaa055948f538983f67ca Mon Sep 17 00:00:00 2001 From: George Zhang Date: Sun, 29 Mar 2026 10:38:27 -0700 Subject: [PATCH] fix: address post-merge vercel workflow/docs issues (#1386) * fix: address post-merge vercel review issues * fix: address remaining publish flow review findings * docs: note explicit plugin compatibility requirements --- .github/workflows/package-publish.yml | 17 ++- docs/cli.md | 4 +- packages/clawdhub/src/cli.ts | 2 +- .../clawdhub/src/cli/commands/github.test.ts | 115 ++++++++++++++++++ packages/clawdhub/src/cli/commands/github.ts | 72 +++++++---- .../src/cli/commands/packages.test.ts | 57 +++++++++ .../clawdhub/src/cli/commands/packages.ts | 3 +- src/__tests__/packages-publish-route.test.tsx | 17 +++ src/components/PackageSourceChooser.tsx | 2 +- 9 files changed, 260 insertions(+), 29 deletions(-) diff --git a/.github/workflows/package-publish.yml b/.github/workflows/package-publish.yml index 04126440..52e1447b 100644 --- a/.github/workflows/package-publish.yml +++ b/.github/workflows/package-publish.yml @@ -80,19 +80,27 @@ jobs: - name: Validate publish mode inputs env: DRY_RUN: ${{ inputs.dry_run }} + JSON_MODE: ${{ inputs.json }} CLAWHUB_TOKEN: ${{ secrets.clawhub_token }} run: | if [[ "$DRY_RUN" != "true" && -z "$CLAWHUB_TOKEN" ]]; then echo "::error::secrets.clawhub_token is required when dry_run is false." exit 1 fi + if [[ "$JSON_MODE" != "true" ]]; then + echo "::warning::This reusable workflow always emits JSON output; forcing --json for downstream parsing." + fi - name: Write ClawHub config - if: secrets.clawhub_token != '' env: CLAWHUB_TOKEN: ${{ secrets.clawhub_token }} CLAWHUB_REGISTRY: ${{ inputs.registry }} run: | + if [[ -z "$CLAWHUB_TOKEN" ]]; then + echo "No ClawHub token provided, skipping config file creation." + exit 0 + fi + python3 - <<'PY' import json import os @@ -119,13 +127,13 @@ jobs: INPUT_SOURCE: ${{ inputs.source }} INPUT_REF: ${{ inputs.ref }} INPUT_DRY_RUN: ${{ inputs.dry_run }} - INPUT_JSON: ${{ inputs.json }} INPUT_OWNER: ${{ inputs.owner }} INPUT_VERSION: ${{ inputs.version }} INPUT_TAGS: ${{ inputs.tags }} INPUT_SITE: ${{ inputs.site }} INPUT_REGISTRY: ${{ inputs.registry }} INPUT_CLAWHUB_VERSION: ${{ inputs.clawhub_version }} + GITHUB_SHA: ${{ github.sha }} run: | python3 - <<'PY' import json @@ -137,6 +145,8 @@ jobs: if not source: source = os.environ["GITHUB_REPOSITORY"] ref = os.environ["INPUT_REF"].strip() + if not ref and source == os.environ["GITHUB_REPOSITORY"]: + ref = os.environ["GITHUB_SHA"].strip() if ref and "@" not in source and not source.startswith("http"): source = f"{source}@{ref}" @@ -154,8 +164,7 @@ jobs: if os.environ["INPUT_DRY_RUN"] == "true": cmd.append("--dry-run") - if os.environ["INPUT_JSON"] == "true": - cmd.append("--json") + cmd.append("--json") owner = os.environ["INPUT_OWNER"].strip() version = os.environ["INPUT_VERSION"].strip() diff --git a/docs/cli.md b/docs/cli.md index e6aaa2b2..13e0837e 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -219,6 +219,8 @@ Stores your API token + cached registry URL. - Metadata is auto-detected from `package.json`, `openclaw.plugin.json`, and `openclaw.bundle.json`. - For GitHub sources, source attribution is auto-populated from the repo, resolved commit, ref, and subpath. - For local folders, source attribution is auto-detected from local git when the origin remote points at GitHub. +- External code plugins must declare `openclaw.compat.pluginApi` and `openclaw.build.openclawVersion` explicitly. + Top-level `package.json.version` is not used as a fallback for publish validation. - `--dry-run` previews the resolved publish payload without uploading. - `--json` emits machine-readable output for CI. - `--owner ` lets admins publish under a shared owner account while keeping their own token as the actor. @@ -228,7 +230,7 @@ Stores your API token + cached registry URL. #### GitHub Actions ClawHub also ships an official reusable workflow at -[`/.github/workflows/package-publish.yml`](/Users/tengjizhang/.codex/worktrees/7d03/clawhub/.github/workflows/package-publish.yml) +[`/.github/workflows/package-publish.yml`](../.github/workflows/package-publish.yml) for plugin repos. Typical caller setup: diff --git a/packages/clawdhub/src/cli.ts b/packages/clawdhub/src/cli.ts index 5ecbf4b4..8c490ea1 100644 --- a/packages/clawdhub/src/cli.ts +++ b/packages/clawdhub/src/cli.ts @@ -401,7 +401,7 @@ packageCmd .option("--source-repo ", "GitHub repo (owner/repo or URL)") .option("--source-commit ", "Git commit SHA") .option("--source-ref ", "Git ref/tag/branch") - .option("--source-path ", "Repo subpath", ".") + .option("--source-path ", "Repo subpath") .option("--dry-run", "Preview what would be published without uploading") .option("--json", "Output JSON (for CI pipelines)") .action(async (source, options) => { diff --git a/packages/clawdhub/src/cli/commands/github.test.ts b/packages/clawdhub/src/cli/commands/github.test.ts index e74b7e7e..b833299a 100644 --- a/packages/clawdhub/src/cli/commands/github.test.ts +++ b/packages/clawdhub/src/cli/commands/github.test.ts @@ -27,6 +27,37 @@ afterEach(() => { vi.restoreAllMocks(); }); +function mockGitHubCommitLookup(validRefs: string[]) { + const originalFetch = globalThis.fetch; + const fetchMock = vi.fn(async (input) => { + const url = input instanceof Request ? input.url : input.toString(); + const match = url.match(/\/repos\/owner\/repo\/commits\/(.+)$/); + if (!match) { + throw new Error(`Unexpected fetch: ${url}`); + } + const ref = decodeURIComponent(match[1] ?? ""); + if (!validRefs.includes(ref)) { + return new Response("not found", { status: 404 }); + } + return new Response(JSON.stringify({ sha: "0123456789abcdef0123456789abcdef01234567" }), { + status: 200, + headers: { "content-type": "application/json" }, + }); + }); + Object.defineProperty(globalThis, "fetch", { + value: fetchMock, + configurable: true, + writable: true, + }); + return () => { + Object.defineProperty(globalThis, "fetch", { + value: originalFetch, + configurable: true, + writable: true, + }); + }; +} + describe("github publish source helpers", () => { it.each([ ["owner/repo", { kind: "github", owner: "owner", repo: "repo", path: ".", url: "https://github.com/owner/repo" }], @@ -107,9 +138,36 @@ describe("github publish source helpers", () => { ], ])("parses %s as a GitHub source", async (input, expected) => { const workdir = await makeTmpDir(); + const restoreFetch = + input.includes("/tree/") || input.includes("/blob/") + ? mockGitHubCommitLookup([String((expected as { ref?: string }).ref ?? "")]) + : null; try { await expect(resolveSourceInput(input, { workdir })).resolves.toEqual(expected); } finally { + restoreFetch?.(); + await rm(workdir, { recursive: true, force: true }); + } + }); + + it("parses tree URLs whose refs contain slashes", async () => { + const workdir = await makeTmpDir(); + const restoreFetch = mockGitHubCommitLookup(["feature/new-ui"]); + try { + await expect( + resolveSourceInput("https://github.com/owner/repo/tree/feature/new-ui/plugins/demo", { + workdir, + }), + ).resolves.toEqual({ + kind: "github", + owner: "owner", + repo: "repo", + ref: "feature/new-ui", + path: "plugins/demo", + url: "https://github.com/owner/repo", + }); + } finally { + restoreFetch(); await rm(workdir, { recursive: true, force: true }); } }); @@ -241,4 +299,61 @@ describe("github publish source helpers", () => { }); } }); + + it("rejects GitHub archives with unsafe paths", async () => { + const archiveBytes = zipSync({ + "repo-root/../../escape.txt": new TextEncoder().encode("bad\n"), + "repo-root/package.json": new TextEncoder().encode('{"name":"demo","version":"1.0.0"}\n'), + "repo-root/openclaw.plugin.json": new TextEncoder().encode('{"id":"demo","configSchema":{"type":"object"}}\n'), + }); + const archiveBody = archiveBytes.buffer.slice( + archiveBytes.byteOffset, + archiveBytes.byteOffset + archiveBytes.byteLength, + ) as ArrayBuffer; + + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + new Response(JSON.stringify({ default_branch: "main" }), { + status: 200, + headers: { "content-type": "application/json" }, + }), + ) + .mockResolvedValueOnce( + new Response(JSON.stringify({ sha: "0123456789abcdef0123456789abcdef01234567" }), { + status: 200, + headers: { "content-type": "application/json" }, + }), + ) + .mockResolvedValueOnce( + new Response(archiveBody, { + status: 200, + headers: { "content-type": "application/zip" }, + }), + ); + const originalFetch = globalThis.fetch; + Object.defineProperty(globalThis, "fetch", { + value: fetchMock, + configurable: true, + writable: true, + }); + + try { + await expect( + fetchGitHubSource({ + kind: "github", + owner: "owner", + repo: "repo", + path: ".", + url: "https://github.com/owner/repo", + }), + ).rejects.toThrow(/Unsafe path in archive/i); + } finally { + Object.defineProperty(globalThis, "fetch", { + value: originalFetch, + configurable: true, + writable: true, + }); + } + }); }); diff --git a/packages/clawdhub/src/cli/commands/github.ts b/packages/clawdhub/src/cli/commands/github.ts index d7ec81ce..c5b2be70 100644 --- a/packages/clawdhub/src/cli/commands/github.ts +++ b/packages/clawdhub/src/cli/commands/github.ts @@ -2,7 +2,7 @@ import { spawnSync } from "node:child_process"; import { mkdir, mkdtemp, rm, stat, writeFile } from "node:fs/promises"; import { unzipSync } from "fflate"; import { homedir, tmpdir } from "node:os"; -import { dirname, join, resolve } from "node:path"; +import { dirname, join, resolve, sep } from "node:path"; const GITHUB_API = "https://api.github.com"; const GITHUB_HOSTS = new Set(["github.com", "www.github.com"]); @@ -52,7 +52,7 @@ export async function resolveSourceInput( if (!trimmed) throw new Error("Path required"); if (trimmed.startsWith("https://")) { - return parseGitHubUrl(trimmed); + return await parseGitHubUrl(trimmed); } const shorthand = parseGitHubShorthand(trimmed); @@ -178,7 +178,7 @@ function parseGitHubShorthand(input: string): Extract { +async function parseGitHubUrl(input: string): Promise> { let url: URL; try { url = new URL(input); @@ -204,29 +204,14 @@ function parseGitHubUrl(input: string): Extract, subpath: s } async function writeEntries(root: string, entries: Record) { + const absRoot = resolve(root); for (const [path, bytes] of Object.entries(entries)) { if (!path || path.endsWith("/")) continue; - const absPath = join(root, ...path.split("/")); + const absPath = resolve(absRoot, ...path.split("/")); + if (absPath !== absRoot && !absPath.startsWith(`${absRoot}${sep}`)) { + throw new Error(`Unsafe path in archive: ${path}`); + } await mkdir(dirname(absPath), { recursive: true }); await writeFile(absPath, Buffer.from(bytes)); } } +async function resolveGitHubUrlRefAndPath( + owner: string, + repo: string, + kind: "tree" | "blob", + segments: string[], +) { + if (segments.length === 0) throw new Error("Missing ref in GitHub URL"); + + const token = process.env.GITHUB_TOKEN?.trim() || undefined; + const minPathSegments = kind === "blob" ? 1 : 0; + const maxRefSegments = segments.length - minPathSegments; + + for (let refSegmentCount = maxRefSegments; refSegmentCount >= 1; refSegmentCount -= 1) { + const ref = segments.slice(0, refSegmentCount).join("/"); + const pathRemainder = segments.slice(refSegmentCount).join("/"); + if (kind === "blob" && !pathRemainder) continue; + const commit = await tryResolveCommitSha(owner, repo, ref, token); + if (!commit) continue; + const path = + kind === "blob" + ? normalizeRepoSubpath(pathRemainder.split("/").slice(0, -1).join("/") || ".") + : normalizeRepoSubpath(pathRemainder || "."); + return { ref, path }; + } + + throw new Error("GitHub ref not found in URL"); +} + function runGit(cwd: string, args: string[]) { const result = spawnSync("git", ["-C", cwd, ...args], { encoding: "utf8", diff --git a/packages/clawdhub/src/cli/commands/packages.test.ts b/packages/clawdhub/src/cli/commands/packages.test.ts index 86556951..a90fce8c 100644 --- a/packages/clawdhub/src/cli/commands/packages.test.ts +++ b/packages/clawdhub/src/cli/commands/packages.test.ts @@ -599,6 +599,63 @@ describe("package commands", () => { } }); + it("preserves inferred source subpaths for nested local plugin folders", async () => { + const workdir = await makeTmpWorkdir(); + const dateSpy = vi.spyOn(Date, "now").mockReturnValue(333_333_333); + try { + const folder = join(workdir, "packages", "demo-plugin"); + await mkdir(folder, { recursive: true }); + await writeFile( + join(folder, "package.json"), + makeCodePluginPackageJson({ + name: "demo-plugin", + displayName: "Demo Plugin", + version: "1.0.0", + }), + "utf8", + ); + await writeFile( + join(folder, "openclaw.plugin.json"), + JSON.stringify({ id: "demo.plugin", configSchema: { type: "object" } }), + "utf8", + ); + + runGit(workdir, ["init", "-b", "main"]); + runGit(workdir, ["remote", "add", "origin", "git@github.com:openclaw/demo-plugin.git"]); + runGit(workdir, ["add", "."]); + runGit(workdir, ["-c", "user.name=Test", "-c", "user.email=test@example.com", "commit", "-m", "init"]); + + httpMocks.apiRequestForm.mockResolvedValueOnce({ + ok: true, + packageId: "pkg_1", + releaseId: "rel_1", + }); + + await cmdPublishPackage(makeOpts(workdir), "packages/demo-plugin", {}); + + expect(getPublishPayload()).toEqual({ + name: "demo-plugin", + displayName: "Demo Plugin", + family: "code-plugin", + version: "1.0.0", + changelog: "", + tags: ["latest"], + source: { + kind: "github", + url: "https://github.com/openclaw/demo-plugin", + repo: "openclaw/demo-plugin", + ref: "main", + commit: expect.any(String), + path: "packages/demo-plugin", + importedAt: 333_333_333, + }, + }); + dateSpy.mockRestore(); + } finally { + await rm(workdir, { recursive: true, force: true }); + } + }); + it("supports dry-run without auth or publish and prints a summary", async () => { const workdir = await makeTmpWorkdir(); const dateSpy = vi.spyOn(Date, "now").mockReturnValue(444_444_444); diff --git a/packages/clawdhub/src/cli/commands/packages.ts b/packages/clawdhub/src/cli/commands/packages.ts index ca554b11..05612afa 100644 --- a/packages/clawdhub/src/cli/commands/packages.ts +++ b/packages/clawdhub/src/cli/commands/packages.ts @@ -792,7 +792,8 @@ function buildSource( const rawRepo = options.sourceRepo?.trim() || inferred?.repo?.trim(); const rawCommit = options.sourceCommit?.trim() || inferred?.commit?.trim(); const rawRef = options.sourceRef?.trim() || inferred?.ref?.trim(); - const rawPath = options.sourcePath?.trim() || inferred?.path?.trim(); + const explicitPath = options.sourcePath?.trim(); + const rawPath = explicitPath !== undefined ? explicitPath : inferred?.path?.trim(); if (!rawRepo && !rawCommit && !rawRef && !rawPath) return undefined; if (!rawRepo || !rawCommit) fail("--source-repo and --source-commit must be set together"); const repo = normalizeGitHubRepo(rawRepo); diff --git a/src/__tests__/packages-publish-route.test.tsx b/src/__tests__/packages-publish-route.test.tsx index 3adc53ca..94a82e0d 100644 --- a/src/__tests__/packages-publish-route.test.tsx +++ b/src/__tests__/packages-publish-route.test.tsx @@ -281,6 +281,23 @@ describe("plugins publish route", () => { expect(publishRelease).not.toHaveBeenCalled(); }); + it("does not mark the upload summary ready while validation errors are present", async () => { + renderPublishRoute(); + + const bigFile = new File([new Uint8Array(10 * 1024 * 1024 + 1)], "too-big.bin", { + type: "application/octet-stream", + }); + + fireEvent.change(getFileInput(), { target: { files: [bigFile] } }); + + await waitFor(() => { + expect(screen.getByText(/Each file must be 10MB or smaller/i)).toBeTruthy(); + }); + + const summary = document.querySelector(".plugin-upload-summary"); + expect(summary?.classList.contains("is-ready")).toBe(false); + }); + it("publishes a bundle plugin folder with bundle metadata", async () => { renderPublishRoute(); diff --git a/src/components/PackageSourceChooser.tsx b/src/components/PackageSourceChooser.tsx index e118b0bd..de206393 100644 --- a/src/components/PackageSourceChooser.tsx +++ b/src/components/PackageSourceChooser.tsx @@ -21,7 +21,7 @@ export function PackageSourceChooser(props: { const [isDragging, setIsDragging] = useState(false); const archiveInputRef = useRef(null); const directoryInputRef = useRef(null); - const isMetadataLocked = props.files.length === 0; + const isMetadataLocked = props.files.length === 0 || Boolean(props.validationError); const setDirectoryInputRef = (node: HTMLInputElement | null) => { directoryInputRef.current = node;