From 7d4d02fff028af147a3f0fb91f4a014d766716fc Mon Sep 17 00:00:00 2001 From: Ayaan Zaidi Date: Mon, 10 Aug 2026 13:50:33 +0530 Subject: [PATCH] feat(skills): improve used skills autonomously Teach the semantic reviewer to improve skills the agent actually used, keep review input provider-bound, and preserve bounded deterministic retries. Co-authored-by: Ayaan Zaidi --- docs/tools/self-learning.md | 40 +++-- docs/tools/skill-workshop.md | 20 +-- .../agent-tools.before-tool-call.e2e.test.ts | 5 + .../agent-tools.before-tool-call.wrapper.ts | 9 + .../harness/agent-end-side-effects.test.ts | 12 ++ src/agents/harness/agent-end-side-effects.ts | 3 + .../tools/skill-workshop-tool.review.test.ts | 72 +++++++- src/agents/tools/skill-workshop-tool.ts | 72 ++++---- src/skills/runtime/run-usage.ts | 35 ++++ .../experience-review-auto-apply.test.ts | 10 +- .../workshop/experience-review-prompt.ts | 52 +++++- src/skills/workshop/experience-review.test.ts | 170 +++++++++++++++++- src/skills/workshop/experience-review.ts | 46 +++-- src/skills/workshop/service.ts | 6 +- src/skills/workshop/types.ts | 4 +- 15 files changed, 459 insertions(+), 97 deletions(-) create mode 100644 src/skills/runtime/run-usage.ts diff --git a/docs/tools/self-learning.md b/docs/tools/self-learning.md index b623a75f5fa4..5d64dfeb71b5 100644 --- a/docs/tools/self-learning.md +++ b/docs/tools/self-learning.md @@ -53,19 +53,21 @@ A later foreground completion in the same session restarts the quiet period. Only one experience review runs at a time. The foreground answer is never delayed. The reviewer is isolated and biased toward small, well-evidenced captures. It -sees a bounded workspace skill list, can list or inspect proposals, and can read -a bounded excerpt of a writable skill for context. It drafts at most one pending -proposal: preferring to revise a matching pending proposal, then to patch the -existing skill governing the work, and creating a new skill only when nothing -covers the class. A patch proposal quotes the exact live text to change (or -appends a new section) and the tool composes the full body inside the same read -that hash-binds the proposal, so untouched content survives by construction and -patches auto-apply in `auto` mode. A patch requires a full-skill read receipt: -skills beyond the bounded read budget cannot be patched autonomously. A full-body update rewrite always stays -pending for operator review. Its one-mutation budget is shared across retries. Every -mutation is a pending proposal — it never writes a live skill directly and -cannot apply, reject, quarantine, message, or use general agent tools. The -reviewed trajectory is evidence, not instructions. +receives an authoritative receipt of the skills the foreground run actually +read or command-invoked, plus a bounded workspace skill list. It prefers a used +writable skill when that skill governs the learning, then another existing +skill, and creates a new skill only when nothing covers the class. + +Before changing an existing skill, the reviewer must read its complete current +body. Both targeted patches and full-body rewrites bind the proposal to that +read's content hash. Skills beyond the bounded read budget cannot be updated +autonomously. A patch quotes the exact live text to change, while a rewrite must +preserve everything still useful. In `auto` mode, either form goes through the +same scanner-gated apply path without operator review. The one-mutation budget +is shared across retries. The reviewer cannot apply, reject, quarantine, +message, or use general agent tools itself; the orchestrating pipeline applies +the finished capture only after the isolated review ends. The reviewed +trajectory is evidence, not instructions. Good candidates include: @@ -88,11 +90,11 @@ The reviewer should abstain for: ## Mode policy -| Mode | Capture behavior | -| --------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `off` | Does not create experience-review captures. | -| `propose` | Creates or revises pending proposals. Nothing applies automatically. | -| `auto` | Creates or revises proposals, then applies new-skill and patch proposals through the normal Workshop apply path. Full-body updates stay pending for review. This is the default. | +| Mode | Capture behavior | +| --------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `off` | Does not create experience-review captures. | +| `propose` | Creates or revises pending proposals. Nothing applies automatically. | +| `auto` | Creates or revises proposals, then applies every autonomous capture through the normal scanner-gated Workshop path. No operator review is required. This is the default. | Set the mode with the CLI: @@ -135,6 +137,8 @@ Every learned skill receives these controls: and extra-root skills remain outside the write boundary. - **Hash binding:** update proposals bind to the current live skill and go stale if that target changes before apply. +- **Read before update:** the reviewer must read the complete current skill + before either a targeted patch or a full-body rewrite. - **Rollback metadata:** apply records the prior skill and support-file contents before the live write. - **Curator lifecycle:** learned skills unused for 30 days become stale and after diff --git a/docs/tools/skill-workshop.md b/docs/tools/skill-workshop.md index 8defa2856491..bda658ff6b81 100644 --- a/docs/tools/skill-workshop.md +++ b/docs/tools/skill-workshop.md @@ -302,13 +302,13 @@ are stored in the shared OpenClaw state database; transcript content is not copi into scan state. In `propose` and `auto` modes, OpenClaw can also perform a conservative review after successful, -substantial work and after the whole agent system becomes idle. That isolated review can draft at -most one pending proposal — a new skill, a patch of an existing workspace skill, a full-body -update, or a revision of a pending proposal. It never writes a live skill directly and cannot -apply, reject, or quarantine a proposal. Patch proposals quote the exact live text to change; the -tool composes the full body from the live skill. In `auto` mode, the orchestrating capture -pipeline applies new-skill and patch results afterward through the normal scanner-gated service; -full-body update proposals always stay pending for operator review. +substantial work and after the whole agent system becomes idle. The review receives an +authoritative receipt of skills the foreground run actually used. It can draft at most one pending +proposal: a new skill, a patch or full-body rewrite of an existing workspace skill, or a revision +of a pending proposal. Existing skills must be read before either update form, and the proposal is +bound to that exact content hash. The reviewer never writes a live skill directly and cannot +apply, reject, or quarantine a proposal. In `auto` mode, the orchestrating pipeline applies every +autonomous result afterward through the normal scanner-gated service, without operator review. See [Self-learning](/tools/self-learning) for enablement, eligibility, privacy and cost details, the proposal threshold, and troubleshooting. @@ -343,9 +343,9 @@ In `propose` and `auto` modes, an isolated run of the selected model decides whe completed trajectory clears the evidence-gated proposal bar. The foreground model is not prompted to learn before it replies. The background reviewer preserves the foreground run as proposal provenance, cannot access general agent tools, and cannot make lifecycle decisions. In `auto` -mode, the capture pipeline applies resulting new-skill and patch proposals only after the -isolated run completes; full-body update proposals always stay pending for operator review, -because the reviewer authors them without a mechanical preservation guarantee. The review starts +mode, the capture pipeline applies every autonomous proposal only after the isolated run +completes. Existing-skill changes require a complete read receipt and content-hash binding before +they are eligible for that apply step. The review starts only when the foreground runtime reports its resolved model and that `skill_workshop` was actually available. Restrictive or unknown tool policy therefore fails closed and creates no proposal. diff --git a/src/agents/agent-tools.before-tool-call.e2e.test.ts b/src/agents/agent-tools.before-tool-call.e2e.test.ts index 504239a9303d..dc69c0406ce4 100644 --- a/src/agents/agent-tools.before-tool-call.e2e.test.ts +++ b/src/agents/agent-tools.before-tool-call.e2e.test.ts @@ -39,6 +39,7 @@ import { createHookRunner, type HookRunner } from "../plugins/hooks.js"; import { createEmptyPluginRegistry } from "../plugins/registry-empty.js"; import { setActivePluginRegistry } from "../plugins/runtime.js"; import { setPluginToolMeta } from "../plugins/tools.js"; +import { consumeRunSkillUsage } from "../skills/runtime/run-usage.js"; import { createCanonicalFixtureSkill } from "../skills/test-support/test-helpers.js"; import { createChannelTestPluginBase, createTestRegistry } from "../test-utils/channel-plugins.js"; import { @@ -1495,6 +1496,10 @@ describe("before_tool_call loop detection behavior", () => { expect(JSON.stringify(emitted)).not.toContain("SKILL.md"); expect(JSON.stringify(emitted)).not.toContain(skillBaseDir); expect(privateData[0]?.skillUsage?.skillFile).toBe(skillFilePath); + expect(consumeRunSkillUsage("run-1")).toEqual([ + { name: "demo-skill", source: "workspace", activation: "read" }, + ]); + expect(consumeRunSkillUsage("run-1")).toEqual([]); }); }); diff --git a/src/agents/agent-tools.before-tool-call.wrapper.ts b/src/agents/agent-tools.before-tool-call.wrapper.ts index 52da7af64905..ea8509e88c7e 100644 --- a/src/agents/agent-tools.before-tool-call.wrapper.ts +++ b/src/agents/agent-tools.before-tool-call.wrapper.ts @@ -14,6 +14,7 @@ import { } from "../infra/diagnostic-trace-context.js"; import { pruneMapToMaxSize } from "../infra/map-size.js"; import { copyPluginToolMeta, getPluginToolMeta } from "../plugins/tools.js"; +import { recordRunSkillUsage } from "../skills/runtime/run-usage.js"; import { buildToolContentPrivateData, emitSkillUsedDiagnostic, @@ -545,6 +546,14 @@ export function wrapToolWithBeforeToolCallHook( toolParams: executeParams, ctx, }); + if (skillMatch) { + recordRunSkillUsage({ + runId: ctx?.runId, + name: skillMatch.skillName, + source: skillMatch.skillSource, + activation: skillMatch.activation, + }); + } if (hookOptions.emitDiagnostics) { if (skillMatch) { emitSkillUsedDiagnostic({ diff --git a/src/agents/harness/agent-end-side-effects.test.ts b/src/agents/harness/agent-end-side-effects.test.ts index b7ad1f31e48e..152d0bf7aee4 100644 --- a/src/agents/harness/agent-end-side-effects.test.ts +++ b/src/agents/harness/agent-end-side-effects.test.ts @@ -1,5 +1,6 @@ // Verifies agent-end side effects keep plugin hooks independent from experience review. import { beforeEach, describe, expect, it, vi } from "vitest"; +import { recordRunSkillUsage } from "../../skills/runtime/run-usage.js"; import { scheduleSkillExperienceReview } from "../../skills/workshop/experience-review-default.js"; import { awaitAgentEndSideEffects, runAgentEndSideEffects } from "./agent-end-side-effects.js"; import { @@ -28,6 +29,12 @@ describe("agent end side effects", () => { }); it("fires plugin agent_end hooks alongside experience review scheduling", async () => { + recordRunSkillUsage({ + runId: "run-1", + name: "release-runbook", + source: "workspace", + activation: "read", + }); runAgentEndSideEffects({ event: { messages: [], @@ -52,6 +59,11 @@ describe("agent end side effects", () => { expect(mockRunAgentEndHook).toHaveBeenCalledTimes(1); await vi.waitFor(() => expect(mockExperienceReview).toHaveBeenCalledTimes(1)); + expect(mockExperienceReview).toHaveBeenCalledWith( + expect.objectContaining({ + usedSkills: [{ name: "release-runbook", source: "workspace", activation: "read" }], + }), + ); }); it("still runs agent_end hooks when experience review scheduling fails", async () => { diff --git a/src/agents/harness/agent-end-side-effects.ts b/src/agents/harness/agent-end-side-effects.ts index 3c3f92dbb6c4..814d2ca45393 100644 --- a/src/agents/harness/agent-end-side-effects.ts +++ b/src/agents/harness/agent-end-side-effects.ts @@ -6,6 +6,7 @@ import type { ChatType } from "../../channels/chat-type.js"; * either fire-and-forget or awaited during tests/shutdown. */ import { createSubsystemLogger } from "../../logging/subsystem.js"; +import { consumeRunSkillUsage } from "../../skills/runtime/run-usage.js"; import { awaitAgentHarnessAgentEndHook, runAgentHarnessAgentEndHook, @@ -36,12 +37,14 @@ type AgentEndSideEffectsParams = Omit & { }; async function runCoreAgentEndSideEffects(params: AgentEndSideEffectsParams): Promise { + const usedSkills = consumeRunSkillUsage(params.ctx.runId); try { const { scheduleSkillExperienceReview } = await import("../../skills/workshop/experience-review-default.js"); scheduleSkillExperienceReview({ event: params.event, ctx: params.ctx, + usedSkills, ...(params.ctx.config ? { config: params.ctx.config } : {}), }); } catch (error) { diff --git a/src/agents/tools/skill-workshop-tool.review.test.ts b/src/agents/tools/skill-workshop-tool.review.test.ts index b09f059c2107..a37281fd5c7f 100644 --- a/src/agents/tools/skill-workshop-tool.review.test.ts +++ b/src/agents/tools/skill-workshop-tool.review.test.ts @@ -1,5 +1,6 @@ // skill_workshop review-mode tests cover the proposal-only reviewer surface: // mutation budgets, read receipts, and patch/update drafting for live skills. +import { writeFileSync } from "node:fs"; import fs from "node:fs/promises"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; @@ -115,6 +116,14 @@ describe("skill_workshop review mode", () => { updateProposals: true, proposalMutationBudget, }); + await expect( + reviewTool.execute("update-without-read", { + action: "update", + skill_name: "weather-planner", + proposal_content: "# Weather Planner\n\nCheck alerts and timing.\n", + }), + ).rejects.toThrow("read the live skill first"); + await reviewTool.execute("review-read", { action: "read", skill_name: "weather-planner" }); const update = await reviewTool.execute("review-update", { action: "update", skill_name: "weather-planner", @@ -183,7 +192,6 @@ describe("skill_workshop review mode", () => { kind: "update", skillKey: "weather-planner", }); - expect(proposalMutationBudget.patchProposalIds?.size).toBe(1); expect((extended.details as { description?: string }).description ?? "").not.toContain( "Replacement", ); @@ -234,6 +242,66 @@ describe("skill_workshop review mode", () => { expect(proposalMutationBudget.remaining).toBe(1); }); + it("refunds a stale update race so the reviewer can re-read and retry", async () => { + const workspaceDir = await tempDirs.make("openclaw-skill-workshop-stale-update-race-"); + await seedLiveSkill( + workspaceDir, + "weather-planner", + "Plan around the weather forecast", + "# Weather Planner\n\nCheck weather before outdoor recommendations.\n", + ); + + const liveSkillFile = path.join(workspaceDir, "skills", "weather-planner", "SKILL.md"); + const operatorEditedSkill = (await fs.readFile(liveSkillFile, "utf8")).replace( + "Check weather before outdoor recommendations.", + "Operator-edited steps during proposal creation.", + ); + let remaining = 1; + let mutateOnReserve = false; + const proposalMutationBudget: SkillWorkshopProposalMutationBudget = { + get remaining() { + return remaining; + }, + set remaining(value) { + remaining = value; + if (mutateOnReserve && value === 0) { + mutateOnReserve = false; + writeFileSync(liveSkillFile, operatorEditedSkill, "utf8"); + } + }, + }; + const reviewTool = createSkillWorkshopTool({ + workspaceDir, + proposalOnly: true, + updateProposals: true, + proposalMutationBudget, + }); + await reviewTool.execute("review-read", { action: "read", skill_name: "weather-planner" }); + + mutateOnReserve = true; + await expect( + reviewTool.execute("stale-update-race", { + action: "update", + skill_name: "weather-planner", + proposal_content: "# Weather Planner\n\nCheck alerts and timing.\n", + }), + ).rejects.toThrow("Skill changed since the reviewer's read"); + expect(proposalMutationBudget.remaining).toBe(1); + + await reviewTool.execute("review-read-again", { + action: "read", + skill_name: "weather-planner", + }); + const update = await reviewTool.execute("review-update-retry", { + action: "update", + skill_name: "weather-planner", + proposal_content: + "# Weather Planner\n\nOperator-edited steps during proposal creation.\nCheck alerts and timing.\n", + }); + expect(update.details).toMatchObject({ status: "pending", kind: "update" }); + expect(proposalMutationBudget.remaining).toBe(0); + }); + it("caps reviewer live-skill reads at the read budget", async () => { const workspaceDir = await tempDirs.make("openclaw-skill-workshop-review-read-cap-"); await seedLiveSkill( @@ -265,7 +333,7 @@ describe("skill_workshop review mode", () => { old_string: "A detailed operational line.", new_string: "A rewritten operational line.", }), - ).rejects.toThrow("cannot be patched autonomously"); + ).rejects.toThrow("cannot be updated autonomously"); }); it("does not refund the review mutation budget after a failed mutation", async () => { diff --git a/src/agents/tools/skill-workshop-tool.ts b/src/agents/tools/skill-workshop-tool.ts index 8f1625d331cd..0ab703b91a79 100644 --- a/src/agents/tools/skill-workshop-tool.ts +++ b/src/agents/tools/skill-workshop-tool.ts @@ -19,6 +19,7 @@ import { rejectSkillProposal, resolvePendingSkillProposal, reviseSkillProposal, + SkillProposalStaleTargetError, } from "../../skills/workshop/service.js"; import { SKILL_AUTHORING_STANDARDS_PROMPT } from "../../skills/workshop/skill-authoring-standards.js"; import type { @@ -113,7 +114,7 @@ function buildSkillWorkshopToolSchema( { action: stringEnum(proposalOnly ? proposalActions : [...SKILL_WORKSHOP_ACTIONS], { description: proposalOnly - ? `create = new skill;${updateProposals ? " patch = targeted find-and-replace on an existing live skill (quote the exact current text in old_string, replacement in new_string; empty old_string appends new_string at the end); read = bounded excerpt of an existing live skill (read before patching so you can quote it); update = full-body update proposal (stays pending for operator review);" : ""} revise = existing pending proposal; list/inspect discover pending proposals (not filesystem search).${supportsCompletion ? " complete = durably finish this review after all proposal work." : ""} Nothing writes a live skill directly; lifecycle actions are unavailable.` + ? `create = new skill;${updateProposals ? " patch = targeted find-and-replace on an existing live skill (quote the exact current text in old_string, replacement in new_string; empty old_string appends new_string at the end); read = bounded excerpt of an existing live skill (required before patch or update); update = full-body rewrite of an existing live skill after reading it;" : ""} revise = existing pending proposal; list/inspect discover pending proposals (not filesystem search).${supportsCompletion ? " complete = durably finish this review after all proposal work." : ""} Nothing writes a live skill directly; lifecycle actions are unavailable.` : "create = new skill; update = existing live skill; revise = existing pending proposal; list/inspect discover pending proposals (not filesystem search); evaluate runs plugin evaluators for the exact draft; apply/reject/quarantine are explicit lifecycle actions.", }), proposal_id: Type.Optional( @@ -436,48 +437,47 @@ export function createSkillWorkshopTool(options: SkillWorkshopToolOptions): AnyA const goal = readStringParam(params, "goal"); const evidence = readStringParam(params, "evidence"); - let resolvedPatchSkillKey: string | undefined; - if (action === "patch") { - if (options.updateProposals !== true) { - throw new ToolInputError("this Skill Workshop session cannot patch live skills"); - } - // Pre-validate before spending the mutation budget so a mismatched quote or a - // stale read costs a retry, not the whole review. The read receipt proves the - // reviewer itself saw the current body — a quoted span alone could have been - // injected through the untrusted trajectory. The service still composes - // authoritatively from its own hash-binding read. + if (action === "patch" && options.updateProposals !== true) { + throw new ToolInputError("this Skill Workshop session cannot patch live skills"); + } + let reviewerUpdateContentHash: string | undefined; + if (options.updateProposals === true && (action === "patch" || action === "update")) { + // The reviewer must see the entire current skill before either a targeted + // patch or a rewrite. The service binds the resulting proposal to that read. const target = await readWritableWorkspaceSkill( options.workspaceDir, readStringParam(params, "skill_name", { required: true, label: "skill_name" }), { config: options.config, agentId: options.agentId }, ); - resolvedPatchSkillKey = target.skillKey; const readHash = options.proposalMutationBudget?.readSkillHashes?.get(target.skillKey); if (!readHash) { throw new ToolInputError( target.content.length > REVIEWER_SKILL_READ_MAX_CHARS - ? `skill "${target.skillKey}" exceeds the reviewer read budget and cannot be patched autonomously; draft a full-body update instead (it stays pending for the operator)` - : `read the live skill first: call action=read with skill_name "${target.skillKey}", then quote its current text in the patch`, + ? `skill "${target.skillKey}" exceeds the reviewer read budget and cannot be updated autonomously` + : `read the live skill first: call action=read with skill_name "${target.skillKey}", then ${action === "patch" ? "quote its current text in the patch" : "rewrite it from the returned content"}`, ); } if (readHash !== sha256Hex(target.content)) { options.proposalMutationBudget?.readSkillHashes?.delete(target.skillKey); throw new ToolInputError( - `skill "${target.skillKey}" changed since it was read: call action=read again and redraft the patch from the current content`, + `skill "${target.skillKey}" changed since it was read: call action=read again and redraft the ${action} from the current content`, ); } - try { - composeSkillBodyPatch(stripProposalFrontmatterForSkill(target.content), { - oldString: - readStringParam(params, "old_string", { label: "old_string", trim: false }) ?? "", - newString: readStringParam(params, "new_string", { - required: true, - label: "new_string", - trim: false, - }), - }); - } catch (error) { - throw new ToolInputError(error instanceof Error ? error.message : String(error)); + reviewerUpdateContentHash = readHash; + if (action === "patch") { + try { + composeSkillBodyPatch(stripProposalFrontmatterForSkill(target.content), { + oldString: + readStringParam(params, "old_string", { label: "old_string", trim: false }) ?? "", + newString: readStringParam(params, "new_string", { + required: true, + label: "new_string", + trim: false, + }), + }); + } catch (error) { + throw new ToolInputError(error instanceof Error ? error.message : String(error)); + } } } @@ -530,6 +530,7 @@ export function createSkillWorkshopTool(options: SkillWorkshopToolOptions): AnyA required: true, label: "skill_name", }), + expectedCurrentContentHash: reviewerUpdateContentHash, description: readStringParam(params, "description"), content: requireProposalContent(proposalContent), supportFiles, @@ -553,9 +554,7 @@ export function createSkillWorkshopTool(options: SkillWorkshopToolOptions): AnyA required: true, label: "skill_name", }), - expectedCurrentContentHash: options.proposalMutationBudget?.readSkillHashes?.get( - resolvedPatchSkillKey ?? "", - ), + expectedCurrentContentHash: reviewerUpdateContentHash, composePatch: { oldString: readStringParam(params, "old_string", { label: "old_string", trim: false }) ?? "", @@ -609,12 +608,6 @@ export function createSkillWorkshopTool(options: SkillWorkshopToolOptions): AnyA options.proposalMutationBudget.mutatedProposalIds ?? new Set(); mutatedProposalIds.add(proposal.record.id); options.proposalMutationBudget.mutatedProposalIds = mutatedProposalIds; - if (action === "patch") { - const patchProposalIds = - options.proposalMutationBudget.patchProposalIds ?? new Set(); - patchProposalIds.add(proposal.record.id); - options.proposalMutationBudget.patchProposalIds = patchProposalIds; - } options.proposalMutationBudget.completed = mutatedProposalIds.size; options.proposalMutationBudget.successfulMutations = (options.proposalMutationBudget.successfulMutations ?? 0) + 1; @@ -628,10 +621,9 @@ export function createSkillWorkshopTool(options: SkillWorkshopToolOptions): AnyA return proposalResult(proposal, { contentText }); } catch (error) { if (reservesMutation && options.proposalMutationBudget) { - // A service-side patch composition failure means the target changed in the - // instant between prevalidation and the service read — not a model error. - // Refund so the reviewer can re-read and retry within its budget. - if (action === "patch" && error instanceof Error && error.message.startsWith("Patch ")) { + // A concurrent live edit is not a reviewer mutation. Preserve the budget + // so the reviewer can re-read the new body and redraft either update form. + if (error instanceof SkillProposalStaleTargetError) { options.proposalMutationBudget.remaining += 1; } options.proposalMutationBudget.failedMutations = diff --git a/src/skills/runtime/run-usage.ts b/src/skills/runtime/run-usage.ts new file mode 100644 index 000000000000..6c99a057984f --- /dev/null +++ b/src/skills/runtime/run-usage.ts @@ -0,0 +1,35 @@ +import { pruneMapToMaxSize } from "../../infra/map-size.js"; +import type { SkillTelemetrySource } from "../types.js"; + +const MAX_TRACKED_SKILL_USAGE_RUNS = 1024; + +export type RunSkillUsage = Readonly<{ + name: string; + source: SkillTelemetrySource; + activation: "command" | "read"; +}>; + +const skillUsageByRun = new Map>(); + +/** Records the skills the foreground run demonstrably invoked or read. */ +export function recordRunSkillUsage(params: RunSkillUsage & { runId?: string }): void { + const runId = params.runId; + if (!runId) { + return; + } + const usage = skillUsageByRun.get(runId) ?? new Map(); + const record = { name: params.name, source: params.source, activation: params.activation }; + usage.set(`${record.source}\u0000${record.name}\u0000${record.activation}`, record); + skillUsageByRun.set(runId, usage); + pruneMapToMaxSize(skillUsageByRun, MAX_TRACKED_SKILL_USAGE_RUNS); +} + +/** Transfers one completed run's usage receipt to its terminal side effects. */ +export function consumeRunSkillUsage(runId: string | undefined): RunSkillUsage[] { + if (!runId) { + return []; + } + const usage = skillUsageByRun.get(runId); + skillUsageByRun.delete(runId); + return usage ? [...usage.values()] : []; +} diff --git a/src/skills/workshop/experience-review-auto-apply.test.ts b/src/skills/workshop/experience-review-auto-apply.test.ts index 24e31afe4f04..6d39d444c4a6 100644 --- a/src/skills/workshop/experience-review-auto-apply.test.ts +++ b/src/skills/workshop/experience-review-auto-apply.test.ts @@ -91,7 +91,7 @@ describe("experience review auto apply", () => { ); }); - it("leaves reviewer update proposals pending instead of auto-applying them", async () => { + it("auto-applies reviewer full-body updates after an authoritative read", async () => { const workspaceDir = await tempDirs.make("openclaw-experience-auto-apply-update-"); const seedTool = createSkillWorkshopTool({ workspaceDir, @@ -121,6 +121,10 @@ describe("experience review auto apply", () => { autonomousCapture: params.skillWorkshopAutonomousCapture, proposalMutationBudget: params.skillWorkshopProposalMutationBudget, }); + await tool.execute("review-read", { + action: "read", + skill_name: "deployment-preflight", + }); await tool.execute("review-update", { action: "update", skill_name: "deployment-preflight", @@ -150,11 +154,11 @@ describe("experience review auto apply", () => { const updateEntry = manifest.proposals.find((entry) => entry.kind === "update"); expect(updateEntry).toMatchObject({ skillKey: "deployment-preflight", - status: "pending", + status: "applied", }); await expect( fs.readFile(`${workspaceDir}/skills/deployment-preflight/SKILL.md`, "utf8"), - ).resolves.toContain("Operator-authored preflight steps."); + ).resolves.toContain("Reviewer-rewritten steps."); }); it("auto-applies reviewer patch proposals composed from the live body", async () => { diff --git a/src/skills/workshop/experience-review-prompt.ts b/src/skills/workshop/experience-review-prompt.ts index 9d6773e00020..7ba09276116d 100644 --- a/src/skills/workshop/experience-review-prompt.ts +++ b/src/skills/workshop/experience-review-prompt.ts @@ -1,16 +1,19 @@ import { isRecord } from "@openclaw/normalization-core/record-coerce"; import { sliceUtf16Safe, truncateUtf16Safe } from "@openclaw/normalization-core/utf16-slice"; +import type { RunSkillUsage } from "../runtime/run-usage.js"; import { SKILL_AUTHORING_STANDARDS_PROMPT } from "./skill-authoring-standards.js"; const EXPERIENCE_REVIEW_MAX_TRANSCRIPT_CHARS = 60_000; const EXPERIENCE_REVIEW_MAX_SKILL_ENTRIES = 50; const EXPERIENCE_REVIEW_MAX_SKILL_LINE_CHARS = 200; +const EXPERIENCE_REVIEW_MAX_USED_SKILLS_CHARS = 2_000; type ExperienceReviewPromptCandidate = { ctx: { runId?: string }; transcript: string; modelIterations: number; turnAborted?: boolean; + usedSkills?: readonly RunSkillUsage[]; existingSkills?: readonly { name: string; description?: string }[]; }; @@ -110,6 +113,52 @@ function renderExistingSkillsSection( ]; } +function compareRunSkillUsage(left: RunSkillUsage, right: RunSkillUsage): number { + for (const field of ["name", "source", "activation"] as const) { + if (left[field] !== right[field]) { + return left[field] < right[field] ? -1 : 1; + } + } + return 0; +} + +function renderUsedSkillsSection( + usedSkills: ExperienceReviewPromptCandidate["usedSkills"], +): string[] { + if (!usedSkills?.length) { + return []; + } + const shown = usedSkills + .toSorted(compareRunSkillUsage) + .slice(0, EXPERIENCE_REVIEW_MAX_SKILL_ENTRIES); + const header = "Skills actually used in this trajectory (authoritative runtime receipt):"; + const preference = + "Prefer improving a used writable workspace skill when it governs the learning."; + const reservedOmission = `(+${usedSkills.length} more used skills omitted)`; + const entries: string[] = []; + for (const skill of shown) { + const line = truncateUtf16Safe( + `- ${skill.name} (${skill.source}, ${skill.activation})`, + EXPERIENCE_REVIEW_MAX_SKILL_LINE_CHARS, + ); + if ( + ["", header, ...entries, line, reservedOmission, preference].join("\n").length > + EXPERIENCE_REVIEW_MAX_USED_SKILLS_CHARS + ) { + break; + } + entries.push(line); + } + const omitted = usedSkills.length - entries.length; + return [ + "", + header, + ...entries, + ...(omitted > 0 ? [`(+${omitted} more used skills omitted)`] : []), + preference, + ]; +} + export function buildSkillExperienceReviewPrompt( candidate: ExperienceReviewPromptCandidate, ): string { @@ -127,7 +176,7 @@ export function buildSkillExperienceReviewPrompt( "", SKILL_AUTHORING_STANDARDS_PROMPT, "", - "Choose the smallest mutation, in order: (1) revise a pending proposal on the same topic — use list/inspect to check; (2) patch the existing workspace skill that governs this work — read it first, then quote the exact text to change in old_string with your replacement in new_string, or use an empty old_string to append a new section; place the learning where it belongs and match the skill's style; (3) update with a full replacement body only when the whole skill needs restructuring — those stay pending for the operator; (4) create one new class-level skill only when no existing skill covers this class of work. Make at most one create/patch/update/revise call. Every mutation is a pending proposal; nothing writes a live skill directly, and the tool cannot apply, reject, or quarantine. If nothing genuinely clears the bar, answer NOTHING_TO_LEARN.", + "Choose the smallest mutation, in order: (1) revise a pending proposal on the same topic — use list/inspect to check; (2) patch a used writable workspace skill that governs this work, otherwise the best existing workspace skill — read it first, then quote the exact text to change in old_string with your replacement in new_string, or use an empty old_string to append a new section; place the learning where it belongs and match the skill's style; (3) update with a full replacement body only when the whole skill needs restructuring — read it first and preserve everything still useful; (4) create one new class-level skill only when no existing skill covers this class of work. Make at most one create/patch/update/revise call. Every mutation starts as a pending proposal; nothing writes a live skill during this review, and the configured pipeline decides whether to apply it afterward. If nothing genuinely clears the bar, answer NOTHING_TO_LEARN.", "", candidate.turnAborted === true ? `Interrupted run (stopped before completion): ${candidate.ctx.runId ?? "unknown"}` @@ -137,6 +186,7 @@ export function buildSkillExperienceReviewPrompt( "The trajectory may end mid-task. Only capture procedures that visibly worked before the interruption.", ] : []), + ...renderUsedSkillsSection(candidate.usedSkills), ...renderExistingSkillsSection(candidate.existingSkills), `Model iterations in turn: ${candidate.modelIterations}`, "", diff --git a/src/skills/workshop/experience-review.test.ts b/src/skills/workshop/experience-review.test.ts index 1cb26cf0dfd7..8c77b0a9d9c5 100644 --- a/src/skills/workshop/experience-review.test.ts +++ b/src/skills/workshop/experience-review.test.ts @@ -25,6 +25,9 @@ function completedRun( senderId?: string; senderName?: string; chatType?: "direct" | "group"; + modelProviderId?: string; + authProfileId?: string; + usedSkills?: SkillExperienceReviewParams["usedSkills"]; } = {}, ): SkillExperienceReviewParams { const iterations = options.iterations ?? 10; @@ -55,9 +58,9 @@ function completedRun( ...(options.modelMetadata === false ? {} : { - modelProviderId: "openai", + modelProviderId: options.modelProviderId ?? "openai", modelId: "gpt-test", - authProfileId: "openai:work", + authProfileId: options.authProfileId ?? "openai:work", }), skillWorkshopAvailable: options.skillWorkshopAvailable ?? true, ...(options.modelIterations === undefined @@ -76,6 +79,7 @@ function completedRun( }, }, }, + ...(options.usedSkills ? { usedSkills: options.usedSkills } : {}), }; } @@ -112,6 +116,38 @@ describe("skill experience review scheduler", () => { scheduler.clear(); }); + it("scopes deep direct and group reviews to the current user turn", async () => { + vi.useFakeTimers(); + const runReview = vi.fn().mockResolvedValue(undefined); + const scheduler = createSkillExperienceReviewScheduler({ + isSystemActive: () => false, + runReview, + }); + const direct = completedRun({ sessionKey: "agent:main:direct" }); + direct.event.messages.unshift( + { role: "user", content: "Earlier correction from this direct session." }, + { role: "assistant", content: "Earlier response." }, + ); + scheduler.schedule(direct); + await vi.advanceTimersByTimeAsync(30_000); + expect(runReview.mock.calls[0]?.[0].transcript).not.toContain( + "Earlier correction from this direct session.", + ); + + runReview.mockClear(); + const group = completedRun({ sessionKey: "agent:main:group", chatType: "group" }); + group.event.messages.unshift( + { role: "user", content: "Earlier message from another group participant." }, + { role: "assistant", content: "Earlier group response." }, + ); + scheduler.schedule(group); + await vi.advanceTimersByTimeAsync(30_000); + expect(runReview.mock.calls[0]?.[0].transcript).not.toContain( + "Earlier message from another group participant.", + ); + scheduler.clear(); + }); + it("uses exact harness iterations for a Codex-style projected trajectory", async () => { vi.useFakeTimers(); const runReview = vi.fn().mockResolvedValue(undefined); @@ -146,6 +182,89 @@ describe("skill experience review scheduler", () => { scheduler.clear(); }); + it("carries skills actually used across accumulated shallow turns", async () => { + vi.useFakeTimers(); + const runReview = vi.fn().mockResolvedValue(undefined); + const scheduler = createSkillExperienceReviewScheduler({ + isSystemActive: () => false, + runReview, + }); + + scheduler.schedule( + completedRun({ + modelIterations: 5, + runId: "run-a", + usedSkills: [{ name: "release-runbook", source: "workspace", activation: "read" }], + }), + ); + scheduler.schedule( + completedRun({ + modelIterations: 5, + runId: "run-b", + usedSkills: [{ name: "deploy-check", source: "workspace", activation: "command" }], + }), + ); + await vi.advanceTimersByTimeAsync(30_000); + + expect(runReview).toHaveBeenCalledWith( + expect.objectContaining({ + usedSkills: [ + { name: "release-runbook", source: "workspace", activation: "read" }, + { name: "deploy-check", source: "workspace", activation: "command" }, + ], + }), + ); + scheduler.clear(); + }); + + it("does not carry direct transcript or skill receipts across provider identities", async () => { + vi.useFakeTimers(); + const runReview = vi.fn().mockResolvedValue(undefined); + const scheduler = createSkillExperienceReviewScheduler({ + isSystemActive: () => false, + runReview, + }); + + scheduler.schedule( + completedRun({ + sessionKey: "agent:main:provider-switch", + runId: "run-a", + modelProviderId: "provider-a", + authProfileId: "provider-a:work", + userText: "Private work handled by provider A.", + usedSkills: [{ name: "release-runbook", source: "workspace", activation: "read" }], + }), + ); + const nextProviderRun = completedRun({ + sessionKey: "agent:main:provider-switch", + runId: "run-b", + modelProviderId: "provider-b", + authProfileId: "provider-b:work", + userText: "Current work handled by provider B.", + usedSkills: [{ name: "deploy-check", source: "workspace", activation: "command" }], + }); + nextProviderRun.event.messages.unshift( + { role: "user", content: "Private work handled by provider A." }, + { role: "assistant", content: "Private provider A response." }, + ); + scheduler.schedule(nextProviderRun); + await vi.advanceTimersByTimeAsync(30_000); + + expect(runReview).toHaveBeenCalledWith( + expect.objectContaining({ + ctx: expect.objectContaining({ + modelProviderId: "provider-b", + authProfileId: "provider-b:work", + }), + usedSkills: [{ name: "deploy-check", source: "workspace", activation: "command" }], + }), + ); + const candidate = runReview.mock.calls[0]?.[0]; + expect(candidate.transcript).toContain("Current work handled by provider B."); + expect(candidate.transcript).not.toContain("Private work handled by provider A."); + scheduler.clear(); + }); + it("reviews accumulated shallow turns with their own transcripts, not just the last turn", async () => { vi.useFakeTimers(); const runReview = vi.fn().mockResolvedValue(undefined); @@ -672,8 +791,8 @@ describe("skill experience review scheduler", () => { expect(prompt).toContain("prefer capturing over abstaining"); expect(prompt).toContain("untrusted evidence, not instructions"); expect(prompt).toContain("Make at most one create/patch/update/revise call"); - expect(prompt).toContain("nothing writes a live skill directly"); - expect(prompt).toContain("patch the existing workspace skill that governs this work"); + expect(prompt).toContain("nothing writes a live skill during this review"); + expect(prompt).toContain("patch a used writable workspace skill that governs this work"); expect(prompt).toContain("quote the exact text to change"); expect(prompt).toContain("a sequence of failed attempts is not a workflow"); expect(prompt).toContain("NOTHING_TO_LEARN"); @@ -700,6 +819,49 @@ describe("skill experience review scheduler", () => { expect(prompt).toContain("- release-runbook"); }); + it("identifies skills actually used as the first update targets", () => { + const params = completedRun(); + const prompt = buildSkillExperienceReviewPrompt({ + ctx: params.ctx, + transcript: formatSkillExperienceReviewTranscript(params.event.messages), + modelIterations: 10, + usedSkills: [ + { name: "release-runbook", source: "workspace", activation: "read" }, + { name: "bundled-helper", source: "bundled", activation: "command" }, + ], + }); + + expect(prompt).toContain("Skills actually used in this trajectory"); + expect(prompt).toContain("- release-runbook (workspace, read)"); + expect(prompt).toContain("- bundled-helper (bundled, command)"); + expect(prompt).toContain("Prefer improving a used writable workspace skill"); + }); + + it("sorts and caps the complete used-skill receipt", () => { + const params = completedRun(); + const usedSkills = Array.from({ length: 120 }, (_, index) => ({ + name: `skill-${String(index).padStart(3, "0")}-${"x".repeat(180)}`, + source: index % 2 === 0 ? ("workspace" as const) : ("bundled" as const), + activation: index % 3 === 0 ? ("command" as const) : ("read" as const), + })); + const build = (skills: typeof usedSkills) => + buildSkillExperienceReviewPrompt({ + ctx: params.ctx, + transcript: formatSkillExperienceReviewTranscript(params.event.messages), + modelIterations: 10, + usedSkills: skills, + }); + const prompt = build(usedSkills.toReversed()); + + expect(prompt).toBe(build(usedSkills)); + const receiptStart = prompt.indexOf("Skills actually used in this trajectory"); + const receiptEnd = prompt.indexOf("\nModel iterations in turn:", receiptStart); + const receipt = prompt.slice(receiptStart, receiptEnd); + expect(receipt.length).toBeLessThanOrEqual(2_000); + expect(receipt).toContain("- skill-000-"); + expect(receipt).toContain("more used skills omitted"); + }); + it("caps the existing-skill list injected into the review prompt", () => { const params = completedRun(); const prompt = buildSkillExperienceReviewPrompt({ diff --git a/src/skills/workshop/experience-review.ts b/src/skills/workshop/experience-review.ts index 42ccd7d64b57..18111d93c580 100644 --- a/src/skills/workshop/experience-review.ts +++ b/src/skills/workshop/experience-review.ts @@ -5,6 +5,7 @@ import type { OpenClawConfig } from "../../config/types.openclaw.js"; import { createSubsystemLogger } from "../../logging/subsystem.js"; import { runWithGatewayIndependentRootWorkAdmission } from "../../process/gateway-work-admission.js"; import { CommandLane } from "../../process/lanes.js"; +import type { RunSkillUsage } from "../runtime/run-usage.js"; import { autoApplySkillProposal } from "./auto-apply.js"; import { resolveSkillWorkshopConfig } from "./config.js"; import { @@ -71,6 +72,7 @@ type ExperienceReviewAgentContext = { export type SkillExperienceReviewParams = { event: ExperienceReviewAgentEndEvent; ctx: ExperienceReviewAgentContext; + usedSkills?: readonly RunSkillUsage[]; config?: OpenClawConfig; }; @@ -79,6 +81,7 @@ export type ExperienceReviewCandidate = { config?: OpenClawConfig; transcript: string; modelIterations: number; + usedSkills?: readonly RunSkillUsage[]; turnAborted?: boolean; }; @@ -104,6 +107,18 @@ type PendingExperienceReview = { timer?: ExperienceReviewTimer; }; +function mergeRunSkillUsage( + ...groups: Array +): RunSkillUsage[] { + const merged = new Map(); + for (const group of groups) { + for (const usage of group ?? []) { + merged.set(`${usage.source}\u0000${usage.name}\u0000${usage.activation}`, usage); + } + } + return [...merged.values()]; +} + function isAuthProfileMigrationRequiredError( error: unknown, ): error is { code: "AUTH_PROFILE_MIGRATION_REQUIRED" } { @@ -216,6 +231,7 @@ export function createSkillExperienceReviewScheduler(deps: ExperienceReviewSched senderScope: string; iterations: number; messages: unknown[]; + usedSkills: RunSkillUsage[]; aborted: boolean; lastRunId?: string; } @@ -347,6 +363,12 @@ export function createSkillExperienceReviewScheduler(deps: ExperienceReviewSched let reviewIterations = modelIterations; let reviewMessages = turnMessages; let reviewAborted = !params.event.success; + let reviewUsedSkills = mergeRunSkillUsage( + existing && existing.candidate.ctx.runId === params.ctx.runId + ? existing.candidate.usedSkills + : undefined, + params.usedSkills, + ); if (modelIterations >= EXPERIENCE_REVIEW_MIN_MODEL_ITERATIONS) { shallowBySession.delete(sessionKey); } else { @@ -393,7 +415,13 @@ export function createSkillExperienceReviewScheduler(deps: ExperienceReviewSched shallowBySession.delete(oldestKey); } } - accumulator = { senderScope, iterations: 0, messages: [], aborted: false }; + accumulator = { + senderScope, + iterations: 0, + messages: [], + usedSkills: [], + aborted: false, + }; shallowBySession.set(sessionKey, accumulator); } const runId = params.ctx.runId?.trim(); @@ -405,6 +433,7 @@ export function createSkillExperienceReviewScheduler(deps: ExperienceReviewSched accumulator.lastRunId = runId; accumulator.iterations += modelIterations; accumulator.aborted = accumulator.aborted || !params.event.success; + accumulator.usedSkills = mergeRunSkillUsage(accumulator.usedSkills, params.usedSkills); accumulator.messages = [...accumulator.messages, ...turnMessages].slice( -EXPERIENCE_REVIEW_MAX_SHALLOW_MESSAGES, ); @@ -418,6 +447,7 @@ export function createSkillExperienceReviewScheduler(deps: ExperienceReviewSched reviewIterations = accumulator.iterations; reviewMessages = accumulator.messages; reviewAborted = accumulator.aborted; + reviewUsedSkills = accumulator.usedSkills; } { if (!existing && pendingBySession.size >= EXPERIENCE_REVIEW_MAX_PENDING) { @@ -462,6 +492,7 @@ export function createSkillExperienceReviewScheduler(deps: ExperienceReviewSched ...(params.config ? { config: params.config } : {}), transcript: formatSkillExperienceReviewTranscript(reviewMessages), modelIterations: reviewIterations, + usedSkills: reviewUsedSkills, turnAborted: reviewAborted, }; const pending = existing ?? { candidate, generation: 0 }; @@ -514,7 +545,6 @@ async function runSkillExperienceReviewInner( const sessionId = randomUUID(); const proposalMutationBudget: SkillWorkshopProposalMutationBudget = { remaining: 1, - patchProposalIds: new Set(), readSkillHashes: new Map(), }; const reviewSessionKey = `agent:${candidate.ctx.agentId ?? "main"}:${EXPERIENCE_REVIEW_SESSION_SEGMENT}:incognito-${sessionId}`; @@ -606,18 +636,6 @@ async function runSkillExperienceReviewInner( ) { continue; } - // Patch proposals auto-apply: the service composed them by replacing only the span - // the reviewer quoted from the live body (or appending), so untouched content - // survives by construction. Full-body update proposals stay pending for review. - if ( - proposal.record.kind === "update" && - proposalMutationBudget.patchProposalIds?.has(proposalId) !== true - ) { - log.info( - `skill experience review left full-body update proposal ${proposalId} pending for operator review`, - ); - continue; - } await autoApplySkillProposal({ workspaceDir, ...(candidate.ctx.agentId ? { agentId: candidate.ctx.agentId } : {}), diff --git a/src/skills/workshop/service.ts b/src/skills/workshop/service.ts index 934014e920bf..3fcb5d015a0c 100644 --- a/src/skills/workshop/service.ts +++ b/src/skills/workshop/service.ts @@ -69,6 +69,8 @@ type SkillWorkshopWorkspaceOptions = { agentId?: string; }; +export class SkillProposalStaleTargetError extends Error {} + function proposalStoreOptions(env?: NodeJS.ProcessEnv) { return env ? { env } : {}; } @@ -269,8 +271,8 @@ export async function proposeUpdateSkill( input.expectedCurrentContentHash !== undefined && sha256Hex(currentContent) !== input.expectedCurrentContentHash ) { - throw new Error( - "Patch target changed since the reviewer's read: read the skill again and redraft the patch.", + throw new SkillProposalStaleTargetError( + "Skill changed since the reviewer's read: read it again and redraft the update.", ); } // Composition uses the same read that currentContentHash binds the proposal to, so a diff --git a/src/skills/workshop/types.ts b/src/skills/workshop/types.ts index 8070632621c7..67b8d3061286 100644 --- a/src/skills/workshop/types.ts +++ b/src/skills/workshop/types.ts @@ -73,9 +73,7 @@ export type SkillWorkshopProposalMutationBudget = { failedMutations?: number; /** Run-local identity set used to keep idea counts distinct. */ mutatedProposalIds?: Set; - /** Proposals composed mechanically by patching the live body with a reviewer edit. */ - patchProposalIds?: Set; - /** Content hash per live skill read this run; patches require a matching receipt. */ + /** Content hash per live skill read this run; autonomous updates require a matching receipt. */ readSkillHashes?: Map; };