Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
@deepujain is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
Codex review: needs real behavior proof before merge. What this changesThe PR parses a single top-level env name into required environment metadata while preserving structured metadata precedence. Example: Publish a skill containing env: RISKSTATE_API_KEY without a structured metadata block.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness⛔ Blocked before merge - 1 item remains Keep open: current main still drops scalar env declarations, and this PR provides a focused repair with no identified code defects. Priority: P2 Before merge
FindingsNone. Tests
Agent review detailsHow this fits togetherThe skill publishing pipeline parses SKILL.md frontmatter into stored version metadata consumed by the setup API, installation card, and security scanner. flowchart LR
A[Published SKILL.md] --> B[Frontmatter parser]
B --> C[Metadata normalization]
C --> D[Stored version metadata]
D --> E[Installation guidance]
D --> F[Scanner prompt]
Technical reviewBest possible solution: Land the narrow normalization in the existing fallback while retaining canonical metadata precedence and leaving historical versions unchanged. Do we have a high-confidence way to reproduce the issue? Yes at source level: main routes env: RISKSTATE_API_KEY into an array-only parser, which discards it; no target code or tests were run during this read-only review. Is this the best way to solve the issue? Best-fix verdict: best; normalizing only the top-level scalar preserves existing structured declarations, whereas changing the shared declaration parser would also broaden namespaced inputs and changing scanner or UI consumers would duplicate normalization. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against c9a9fe2ed4c4. Provenance checked
TestingProof path: unit tests only. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 29, 2026, 4:03 AM ET / 08:03 UTC. |
Recognize a single top-level env name as a required declaration for installation guidance and scanner input. Preserve metadata precedence and reject malformed scalar declarations. Fixes openclaw#1270 Signed-off-by: Deepak Jain <deepujain@gmail.com>
bef387a to
ac738e0
Compare
Closes #1270.
What Problem This Solves
A skill declaring
env: RISKSTATE_API_KEYloses that credential declaration when ClawHub builds its installation and scanner metadata.User Impact
Newly published versions retain the key as a required credential in installation guidance and scanner input. Stored metadata is not backfilled; publishing a new version refreshes it.
Why This Change Was Made
The parser already accepts top-level env arrays. This extends that fallback to one valid variable name while preserving structured metadata precedence and optional declarations. Assignments and lists in scalar text are ignored. The documentation continues to recommend
metadata.openclaw.requires.envfor new skills.There is no schema migration or CLI release. The backend change takes effect through the normal manual deployment.
Evidence
The scanner regression fails on
mainat907b6398because the required key is missing, and passes onac738e07.On
ac738e07, the shipped ClawHub CLI publishedscalar-env-proof@1.0.0to a disposable local Convex backend running this revision. Its SKILL.md declaredenv: RISKSTATE_API_KEY. The public skill API returned this installation setup:[{"key":"RISKSTATE_API_KEY","required":true}]The scanner claim API returned the published version's queued job. The worker downloaded SKILL.md, verified its hash, and retained this normalized version metadata in its workspace:
{"envVars":[{"name":"RISKSTATE_API_KEY","required":true}]}Convex typechecking passed before publication. Prepublication scan results were simulated using the repository's local-test protocol, with the VirusTotal wait set to zero. No external model verdict, VirusTotal scan, or production publication was run.
bunx vitest run convex/lib/skills.test.ts convex/lib/securityPrompt.test.ts --maxWorkers=2: 63 tests passed.bun run ci:static: passed.bun run ci:unit --maxWorkers=2on Linux: 8,366 tests passed, five skipped, and coverage thresholds passed.bun run ci:types-build: passed, including root, schema, CLI and admin TypeScript checks and the production build.Bun 1.4.4 was used throughout, with Node 24.22.1 for the runtime and build checks and Node 22.19.0 for Linux unit validation. The macOS coverage run timed out in the unchanged Mermaid fallback test; the complete gate passed on Linux.
Evidence for ClawSweeper's publication-proof request is included above. Current-head automated review is pending.
AI-assisted contribution. I reviewed the implementation and tests and can maintain this change.