Skip to content

refactor(skills): centralize internal write paths#108482

Open
tadad wants to merge 4 commits into
openclaw:mainfrom
tadad:codex/skills-write-service-api
Open

refactor(skills): centralize internal write paths#108482
tadad wants to merge 4 commits into
openclaw:mainfrom
tadad:codex/skills-write-service-api

Conversation

@tadad

@tadad tadad commented Jul 15, 2026 •
edited
Loading

Copy link
Copy Markdown

What Problem This Solves

Core skill-content mutations currently enter through several low-level Workshop and lifecycle helpers. That makes ownership difficult to enforce, and successful lifecycle installs do not all explicitly invalidate the skills snapshot.

Why This Change Was Made

This introduces one internal skillsWriteService owner for proposal creation/application, full-bundle installation, and explicit snapshot refresh. The Workshop tool, CLI, autocapture, existing Gateway proposal handlers, ClawHub, source installs, uploaded archives, and install-spec completion now converge on that boundary.

The PR deliberately adds no new public Gateway or Plugin SDK contract. Existing skills.proposals.*, skills.upload.*, and skills.install methods remain canonical. The earlier parallel skills.write.* RPC family, direct-write capability, protocol/native-client changes, and public-API documentation were removed before shipping; direct/plugin exposure requires a separately approved policy and API design.

An architecture test prevents production callers from bypassing the service through the low-level proposal, active-workspace writer, or archive-install entry points.

User Impact

There is no new public API and no migration requirement. Existing proposal and install workflows keep their current contracts, while successful ClawHub, source, upload, and dependency installs now explicitly refresh the skills snapshot.

AI-assisted: this change was implemented and reviewed with Codex.

Evidence

  • corepack pnpm build — passed locally, including package builds, CLI bootstrap guard, Plugin SDK export verification, and Control UI build
  • node scripts/run-vitest.mjs src/skills/write-service.test.ts src/skills/write-service-boundary.test.ts src/gateway/skills-write-service.e2e.test.ts src/gateway/server-methods/skills.proposals.test.ts src/gateway/server-methods/skills-upload.test.ts src/skills/lifecycle/source-install.test.ts src/skills/lifecycle/install.test.ts src/skills/lifecycle/clawhub.test.ts — 114 tests passed across unit, Gateway, and E2E shards
  • node scripts/run-vitest.mjs src/agents/tools/skill-workshop-tool.test.ts src/cli/skills-cli.workshop.test.ts src/skills/research/autocapture.test.ts — 56 tests passed
  • Targeted oxfmt, oxlint, and git diff --check — passed
  • Strict maintainability review — no findings
  • .agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --stream-engine-output — clean; no accepted/actionable findings, patch correct at 0.91 confidence

Built-product smoke used dist/entry.js gateway run with an isolated temporary state directory, config, home, and workspace. It connected through the built @openclaw/gateway-client package over a real WebSocket. No user environment files, Blacksmith, or Testbox were used.

PASS connect built_gateway=true isolated_state=true admin_scope=true
PASS propose method=skills.proposals.create status=pending
PASS apply method=skills.proposals.apply status=applied file_verified=true
PASS install method=skills.install source=upload bundle_files=2 file_verified=true
PASS refresh method=skills.status visible_skills=2
RESULT skills_write_service_live_smoke=pass

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: android App: android app: web-ui App: web-ui gateway Gateway runtime cli CLI command changes agents Agent runtime and tooling size: XL labels Jul 15, 2026
@github-actions github-actions Bot mentioned this pull request Jul 15, 2026
Open
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 16, 2026
@clawsweeper

clawsweeper Bot commented Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 10:54 PM ET / July 21, 2026, 02:54 UTC.

Summary
The branch centralizes internal skill proposal, bundle-install, and snapshot-refresh calls behind skillsWriteService, with regression coverage for Gateway, CLI, Workshop, autocapture, source, upload, ClawHub, and install flows.

PR surface: Source +111, Tests +328. Total +439 across 16 files.

Reproducibility: not applicable. this is an internal refactor rather than a reported failing user workflow. The supplied live Gateway smoke exercises the affected canonical proposal, upload-install, and status-refresh paths after the change.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: src/gateway/skills-write-service.e2e.test.ts, serialized state: src/skills/write-service.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Rebase or refresh the review against current main before merge.

Risk before merge

  • [P1] GitHub reports the branch as behind current main; refresh the rebase and focused boundary review against the landing head before merge.

Maintainer options:

  1. Decide the mitigation before merge
    Rebase onto current main, preserve the existing Gateway proposal/upload/install contracts, and land only after the refreshed diff still proves that all production skill-content mutation paths use the single internal boundary.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No discrete mechanical defect is established; the next action is ordinary maintainer review of a refreshed landing-head diff.

Security
Cleared: The reviewed diff adds no dependency, workflow, secret, permission, package-resolution, or public authorization surface; it narrows internal routing while retaining existing Gateway validation paths.

Review details

Best possible solution:

Rebase onto current main, preserve the existing Gateway proposal/upload/install contracts, and land only after the refreshed diff still proves that all production skill-content mutation paths use the single internal boundary.

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

Not applicable: this is an internal refactor rather than a reported failing user workflow. The supplied live Gateway smoke exercises the affected canonical proposal, upload-install, and status-refresh paths after the change.

Is this the best way to solve the issue?

Yes, provisionally: retaining existing public Gateway methods while consolidating internal write ownership is narrower and safer than the earlier parallel public RPC approach. A current-main rebase review remains necessary before merge.

AGENTS.md: unclear because the file could not be read completely.

Codex review notes: model internal, reasoning high; reviewed against b6865e9bce49.

Label changes

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit. Replaced prior rating: 🦐 gold shrimp.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: This is a bounded internal skills and Gateway refactor with normal user-facing cache-refresh implications but no demonstrated urgent regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit. Replaced prior rating: 🦐 gold shrimp.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body provides after-fix built-product live output from a real Gateway WebSocket flow covering proposal creation/application, uploaded-bundle installation, and observable snapshot refresh.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix built-product live output from a real Gateway WebSocket flow covering proposal creation/application, uploaded-bundle installation, and observable snapshot refresh.
Evidence reviewed

PR surface:

Source +111, Tests +328. Total +439 across 16 files.

View PR surface stats
Area Files Added Removed Net
Source 9 143 32 +111
Tests 7 329 1 +328
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 16 472 33 +439

What I checked:

  • Public-contract preservation: The PR description and src/gateway/server-methods/skills.ts diff replace internal proposal helper calls with skillsWriteService while retaining the existing skills.proposals.* handlers rather than adding the earlier parallel skills.write.* RPC family. (src/gateway/server-methods/skills.ts:404, 9532591d82ef)
  • Write-path coverage: The diff routes Workshop, CLI, Gateway, autocapture, ClawHub, source install, upload install, and lifecycle installs through the new internal boundary or its refresh method, with focused tests covering snapshot-version updates. (src/skills/write-service.ts:1, 9532591d82ef)
  • Real behavior proof: The PR body records an after-fix built-product Gateway smoke over a real WebSocket that created and applied a proposal, installed an uploaded bundle, and observed refreshed skill status. (src/gateway/skills-write-service.e2e.test.ts:1, 9532591d82ef)
  • Review freshness: GitHub reports the PR as cleanly mergeable but behind its base; the branch should be rebased or receive a current-main review refresh before landing because its listed base SHA predates the supplied current-main SHA. (998f0ed27d81)

Likely related people:

  • tadad: The supplied PR history attributes all four commits that introduced and then narrowed the internal skills-write boundary to this contributor; current-main feature ownership could not be confirmed from the available read-only command path. (role: proposed boundary implementer; confidence: low; commits: cb03fe64250d, ce529ffb8376, bb12a10a9e37; files: src/skills/write-service.ts, src/skills/write-service-boundary.test.ts, src/gateway/server-methods/skills.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (4 earlier review cycles)
  • reviewed 2026-07-15T22:47:19.464Z sha 1e1c844 :: needs real behavior proof before merge. :: [P1] Reuse the existing proposal RPC contract
  • reviewed 2026-07-15T23:37:22.159Z sha 1e1c844 :: needs real behavior proof before merge. :: [P1] Keep proposal mutations on the existing Gateway API | [P1] Add idempotency to every mutation request | [P2] Expose the promised API through the plugin SDK boundary
  • reviewed 2026-07-20T18:52:00.447Z sha e5579ac :: needs maintainer review before merge. :: none
  • reviewed 2026-07-20T19:34:20.590Z sha e5579ac :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 16, 2026
@tadad
tadad force-pushed the codex/skills-write-service-api branch from 1e1c844 to b1e696e Compare July 20, 2026 18:25
@openclaw-barnacle openclaw-barnacle Bot added size: M and removed docs Improvements or additions to documentation app: android App: android app: web-ui App: web-ui size: XL labels Jul 20, 2026
@tadad tadad changed the title feat: add a safe API for writing skills refactor(skills): centralize internal write paths Jul 20, 2026
@tadad

tadad commented Jul 20, 2026

Copy link
Copy Markdown
Author

Addressed the architectural findings in e5579ac by narrowing the PR to the internal consolidation path:

  • Removed all five new skills.write.* methods, their protocol/native-client surface, and the overlapping proposal API.
  • Kept the existing skills.proposals.*, upload, and install RPCs as the only public contracts.
  • Removed direct active-skill writes and validation code that no longer had a production caller, so there is no new security gate or idempotency contract to define here.
  • Renamed the core surface from src/skills/api to the explicitly internal src/skills/write-service.ts; this PR no longer claims to expose a Plugin SDK API.
  • Retained the useful consolidation: Workshop, CLI, autocapture, existing Gateway proposal handlers, ClawHub, source install, archive upload, and install-spec refresh now converge on skillsWriteService.
  • Reworked the E2E and built-product smoke to exercise the real canonical Gateway methods. Redacted live output is now in the PR body.

The effective branch diff against origin/main passed the full local build, focused tests, strict maintainability review, and a fresh Codex autoreview with no accepted/actionable findings.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed 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 Jul 21, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 21, 2026
@tadad
tadad force-pushed the codex/skills-write-service-api branch from e5579ac to bb12a10 Compare July 21, 2026 02:36
@tadad

tadad commented Jul 21, 2026

Copy link
Copy Markdown
Author

CI-readiness update for bb12a10, rebased onto main at 83ca537:

  • Removed the reported static import cycle by deferring the Workshop implementation load inside the write service while preserving one canonical skillsWriteService boundary.
  • Made the internal service input/result aliases private, clearing the unused-export failures.
  • Strengthened the boundary test so guarded low-level modules cannot be bypassed with either static or dynamic imports.
  • Re-audited production write ownership: agent tool, CLI, Gateway, and autocapture proposal/apply callers use skillsWriteService; ClawHub, source, upload, and lifecycle archive-install paths use skillsWriteService. Snapshot-only refresh consumers remain separate because they do not mutate skill content.

Local proof on the pushed head:

  • check:import-cycles: 0 cycles
  • check:madge-import-cycles: 0 cycles
  • deadcode:exports: 0 entries
  • deadcode:dependencies: passed
  • check:architecture: passed
  • focused skill/Gateway/CLI/agent regression suite: 170 tests passed across 5 Vitest shards
  • production and test TypeScript checks: passed
  • targeted oxlint, oxfmt, and git diff checks: passed
  • full branch autoreview: no actionable findings
  • commit hook formatting and TruffleHog scan: passed, 0 secrets

The PR remains draft intentionally. Full GitHub CI, CodeQL, label/API guards, and the Docker-backed QA profile still need to run after it is marked ready for review; the prior local Docker limitation is not represented as proof of those lanes.

@tadad
tadad marked this pull request as ready for review July 21, 2026 02:49
@BradGroux BradGroux mentioned this pull request Jul 21, 2026
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling cli CLI command changes gateway Gateway runtime P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: L status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@tadad