[codex] streamline plugin publish flow#1373
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d314617728
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ref = segments[3] ?? ""; | ||
| if (!ref) throw new Error("Missing ref in GitHub URL"); | ||
| const rest = segments.slice(4).join("/"); |
There was a problem hiding this comment.
GitHub tree/blob URLs can include branch names with slashes (for example .../tree/feature/new-ui/plugins/demo), but this parser always treats segments[3] as the entire ref and everything after it as path. In that case it resolves ref=feature and path=new-ui/plugins/demo, which can publish the wrong content or fail with GitHub ref not found for otherwise valid URLs copied from GitHub.
Useful? React with 👍 / 👎.
|
|
||
| output_path = Path(os.environ["RUNNER_TEMP"]) / "package-publish.json" | ||
| raw = output_path.read_text(encoding="utf-8").strip() | ||
| parsed = json.loads(raw) |
There was a problem hiding this comment.
This workflow exposes inputs.json, but the capture step always does json.loads(raw) on the command output. If a caller sets json: false, clawhub package publish emits human-readable text instead of JSON and this step crashes, so the reusable workflow advertises a mode it cannot execute successfully.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR streamlines the ClawHub plugin publish flow end-to-end: it adds a source-aware Key changes:
Issues found:
Confidence Score: 4/5Safe to merge after resolving the zip-slip and confirming the validation-fallback removal is intentional with a migration note. Two P1 findings remain: a zip-slip path traversal in the new
|
| Filename | Overview |
|---|---|
| packages/clawdhub/src/cli/commands/github.ts | New file implementing GitHub source resolution, zip download, and temp-dir extraction. Contains a zip-slip vulnerability in writeEntries — paths from the archive are joined into the temp dir without verifying the resolved path stays inside root. |
| convex/lib/packageRegistry.ts | Replaces inline compatibility extraction with shared schema helpers. Introduces a breaking validation change: openclaw.build.openclawVersion is now strictly required and no longer falls back to package.json.version. |
| packages/clawdhub/src/cli/commands/packages.ts | Adds --dry-run/--json support, source-aware publish (local/GitHub), and pre-upload metadata validation. The preparePackagePublishPlan + deferred cleanup pattern is clean and dry-run/real publish share the same plan object correctly. |
| src/components/PackageSourceChooser.tsx | New component extracting the upload panel from the publish route. Fixes the multi-picker issue. Minor regression: isMetadataLocked no longer incorporates validationError, so is-ready can appear incorrectly during error states. |
| packages/schema/src/openclawContract.ts | New shared schema module for the OpenClaw external plugin contract. Logic is consistent; note normalizeOpenClawExternalPluginCompatibility still falls back builtWithOpenClawVersion to root.version for display while the validation function is strict. |
| src/lib/pluginPublishPrefill.ts | Clean extraction of prefill logic from the publish route; adds missingRequiredFields and compatibility to the prefill result with no logic regressions. |
| .github/workflows/package-publish.yml | New reusable workflow_call workflow for plugin maintainers. Command construction uses shlex.quote to prevent shell injection. All temp files use RUNNER_TEMP. |
| src/routes/publish-plugin.tsx | Refactored to use PackageSourceChooser and pluginPublishPrefill. Adds codePluginFieldIssues state to block submission when required OpenClaw metadata is missing. Double guard (disabled button + submit handler check) is sound. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/clawdhub/src/cli/commands/github.ts
Line: 348-355
Comment:
**Zip-slip path traversal in `writeEntries`**
`path.join(root, ...path.split("/"))` normalizes `..` segments but does not verify that the resolved path stays inside `root`. A zip entry named `foo/../../etc/cron.daily/evil` would resolve to `/etc/cron.daily/evil` (given a `/tmp/...` root) after Node's normalization.
Even though the archives here come from GitHub's API and are unlikely to be crafted this way, the function accepts arbitrary `Record<string, Uint8Array>` entries and the same helper could be reused or extended later.
Add a containment check after resolving the path:
```suggestion
async function writeEntries(root: string, entries: Record<string, Uint8Array>) {
const absRoot = resolve(root);
for (const [path, bytes] of Object.entries(entries)) {
if (!path || path.endsWith("/")) continue;
const absPath = join(absRoot, ...path.split("/"));
if (!absPath.startsWith(absRoot + sep)) {
throw new Error(`Unsafe path in archive: ${path}`);
}
await mkdir(dirname(absPath), { recursive: true });
await writeFile(absPath, Buffer.from(bytes));
}
}
```
(`sep` and `resolve` are already imported from `"node:path"`.)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: convex/lib/packageRegistry.ts
Line: 194-199
Comment:
**Breaking validation change: `openclaw.build.openclawVersion` no longer falls back to `package.json.version`**
The old `extractCompatibility` used `package.json.version` as a fallback when `openclaw.build.openclawVersion` was absent:
```ts
} else if (version) {
compatibility.builtWithOpenClawVersion = version;
}
```
So the old backend check `if (!compatibility.builtWithOpenClawVersion)` would pass for any plugin that had a top-level `version` field, even without an explicit `openclaw.build.openclawVersion`.
The new `listMissingOpenClawExternalCodePluginFieldPaths` strictly requires `openclaw.build.openclawVersion` with no fallback. Plugins that previously published successfully with only `{ "version": "1.0.0" }` and no `openclaw.build` block will now receive a `ConvexError` at publish time.
Note that `normalizeOpenClawExternalPluginCompatibility` (used for display/compatibility data) still retains the fallback, creating a split: the compatibility data for previously-published releases reflects the fallback value, but the validation gate now prevents those same packages from being published again.
Please confirm this is deliberate and consider adding a migration note or a more informative error message.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/components/PackageSourceChooser.tsx
Line: 23
Comment:
**`is-ready` class applied even when `validationError` is non-null**
The old inline code computed `isMetadataLocked` as:
```ts
const isMetadataLocked = files.length === 0 || Boolean(validationError);
```
and used it to suppress the `is-ready` CSS class on `plugin-upload-summary`. In the extracted component the check is now:
```ts
const isMetadataLocked = props.files.length === 0;
```
So when `props.validationError` is set (e.g., file-size limit exceeded) the summary will still receive the `is-ready` class, giving the upload panel a visually "ready" appearance while actually blocking the form.
```suggestion
const isMetadataLocked = props.files.length === 0 || Boolean(props.validationError);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test: fix clawdhub ci portability" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async function writeEntries(root: string, entries: Record<string, Uint8Array>) { | ||
| for (const [path, bytes] of Object.entries(entries)) { | ||
| if (!path || path.endsWith("/")) continue; | ||
| const absPath = join(root, ...path.split("/")); | ||
| await mkdir(dirname(absPath), { recursive: true }); | ||
| await writeFile(absPath, Buffer.from(bytes)); | ||
| } | ||
| } |
There was a problem hiding this comment.
writeEntries
path.join(root, ...path.split("/")) normalizes .. segments but does not verify that the resolved path stays inside root. A zip entry named foo/../../etc/cron.daily/evil would resolve to /etc/cron.daily/evil (given a /tmp/... root) after Node's normalization.
Even though the archives here come from GitHub's API and are unlikely to be crafted this way, the function accepts arbitrary Record<string, Uint8Array> entries and the same helper could be reused or extended later.
Add a containment check after resolving the path:
| async function writeEntries(root: string, entries: Record<string, Uint8Array>) { | |
| for (const [path, bytes] of Object.entries(entries)) { | |
| if (!path || path.endsWith("/")) continue; | |
| const absPath = join(root, ...path.split("/")); | |
| await mkdir(dirname(absPath), { recursive: true }); | |
| await writeFile(absPath, Buffer.from(bytes)); | |
| } | |
| } | |
| async function writeEntries(root: string, entries: Record<string, Uint8Array>) { | |
| const absRoot = resolve(root); | |
| for (const [path, bytes] of Object.entries(entries)) { | |
| if (!path || path.endsWith("/")) continue; | |
| const absPath = join(absRoot, ...path.split("/")); | |
| if (!absPath.startsWith(absRoot + sep)) { | |
| throw new Error(`Unsafe path in archive: ${path}`); | |
| } | |
| await mkdir(dirname(absPath), { recursive: true }); | |
| await writeFile(absPath, Buffer.from(bytes)); | |
| } | |
| } |
(sep and resolve are already imported from "node:path".)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/clawdhub/src/cli/commands/github.ts
Line: 348-355
Comment:
**Zip-slip path traversal in `writeEntries`**
`path.join(root, ...path.split("/"))` normalizes `..` segments but does not verify that the resolved path stays inside `root`. A zip entry named `foo/../../etc/cron.daily/evil` would resolve to `/etc/cron.daily/evil` (given a `/tmp/...` root) after Node's normalization.
Even though the archives here come from GitHub's API and are unlikely to be crafted this way, the function accepts arbitrary `Record<string, Uint8Array>` entries and the same helper could be reused or extended later.
Add a containment check after resolving the path:
```suggestion
async function writeEntries(root: string, entries: Record<string, Uint8Array>) {
const absRoot = resolve(root);
for (const [path, bytes] of Object.entries(entries)) {
if (!path || path.endsWith("/")) continue;
const absPath = join(absRoot, ...path.split("/"));
if (!absPath.startsWith(absRoot + sep)) {
throw new Error(`Unsafe path in archive: ${path}`);
}
await mkdir(dirname(absPath), { recursive: true });
await writeFile(absPath, Buffer.from(bytes));
}
}
```
(`sep` and `resolve` are already imported from `"node:path"`.)
How can I resolve this? If you propose a fix, please make it concise.| if (!runtimeId) throw new ConvexError("openclaw.plugin.json must declare an id"); | ||
|
|
||
| const compatibility = extractCompatibility(params.packageJson); | ||
| if (!compatibility?.pluginApiRange) { | ||
| throw new ConvexError("package.json openclaw.compat.pluginApi is required"); | ||
| } | ||
| if (!compatibility.builtWithOpenClawVersion) { | ||
| throw new ConvexError("package.json openclaw.build.openclawVersion is required"); | ||
| const missingOpenClawFields = listMissingOpenClawExternalCodePluginFieldPaths(params.packageJson); | ||
| if (missingOpenClawFields.length > 0) { | ||
| throw new ConvexError(`package.json ${missingOpenClawFields[0]} is required`); |
There was a problem hiding this comment.
openclaw.build.openclawVersion no longer falls back to package.json.version
The old extractCompatibility used package.json.version as a fallback when openclaw.build.openclawVersion was absent:
} else if (version) {
compatibility.builtWithOpenClawVersion = version;
}So the old backend check if (!compatibility.builtWithOpenClawVersion) would pass for any plugin that had a top-level version field, even without an explicit openclaw.build.openclawVersion.
The new listMissingOpenClawExternalCodePluginFieldPaths strictly requires openclaw.build.openclawVersion with no fallback. Plugins that previously published successfully with only { "version": "1.0.0" } and no openclaw.build block will now receive a ConvexError at publish time.
Note that normalizeOpenClawExternalPluginCompatibility (used for display/compatibility data) still retains the fallback, creating a split: the compatibility data for previously-published releases reflects the fallback value, but the validation gate now prevents those same packages from being published again.
Please confirm this is deliberate and consider adding a migration note or a more informative error message. Is the removal of the package.json.version fallback for openclaw.build.openclawVersion intentional? If so, is a migration guide or clearer error message planned?
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/lib/packageRegistry.ts
Line: 194-199
Comment:
**Breaking validation change: `openclaw.build.openclawVersion` no longer falls back to `package.json.version`**
The old `extractCompatibility` used `package.json.version` as a fallback when `openclaw.build.openclawVersion` was absent:
```ts
} else if (version) {
compatibility.builtWithOpenClawVersion = version;
}
```
So the old backend check `if (!compatibility.builtWithOpenClawVersion)` would pass for any plugin that had a top-level `version` field, even without an explicit `openclaw.build.openclawVersion`.
The new `listMissingOpenClawExternalCodePluginFieldPaths` strictly requires `openclaw.build.openclawVersion` with no fallback. Plugins that previously published successfully with only `{ "version": "1.0.0" }` and no `openclaw.build` block will now receive a `ConvexError` at publish time.
Note that `normalizeOpenClawExternalPluginCompatibility` (used for display/compatibility data) still retains the fallback, creating a split: the compatibility data for previously-published releases reflects the fallback value, but the validation gate now prevents those same packages from being published again.
Please confirm this is deliberate and consider adding a migration note or a more informative error message. Is the removal of the `package.json.version` fallback for `openclaw.build.openclawVersion` intentional? If so, is a migration guide or clearer error message planned?
How can I resolve this? If you propose a fix, please make it concise.| }) { | ||
| const [isDragging, setIsDragging] = useState(false); | ||
| const archiveInputRef = useRef<HTMLInputElement | null>(null); | ||
| const directoryInputRef = useRef<HTMLInputElement | null>(null); |
There was a problem hiding this comment.
is-ready class applied even when validationError is non-null
The old inline code computed isMetadataLocked as:
const isMetadataLocked = files.length === 0 || Boolean(validationError);and used it to suppress the is-ready CSS class on plugin-upload-summary. In the extracted component the check is now:
const isMetadataLocked = props.files.length === 0;So when props.validationError is set (e.g., file-size limit exceeded) the summary will still receive the is-ready class, giving the upload panel a visually "ready" appearance while actually blocking the form.
| const directoryInputRef = useRef<HTMLInputElement | null>(null); | |
| const isMetadataLocked = props.files.length === 0 || Boolean(props.validationError); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/PackageSourceChooser.tsx
Line: 23
Comment:
**`is-ready` class applied even when `validationError` is non-null**
The old inline code computed `isMetadataLocked` as:
```ts
const isMetadataLocked = files.length === 0 || Boolean(validationError);
```
and used it to suppress the `is-ready` CSS class on `plugin-upload-summary`. In the extracted component the check is now:
```ts
const isMetadataLocked = props.files.length === 0;
```
So when `props.validationError` is set (e.g., file-size limit exceeded) the summary will still receive the `is-ready` class, giving the upload panel a visually "ready" appearance while actually blocking the form.
```suggestion
const isMetadataLocked = props.files.length === 0 || Boolean(props.validationError);
```
How can I resolve this? If you propose a fix, please make it concise.![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| #### 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) |
There was a problem hiding this comment.
| [`/.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) |
Documentation contains hardcoded local absolute path /Users/tengjizhang/.codex/worktrees/7d03/clawhub/.github/workflows/package-publish.yml from a developer's machine.
| if: secrets.clawhub_token != '' | ||
| env: | ||
| CLAWHUB_TOKEN: ${{ secrets.clawhub_token }} | ||
| CLAWHUB_REGISTRY: ${{ inputs.registry }} | ||
| run: | |
There was a problem hiding this comment.
| if: secrets.clawhub_token != '' | |
| env: | |
| CLAWHUB_TOKEN: ${{ secrets.clawhub_token }} | |
| CLAWHUB_REGISTRY: ${{ inputs.registry }} | |
| run: | | |
| 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 |
Invalid GitHub Actions syntax: if: secrets.clawhub_token != '' directly references secrets in a conditional without proper expression syntax, which is not supported.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83708f24ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not source: | ||
| source = os.environ["GITHUB_REPOSITORY"] | ||
| ref = os.environ["INPUT_REF"].strip() | ||
| if ref and "@" not in source and not source.startswith("http"): | ||
| source = f"{source}@{ref}" |
There was a problem hiding this comment.
In package-publish.yml's "Resolve publish command" step, when callers rely on defaults (inputs.source empty and inputs.ref empty), source becomes just GITHUB_REPOSITORY and no ref is appended. clawhub package publish owner/repo resolves to the repo default branch, so tag-triggered or non-default-branch runs can publish the wrong commit. This is especially problematic because the documented caller examples do not pass ref, so trusted release workflows can silently publish mismatched code.
Useful? React with 👍 / 👎.
| 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(); |
There was a problem hiding this comment.
buildSource prefers options.sourcePath before inferred path, but the CLI defines --source-path with a default of ".", so inferred subpaths are never used in normal CLI invocation. Publishing from a monorepo subdirectory or a GitHub tree/blob URL therefore records source.path as "." even when files were taken from a nested path, making source attribution incorrect and potentially breaking downstream verification that relies on accurate path metadata.
Useful? React with 👍 / 👎.
| } finally { | ||
| await plan?.cleanup?.(); |
There was a problem hiding this comment.
Cleanup is only invoked via plan?.cleanup in cmdPublishPackage, but plan is assigned only after preparePackagePublishPlan returns successfully. If preparation fails after downloading GitHub contents (for example, missing required package metadata), the temp directory created by fetchGitHubSource is never removed. Repeated failed publishes can accumulate stale /tmp/clawhub-github-publish-* directories and waste disk in CI/automation environments.
Useful? React with 👍 / 👎.
1 participant
Summary
clawhub package publishwith dry-run/json support and non-polluting CI coveragepackages/clawdhubverification architecture so these publish changes are backed by explicit package-local tests and artifact smoke checksRoot cause
Plugin publishing had drifted across the CLI, web UI, docs, and OpenClaw runtime contract. Authors were sent into the legacy skill flow, the web upload interaction could open multiple pickers, and required compatibility metadata was only enforced deep in backend validation.
What changed
clawhub package publishfor local folders, GitHub repos, and GitHub URLsclawhub skill publishas the preferred namespaced skill command and keep bareclawhub publishas the legacy aliasclawhub package publishclawhub-schema/packages/clawdhubso publish validation is consistent in the backend, CLI dry-run, and web preflightpackages/clawdhubonto an explicit package-local verification model with source tests, build verification, and artifact smoke testsValidation
bun run lintbunx tsc -p packages/schema/tsconfig.json --noEmitbunx tsc -p packages/clawdhub/tsconfig.json --noEmitbun run test