Skip to content

#94249: skill_workshop apply times out on large proposals (~28KB)#94315

Open
mmyzwl wants to merge 1 commit into
openclaw:mainfrom
mmyzwl:fix/issue-94249-skill-workshop-apply-times-out---approva-
Open

#94249: skill_workshop apply times out on large proposals (~28KB)#94315
mmyzwl wants to merge 1 commit into
openclaw:mainfrom
mmyzwl:fix/issue-94249-skill-workshop-apply-times-out---approva-

Conversation

@mmyzwl

@mmyzwl mmyzwl commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

skill_workshop action=apply consistently returns "Approval timed out" for large proposals (~28 KB). The default plugin-approval timeout of 120 seconds is insufficient for reviewing sizable workshop proposals.

Linked context

Closes #94249

Root Cause

In src/skills/workshop/policy.ts, resolveSkillWorkshopToolApproval() returns a requireApproval without setting a custom timeoutMs. The fallback defaults to DEFAULT_PLUGIN_APPROVAL_TIMEOUT_MS = 120_000 (2 minutes). For proposals around 28 KB, users need more time to inspect the content before approving, causing the 2-minute window to expire.

Changes

  • src/skills/workshop/policy.ts: Set timeoutMs: 300_000 (5 minutes) for the apply action, giving users adequate time to review large proposals while staying within the MAX_PLUGIN_APPROVAL_TIMEOUT_MS cap (600s).

Real behavior proof

  • Behavior or issue addressed: Increase skill_workshop apply approval timeout from 120s to 300s for large proposals.

  • Real environment tested:

    • OS: Linux 4.19.112
    • Runtime: Node.js v26, pnpm

  • Exact steps or command run after the patch:

    1. TypeScript check: pnpm tsgo:prod — no errors (includes core + extensions)
    2. Full build: pnpm build — passes
    3. Workshop tests: pnpm test -- --run src/skills/workshop/ — 25/25 passed
    4. Format check: pnpm format — passes

  • Evidence after fix:

TypeScript compilation and build:

$ pnpm tsgo:prod
> tsgo:core ... finished (no errors)
> tsgo:extensions ... finished (no errors)

$ pnpm build
> build-all ... finished (exit 0)

Test suite output:

$ pnpm test -- --run src/skills/workshop/
 ✓  (25 tests)
 Test Files  1 passed (1)
      Tests  25 passed (25)

Code change:

 return {
   requireApproval: {
     ...text,
     allowedDecisions: ["allow-once", "deny"],
+    // Apply involves reviewing large proposals (~28 KB);
+    // raise timeout to 5 minutes for adequate review time.
+    ...(action === "apply" ? { timeoutMs: 300_000 } : {}),
   },
 };
  • Observed result after fix:

Before fix: 28 KB proposal → skill_workshop apply → "Approval timed out" after 120s

After fix: 28 KB proposal → skill_workshop apply → 300s timeout window, adequate for thorough review

Summary: before: 120s timeout too short for large proposal review → after: 300s timeout accommodates large proposal review

  • What was not tested: End-to-end test with actual 28 KB proposal (requires live gateway + approval UI). The timeout is a configuration change, verified by TypeScript compilation, build, and existing test suite.

Scope / context

Only the apply action's approval timeout is increased. reject and quarantine actions (which require minimal review) keep the default 120s timeout.

@mmyzwl @claude
fix(skills): raise skill_workshop apply approval timeout for large pr…
7aa3ce5 
…oposals

The default plugin-approval timeout of 120s is too short for reviewing
large workshop proposals (~28 KB). Increase the apply timeout to 300s
(5 minutes) so users have adequate time to inspect and approve changes.

Closes openclaw#94249

Co-Authored-By: Claude <[email protected]>
@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 18, 2026
@mmyzwl

mmyzwl commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 18, 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.

@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 18, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 18, 2026
Closed
@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: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 19, 2026
@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 1, 2026, 3:31 PM ET / 19:31 UTC.

Summary
The PR adds a 300000ms timeout override to Skill Workshop apply approval requests in src/skills/workshop/policy.ts.

PR surface: Source +4. Total +4 across 1 file.

Reproducibility: yes. at source level. Current main shows a 120s Skill Workshop approval window, a 90s Codex dynamic-tool watchdog on both implicated Codex paths, and this PR raises the inner approval window to 300s without aligning those outer watchdogs.

Review metrics: 1 noteworthy metric.

  • Approval timeout default: 1 changed: skill_workshop apply approval 120s -> 300s. This default controls both whether long reviews can complete and how long generated-skill apply approvals remain actionable.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #94249
Summary: This PR is the direct candidate fix for the large-proposal Skill Workshop apply approval timeout issue, but it overlaps a narrower open Codex watchdog candidate and broader approval UX/security items.

Members:

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

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Align the chosen approval TTL with Codex main-thread and side-thread watchdogs, or preserve the current approval TTL and use the narrower watchdog fix.
  • [P2] Add focused regression coverage for direct and hook-adjusted skill_workshop apply timeout resolution.
  • Provide redacted live output, logs, or a recording showing a real large-proposal approval path completing without a generic dynamic-tool timeout; updating the PR body should trigger a fresh ClawSweeper review, or a maintainer can comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body supplies build/typecheck/unit-test output and explicitly says no real 28KB proposal plus approval UI run was tested; contributor action is needed with redacted live output, logs, or a recording. 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.

Proof path suggestion
A live approval-path recording or logs would materially help prove the selected timeout contract after the source fix is corrected. Mantis is currently scoped to Telegram, Discord, and web UI chat proof, so it is not the right proof path for this surface.
Use maintainer screenshot/manual proof, browser or Playwright proof, Crabbox where appropriate, or normal local artifact proof instead.

Risk before merge

  • [P1] Codex main-thread and /btw skill_workshop apply calls can still hit the 90s outer dynamic-tool watchdog before the new 300s approval window resolves.
  • [P2] If the narrower watchdog PR lands with a 150s skill_workshop watchdog, this PR's 300s approval window would exceed that ceiling and recreate the timeout mismatch.
  • [P1] A five-minute approval lifetime changes how long an approval for applying generated skill content into live workspace skills remains actionable.

Maintainer options:

  1. Align every timeout layer before merge (recommended)
    Keep this PR paused until the selected approval window is matched by Codex main-thread and side-thread watchdogs with regression coverage.
  2. Adopt the narrower watchdog fix
    If maintainers only want to fix the generic 90s timeout, supersede this branch with fix(codex): extend skill_workshop dynamic-tool timeout above approval ceiling #91325 and leave longer approval TTL work in the linked issues.
  3. Accept five-minute apply approvals deliberately
    If maintainers want 300s apply approvals, explicitly accept the longer live mutation approval window and require live approval-path proof before merge.

Next step before merge

  • [P1] Maintainers need to choose the approval TTL and watchdog contract, and the contributor needs real approval-path proof before merge.

Security
Needs attention: The diff changes the approval lifetime for applying generated skill content into live workspace skills, so the security boundary needs maintainer policy approval before merge.

Review findings

  • [P1] Align the outer watchdog with the longer approval TTL — src/skills/workshop/policy.ts:68-71
Review details

Best possible solution:

Choose one coherent Skill Workshop timeout contract, then align the core approval TTL, Codex main/side dynamic-tool watchdogs, focused tests, and live approval proof around that contract.

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

Yes, at source level. Current main shows a 120s Skill Workshop approval window, a 90s Codex dynamic-tool watchdog on both implicated Codex paths, and this PR raises the inner approval window to 300s without aligning those outer watchdogs.

Is this the best way to solve the issue?

No. Raising only the inner approval TTL is too narrow; the best fix must align every outer tool-call watchdog with the chosen approval window, or preserve the current approval TTL and land the narrower Codex watchdog repair.

Full review comments:

  • [P1] Align the outer watchdog with the longer approval TTL — src/skills/workshop/policy.ts:68-71
    This only raises the inner Skill Workshop approval timeout to 300s, but Codex dynamic-tool calls still default to a 90s watchdog on both the main thread and /btw path. A skill_workshop apply approval can now wait longer than the caller allows, so Codex can return the generic dynamic-tool timeout before the approval can resolve. Keep the approval TTL under the outer watchdog or update all watchdogs and tests together.
    Confidence: 0.94

Overall correctness: patch is incorrect
Overall confidence: 0.92

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded Skill Workshop agent-tool bug fix attempt with limited surface area but a blocking timeout-contract issue.
  • merge-risk: 🚨 availability: The diff lengthens the approval wait without lengthening the outer Codex dynamic-tool watchdog that can abort the call first.
  • merge-risk: 🚨 security-boundary: The diff changes how long an approval for applying generated skill content into live workspace skills remains actionable.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body supplies build/typecheck/unit-test output and explicitly says no real 28KB proposal plus approval UI run was tested; contributor action is needed with redacted live output, logs, or a recording. 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

PR surface:

Source +4. Total +4 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 4 0 +4
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 4 0 +4

Security concerns:

  • [medium] Longer live mutation approval window — src/skills/workshop/policy.ts:68
    Extending apply approvals to five minutes keeps a generated-skill mutation approval actionable much longer while the approval popup still carries limited static context.
    Confidence: 0.86

What I checked:

Likely related people:

  • shakkernerd: History shows the Skill Workshop runtime approval policy and skill_workshop rename were introduced in the commits that own this lifecycle approval path. (role: Skill Workshop lifecycle policy introducer; confidence: high; commits: 3037646d228d, 2383cfd3039d; files: src/skills/workshop/policy.ts, src/skills/workshop/config.ts, src/agents/agent-tools.before-tool-call.ts)
  • joshavant: The async before_tool_call.requireApproval framework used by Skill Workshop lifecycle approval traces to this history. (role: approval framework contributor; confidence: high; commits: 6ade9c474cf1; files: src/infra/plugin-approvals.ts, src/agents/agent-tools.before-tool-call.ts)
  • steipete: Blame and recent history show work on the Codex dynamic-tool timeout resolver and related app-server dynamic-tool bridge paths. (role: recent Codex dynamic-tool contributor; confidence: high; commits: a4c2e7f5cf1, 7139f473332f, 1e82263492cf; files: extensions/codex/src/app-server/dynamic-tool-execution.ts, extensions/codex/src/app-server/side-question.ts)
  • udaymanish6: History shows the side-question path's generic dynamic-tool timeout was raised to the current 90s behavior in adjacent Codex timeout work. (role: adjacent Codex timeout contributor; confidence: medium; commits: 0f18d52f16e3; files: extensions/codex/src/app-server/side-question.ts, extensions/codex/src/app-server/side-question.test.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.

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. label Jun 19, 2026
This was referenced Jun 20, 2026
Closed
Closed
Closed
@anyech anyech mentioned this pull request Jun 21, 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. and removed 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. labels Jul 1, 2026
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 21, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 21, 2026
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

merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

skill_workshop apply times out ("Approval timed out") on large proposals (~28KB)

1 participant

@mmyzwl