Skip to content

Commit e3bd9da

Browse files
steipetesteipete
committed
fix(skills): make Skill Workshop lifecycle approvals decidable and non-wedging (#91266, #94249, #93173)
Approval wait now fits inside the Codex dynamic-tool watchdog (70s +10s gateway grace under the 90s kill), approval cards carry proposal id, skill name, description, file count, and body size (spoof-safe rendering), and timeouts return a structured pending-not-failed outcome instead of a bare error. Expired requests cannot execute late; no auto-apply; generic plugin approvals unchanged.
1 parent 2db5bd3 commit e3bd9da

17 files changed

Lines changed: 357 additions & 15 deletions

‎docs/tools/skill-workshop.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,14 @@ Agent-initiated `apply`, `reject`, and `quarantine` show an approval prompt by
7474
default. Set `skills.workshop.approvalPolicy` to `"auto"` to skip it in
7575
trusted environments.
7676

77+
The prompt identifies the proposal id and target skill, and shows the proposal
78+
description, support-file count, and body size. Approval requests are bounded
79+
to finish before the agent tool watchdog. If no decision arrives before the
80+
prompt expires, the lifecycle action does not run: the proposal stays pending
81+
and unchanged. Decide later in the Skill Workshop UI or run
82+
`openclaw skills workshop apply|reject|quarantine <proposal-id>`. Agents should
83+
not retry an expired lifecycle action in a loop.
84+
7785
## CLI
7886

7987
```bash

‎extensions/codex/src/app-server/approval-bridge.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -669,6 +669,7 @@ async function runOpenClawToolPolicyForApprovalRequest(params: {
669669
...(params.paramsForRun.agentId ? { agentId: params.paramsForRun.agentId } : {}),
670670
...(params.paramsForRun.config ? { config: params.paramsForRun.config } : {}),
671671
...(cwd ? { cwd } : {}),
672+
workspaceDir: params.paramsForRun.workspaceDir,
672673
...(params.paramsForRun.sessionKey ? { sessionKey: params.paramsForRun.sessionKey } : {}),
673674
...(params.paramsForRun.sessionId ? { sessionId: params.paramsForRun.sessionId } : {}),
674675
...(params.paramsForRun.runId ? { runId: params.paramsForRun.runId } : {}),

‎extensions/codex/src/app-server/dynamic-tools.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ import type {
5858
type CodexDynamicToolHookContext = {
5959
agentId?: string;
6060
config?: EmbeddedRunAttemptParams["config"];
61+
workspaceDir?: string;
6162
sessionId?: string;
6263
sessionKey?: string;
6364
runId?: string;

‎extensions/codex/src/app-server/run-attempt.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -878,6 +878,7 @@ export async function runCodexAppServerAttempt(
878878
hookContext: {
879879
agentId: sessionAgentId,
880880
config: params.config,
881+
workspaceDir: effectiveWorkspace,
881882
sessionId: params.sessionId,
882883
sessionKey: sandboxSessionKey,
883884
runId: params.runId,

‎packages/gateway-protocol/src/plugin-approvals-validators.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import { describe, expect, it } from "vitest";
2+
import { validatePluginApprovalRequestParams } from "./index.js";
3+
4+
describe("plugin approval protocol validators", () => {
5+
it("accepts enriched approval descriptions up to 512 characters", () => {
6+
const request = {
7+
title: "Apply workspace skill proposal",
8+
description: "d".repeat(512),
9+
};
10+
11+
expect(validatePluginApprovalRequestParams(request)).toBe(true);
12+
expect(validatePluginApprovalRequestParams({ ...request, description: "d".repeat(513) })).toBe(
13+
false,
14+
);
15+
});
16+
});

‎packages/gateway-protocol/src/schema/plugin-approvals.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { NonEmptyString } from "./primitives.js";
1111
*/
1212
const MAX_PLUGIN_APPROVAL_TIMEOUT_MS = 600_000;
1313
const PLUGIN_APPROVAL_TITLE_MAX_LENGTH = 80;
14-
const PLUGIN_APPROVAL_DESCRIPTION_MAX_LENGTH = 256;
14+
const PLUGIN_APPROVAL_DESCRIPTION_MAX_LENGTH = 512;
1515

1616
/** Approval request raised by a plugin before a sensitive tool action proceeds. */
1717
export const PluginApprovalRequestParamsSchema = Type.Object(

‎src/agents/agent-tools.before-tool-call.e2e.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1514,6 +1514,29 @@ describe("before_tool_call requireApproval handling", () => {
15141514
expect(result).toHaveProperty("reason", "Denied by user");
15151515
});
15161516

1517+
it("keeps the generic plugin approval timeout reason unchanged", async () => {
1518+
hookRunner.runBeforeToolCall.mockResolvedValue({
1519+
requireApproval: {
1520+
title: "Timeout test",
1521+
description: "Will time out",
1522+
},
1523+
});
1524+
mockCallGateway.mockResolvedValueOnce({ id: "server-id-timeout", status: "accepted" });
1525+
mockCallGateway.mockResolvedValueOnce({ id: "server-id-timeout", decision: null });
1526+
1527+
const result = await runBeforeToolCallHook({
1528+
toolName: "bash",
1529+
params: {},
1530+
ctx: { agentId: "main", sessionKey: "main" },
1531+
});
1532+
1533+
expect(result).toMatchObject({
1534+
blocked: true,
1535+
kind: "failure",
1536+
reason: "Approval timed out",
1537+
});
1538+
});
1539+
15171540
it("blocks turn-source plugin approval timeouts with setup guidance", async () => {
15181541
registerTelegramPluginApprovalSetup();
15191542
hookRunner.runBeforeToolCall.mockResolvedValue({

‎src/agents/agent-tools.before-tool-call.embedded-mode.test.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,8 @@ describe("runBeforeToolCallHook — embedded mode approvals", () => {
436436
);
437437
expect(approvalCall.request.severity).toBe("warning");
438438
expect(approvalCall.request.allowedDecisions).toEqual(["allow-once", "deny"]);
439+
expect(approvalCall.request.timeoutMs).toBe(70_000);
440+
expect(approvalCall.timeoutParams.timeoutMs).toBe(80_000);
439441
expect(approvalCall.request.toolName).toBe("skill_workshop");
440442
expect(approvalCall.request.toolCallId).toBe("call-skill-apply");
441443
expect(runBeforeToolCallMock).toHaveBeenCalledTimes(1);
@@ -481,6 +483,32 @@ describe("runBeforeToolCallHook — embedded mode approvals", () => {
481483
}
482484
});
483485

486+
it("returns an actionable pending outcome when skill_workshop approval expires", async () => {
487+
mockCallGatewayTool.mockResolvedValueOnce({
488+
id: "skill-workshop-timeout",
489+
status: "accepted",
490+
});
491+
mockCallGatewayTool.mockResolvedValueOnce({
492+
id: "skill-workshop-timeout",
493+
decision: null,
494+
});
495+
496+
const result = await runBeforeToolCallHook({
497+
toolName: "skill_workshop",
498+
params: { action: "apply", proposal_id: "weather-20260530-a1b2c3d4e5" },
499+
toolCallId: "call-skill-timeout",
500+
ctx: { agentId: "main", sessionKey: "main" },
501+
});
502+
503+
expect(result).toMatchObject({
504+
blocked: true,
505+
kind: "veto",
506+
deniedReason: "plugin-approval",
507+
reason:
508+
"The Skill Workshop approval request expired without a decision. This lifecycle call left the proposal unchanged and pending; check its current status in case another operator acted on it. Decide in the Skill Workshop UI or run `openclaw skills workshop apply|reject|quarantine <id>`. Do not retry this tool call in a loop.",
509+
});
510+
});
511+
484512
it("runs trusted policies before skill_workshop lifecycle approval", async () => {
485513
const registry = createEmptyPluginRegistry();
486514
registry.trustedToolPolicies = [

‎src/agents/agent-tools.before-tool-call.ts‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -761,9 +761,9 @@ async function requestPluginToolApproval(params: {
761761
}
762762
return {
763763
blocked: true,
764-
kind: "failure",
764+
kind: approval.timeoutReason ? "veto" : "failure",
765765
deniedReason: "plugin-approval",
766-
reason: "Approval timed out",
766+
reason: approval.timeoutReason ?? "Approval timed out",
767767
params: params.baseParams,
768768
};
769769
}
@@ -901,16 +901,17 @@ async function requestPluginToolApproval(params: {
901901
approvalResolution: resolution,
902902
};
903903
}
904+
const fallbackTimeoutReason = approval.timeoutReason ?? "Approval timed out";
904905
const timeoutReason =
905906
requestResult?.deliveryRoute === "turn-source"
906907
? buildPluginApprovalFailureReason({
907-
fallbackReason: "Approval timed out",
908+
fallbackReason: fallbackTimeoutReason,
908909
ctx: params.ctx,
909910
})
910-
: "Approval timed out";
911+
: fallbackTimeoutReason;
911912
return {
912913
blocked: true,
913-
kind: "failure",
914+
kind: approval.timeoutReason ? "veto" : "failure",
914915
deniedReason: "plugin-approval",
915916
reason: timeoutReason,
916917
params: params.baseParams,
@@ -1033,10 +1034,11 @@ async function resolveSkillWorkshopApprovalForFinalParams(params: {
10331034
ctx?: HookContext;
10341035
signal?: AbortSignal;
10351036
}): Promise<HookOutcome | undefined> {
1036-
const result = resolveSkillWorkshopToolApproval({
1037+
const result = await resolveSkillWorkshopToolApproval({
10371038
toolName: params.toolName,
10381039
toolParams: isPlainObject(params.params) ? params.params : {},
10391040
...(params.ctx?.config ? { config: params.ctx.config } : {}),
1041+
...(params.ctx?.workspaceDir ? { workspaceDir: params.ctx.workspaceDir } : {}),
10401042
});
10411043
return await resolveBeforeToolCallApprovalOutcome({
10421044
result,
@@ -1266,10 +1268,11 @@ export async function runBeforeToolCallHook(args: {
12661268
const policyRegistry = getGlobalHookRunnerRegistry() ?? undefined;
12671269
const shouldRunTrustedPolicies = hasTrustedToolPolicies(policyRegistry);
12681270
const normalizedParams = isPlainObject(params) ? params : {};
1269-
const initialCorePolicyResult = resolveSkillWorkshopToolApproval({
1271+
const initialCorePolicyResult = await resolveSkillWorkshopToolApproval({
12701272
toolName,
12711273
toolParams: normalizedParams,
12721274
...(args.ctx?.config ? { config: args.ctx.config } : {}),
1275+
...(args.ctx?.workspaceDir ? { workspaceDir: args.ctx.workspaceDir } : {}),
12731276
});
12741277
if (!initialCorePolicyResult && !shouldRunTrustedPolicies && !hasBeforeToolCallHooks) {
12751278
return { blocked: false, params };

‎src/agents/harness/native-hook-relay.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1401,7 +1401,9 @@ async function runNativeHookRelayPreToolUse(params: {
14011401
...(params.registration.config ? { config: params.registration.config } : {}),
14021402
runId: params.registration.runId,
14031403
...(params.registration.channelId ? { channelId: params.registration.channelId } : {}),
1404-
...(params.invocation.cwd ? { cwd: params.invocation.cwd } : {}),
1404+
...(params.invocation.cwd
1405+
? { cwd: params.invocation.cwd, workspaceDir: params.invocation.cwd }
1406+
: {}),
14051407
},
14061408
});
14071409
if (outcome.blocked) {

0 commit comments

Comments
 (0)