Skip to content

fix: sanitize static publish scan metadata - #3593

Open
morajabi wants to merge 1 commit into
openclaw:mainfrom
morajabi:fix/convex-schema-metadata-publish
Open

morajabi wants to merge 1 commit into
openclaw:mainfrom
morajabi:fix/convex-schema-metadata-publish

Conversation

@morajabi

@morajabi morajabi commented Sep 4, 2026

Copy link
Copy Markdown

Summary

  • normalize package metadata before crossing the Convex action boundary into the Node static scanner
  • add a regression assertion for a valid plugin manifest containing a JSON Schema $schema key

Why

Publishing an exact ClawPack with a channel config schema currently fails before release insertion with:

Field name $schema starts with a '$', which is reserved.

The package publish path already sanitizes extracted manifests before database storage, but the new Node-runtime scan hop receives the raw metadata first. This applies the same existing toConvexSafeJsonValue normalization at that boundary. Package files remain unchanged and are still scanned from storage; only the duplicate structured metadata projection is normalized.

Tests

  • bunx vitest run convex/packages.public.test.ts --testNamePattern 'scans plugin publishes and forwards scan status to insertReleaseInternal'
  • bun run ci:static
  • bun run ci:types-build
  • npx --yes -p node@24 -c 'bun run ci:unit' (6,334 passed, 3 skipped)

@morajabi
morajabi requested review from a team and Patrick-Erichsen as code owners September 4, 2026 12:23
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

@morajabi is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 4, 2026
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 13, 2026, 6:03 AM ET / 10:03 UTC (Revision 4).

ClawSweeper review

What this changes

This PR normalizes manifest metadata before sending it to the Node-based package scanner and adds coverage for a nested JSON Schema key.

Merge readiness

Blocked before merge - 5 items remain

Current main addresses the reported failure through merged #3594, whose discussion includes successful production publish evidence. However, the required formal fixing relationship to this request was not established, so automatic closure is not justified.

Priority: P2
Reviewed head: 84118052f95a4d267d82c302d43423a93a94e87f
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused with no blocking finding, but its own runtime proof remains absent.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: This branch's package-to-scanner normalization is exercised through mocked actions and storage, without an observed real Convex publish reaching release insertion. The production trace on the merged alternative proves its JSON transport, not this implementation. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: This branch's package-to-scanner normalization is exercised through mocked actions and storage, without an observed real Convex publish reaching release insertion. The production trace on the merged alternative proves its JSON transport, not this implementation. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 10 items Verified introduced change: The pinned base-to-head diff adds one production line and 25 test lines across two files; it normalizes metadata and asserts that nested $schema becomes dollar_schema.
Current main handles reserved metadata keys: The publishing helper JSON-encodes the entire metadata object, including plugin manifests, before the action call. The receiving Node action parses that string before invoking the scanner, preserving nested keys.
Merged implementation provenance: GitHub verifies that #3594 merged as 3aba7d1. Its patch changes both transport endpoints and adds reserved-key and scanner-semantics coverage; its stated problem is the same publish-time serialization error.
Findings None None.
Security None None.

How this fits together

ClawHub scans uploaded package files and extracted manifests before recording a release. The publishing action passes those inputs into a Node action and uses the returned moderation verdict.

flowchart LR
  A[Uploaded package] --> B[Extract manifests]
  B --> C[Encode scan metadata]
  C --> D[Node scan action]
  A --> E[Stored package files]
  E --> D
  D --> F[Moderation verdict]
  F --> G[Release processing]
Loading

Decision needed

Question Recommendation
Should this branch be retired in favor of the merged JSON transport, or is there a distinct channel-schema failure still requiring work? Retire the redundant branch: Confirm that #3594 covers this request and close this PR manually.

Why: The observable problem is covered by current source and production evidence, but the formal relationship required for automatic PR closure is unestablished.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: This branch's package-to-scanner normalization is exercised through mocked actions and storage, without an observed real Convex publish reaching release insertion. The production trace on the merged alternative proves its JSON transport, not this implementation. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - No formal fixing relationship was established for automatic closure despite strong behavioral overlap.
  • Resolve merge risk (P1) - GitHub reports merge conflicts; any retained implementation needs review after conflict resolution.
  • Complete next step (P2) - Confirm whether the merged JSON transport fully replaces this PR; if retaining it, identify the remaining failure, resolve conflicts, and provide real publish proof with private details redacted.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +1/-0, tests +25/-0; 2 files The production change is narrowly scoped and accompanied by a nested-schema regression assertion.

Root-cause cluster

Relationship: same_root_cause
Canonical: #3594
Summary: Both patches address reserved manifest keys crossing the same Convex-to-Node scan boundary; the JSON transport alternative is merged.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Preserve main's lossless JSON transport so nested manifest keys reach the scanner unchanged.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Preserve main's lossless JSON transport so nested manifest keys reach the scanner unchanged.

Do we have a high-confidence way to reproduce the issue?

No current-main reproduction is established: current main serializes metadata as JSON, and the merged repair has a reported successful production publish. No runtime tests were executed during this read-only review.

Is this the best way to solve the issue?

The branch is a plausible mitigation, but main's JSON round trip is preferable because it preserves metadata keys rather than renaming them.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 8c2de6c506bb.

Labels

Label justifications:

  • P2: This addresses a bounded package-publishing failure for manifests containing reserved keys.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: This branch's package-to-scanner normalization is exercised through mocked actions and storage, without an observed real Convex publish reaching release insertion. The production trace on the merged alternative proves its JSON transport, not this implementation. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Verified introduced change: The pinned base-to-head diff adds one production line and 25 test lines across two files; it normalizes metadata and asserts that nested $schema becomes dollar_schema. (convex/packages.ts:1013, 84118052f95a)
  • Current main handles reserved metadata keys: The publishing helper JSON-encodes the entire metadata object, including plugin manifests, before the action call. The receiving Node action parses that string before invoking the scanner, preserving nested keys. (convex/packages.ts:1047, 8c2de6c506bb)
  • Merged implementation provenance: GitHub verifies that fix(publish): carry scan metadata as JSON across the Node action boundary #3594 merged as 3aba7d1. Its patch changes both transport endpoints and adds reserved-key and scanner-semantics coverage; its stated problem is the same publish-time serialization error. (convex/staticPublishScanNode.ts:16, 3aba7d1b0088)
  • Fix timestamp: The merged transport fix records 2026-09-04T06:10:36-07:00, equivalent to 2026-09-04T13:10:36Z. (convex/packages.ts, 3aba7d1b0088)
  • Production and release distinction: Containing tags include prod/v2026.09.04.1 and prod/v2026.09.09.1. The latest CLI release remains v0.23.3, published before this backend fix; it should not be described as the fixing CLI release. (3aba7d1b0088)
  • Positive production evidence for the merged alternative: fix(publish): carry scan metadata as JSON across the Node action boundary #3594 (comment) reports a production Matrix publish crossing the Node scanner, creating a release, completing publication checks, and returning HTTP 200 from the version endpoint. This supports the merged JSON transport, not this branch's normalization implementation.

Likely related people:

  • Peter Steinberger: Raw commit ab27fba adds convex/staticPublishScanNode.ts:9 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: ab27fba9b51b; files: convex/staticPublishScanNode.ts)
  • Patrick-Erichsen: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • If retained for distinct remaining behavior, provide a redacted real Convex publish transcript showing the nested $schema manifest crossing the scanner boundary and reaching release insertion.
  • Update the PR body with proof to trigger re-review; if it does not trigger, ask a maintainer to comment @clawsweeper re-review.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (3 earlier review cycles)
  • reviewed 2026-09-04T12:26:37.266Z sha 8411805 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-05T01:05:09.678Z sha 8411805 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-09T13:33:34.323Z sha 8411805 :: needs real behavior proof before merge. :: none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant