Repository navigation
fix(skills): restore pending Workshop approval buttons - #139248
Merged
Merged
Conversation
Carry the core Workshop policy owner through the existing signed approval transport, so pending lifecycle actions can reach authorized Telegram DMs. Consolidate lifecycle action metadata and verify the signed owner boundary. Fixes #135291. Based on #135406 by @gaoanze888; reported by @khaisis. Production LOC: +27/-43 (-16); tests: +38/-0. Co-authored-by: gaoanze <gaoanze@meituan.com>
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review in progressClawSweeper is reviewing this revision. This supersedes any previous blocked status. |
Contributor
|
Codex review: blocked before merge. Reviewed September 5, 2026, 1:59 PM ET / 17:59 UTC. ClawSweeper reviewWhat this changesRestores pending Skill Workshop approval requests by supplying their signed owner, consolidates lifecycle action descriptions, and adds regression coverage for all four lifecycle actions. Merge readiness⛔ Blocked before merge - 2 items remain Keep open: current main still contains the reported defect, and this focused repair has no actionable correctness findings. The maintainer takeover preserves the earlier contributor’s fix and adds useful signing-boundary coverage. Priority: P1 Review scores
Verification
How this fits togetherSkill Workshop manages proposed changes to an agent’s reusable skills. Its approval policy sends pending lifecycle actions through the Gateway, which delivers decision controls and releases or blocks execution. flowchart TD
A[Agent requests Workshop action] --> B{Approval policy}
B -->|Automatic| F[Workshop action executes]
B -->|Pending| C[Workshop supplies approval owner]
C --> D[Gateway validates signed request]
D --> E[Authorized owner receives decision controls]
E -->|Allow and run remains active| F
E -->|Deny or expire| G[Action remains blocked]
Before merge
Agent review detailsSecurityNone. PR surfaceSource -16, Tests +38. Total +22 across 3 files. View PR surface stats
Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep one producer-owned approval path that preserves pending-mode decisions, private delivery, and live-run checks without changing automatic mode. Do we have a high-confidence way to reproduce the issue? Yes, from source: a pending Workshop lifecycle action omits pluginId on current main, leaving the signed owner absent and triggering the Gateway rejection. This review did not execute the runtime. Is this the best way to solve the issue? Yes. Supplying the owner at the Workshop producer reuses the existing transport and preserves Gateway validation; a Telegram workaround or relaxed owner check would repair the wrong boundary. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 925fb0c3353c. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Contributor
Author
|
Merged via squash.
|
vincentkoc
added a commit
that referenced
this pull request
Sep 5, 2026
* commit '911d7d5b0c107f7274f2415ca9655dcc6105f0e8': (867 commits) fix(skills): deliver pending Workshop approval controls (#139248) fix(gateway): deliver loopback ask_user prompts to the originating channel (#138831) docs: keep root agent instructions focused on repository policy (#139258) fix(matrix): exclude archived state roots from scans and selection (#112329) perf(agents): prepare exact tool names once per catalog batch (#139267) fix: preserve Swarm progress and simplify owner plumbing (#139269) fix(tailscale): release claims after gateway process-group exit (#139261) refactor(cli): simplify lazy command registration (#139108) test(ui): retain synthetic widget failure evidence (#139229) fix(buzz): keep implicit replies at thread root (#138879) test(diagnostics): await graceful Gateway shutdown before cleanup (#139247) fix(ci): unblock releases with different tooling package managers (#139259) feat(ui): add social preview links for private sessions (#139250) fix(codex): retain failed conversations for guarded continuation (#139246) chore(ui): refresh control ui locales (#139200) fix(ci): run browser checks for real-gateway inputs (#139256) fix(llama-cpp): reconcile managed router state before requests (#136363) chore(i18n): refresh native locales (#139237) refactor(worker): unify inference payload handling (#139245) refactor(terminal): replay wrapped prefixes in one pass (#139240) ...
This was referenced Sep 5, 2026
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Sep 6, 2026
) Carry the core Workshop policy owner through the existing signed approval transport, so pending lifecycle actions can reach authorized Telegram DMs. Consolidate lifecycle action metadata and verify the signed owner boundary. Fixes openclaw#135291. Based on openclaw#135406 by @gaoanze888; reported by @khaisis. Production LOC: +27/-43 (-16); tests: +38/-0. Co-authored-by: gaoanze <gaoanze@meituan.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #135291
Supersedes #135406.
What Problem This Solves
Fixes an issue where Telegram users requesting a Skill Workshop lifecycle action received no approval buttons when
skills.workshop.approvalPolicywaspending. The Gateway rejected the request before delivering the approval to the authorized owner's DM.Why This Change Was Made
Workshop policy now supplies its
workspace-skillsapproval owner to the existing signed Gateway transport. The Gateway retains its fail-closed owner and live-run checks. Lifecycle action recognition and approval text share one table, removing duplicated policy while preserving titles, descriptions, severity, decisions, and timeout.This maintainer takeover preserves @gaoanze888's producer-side fix from #135406, reported by @khaisis in #135291 and reproduced on 2026.9.1 by @stemkat100. The new regression verifies the owner in the signed Gateway request for apply, reject, quarantine, and restore.
User Impact
Operators using pending Workshop approvals receive the existing decision controls in their configured Telegram DM, including when the request starts in a group. Approving lets the lifecycle action finish and produces a visible result in the originating chat. Automatic policy behavior is unchanged.
Evidence
Verified head:
4ee7ea7e0c05711c0adaa3b745a276c8f1b0e711.approvalOwnerPluginId: "workspace-skills".GREEN.Crabbox validation/build: run_c237dcb87132. Its subsequent standalone fixture launch hit a syntax error before Gateway startup; the corrected fixture reused the verified candidate build in successful run_f067cb152f81. The retry rechecked the artifact SHA, clean tracked tree, toolchain pin, and absent IAM credentials. Both runs used a fresh HOME, public networking, no Tailscale, and no credential hydration.
Commands executed remotely:
CI=1 OPENCLAW_VITEST_MAX_WORKERS=2 node scripts/run-vitest.mjs src/agents/tools/gateway.runtime-identity.test.ts -t 'signs the Skill Workshop' CI=1 OPENCLAW_VITEST_MAX_WORKERS=2 node scripts/run-vitest.mjs src/skills/workshop/policy.test.ts src/agents/tools/gateway.runtime-identity.test.ts src/gateway/server-methods/plugin-approval.agent-runtime.test.ts src/agents/agent-tools.before-tool-call.embedded-mode.test.ts extensions/telegram/src/approval-handler.runtime.test.ts CI=1 node scripts/check-changed.mjs -- src/skills/workshop/policy.ts src/skills/workshop/policy.test.ts src/agents/tools/gateway.runtime-identity.test.ts OPENCLAW_BUILD_PRIVATE_QA=1 pnpm build node --import tsx /tmp/pr135406-proof-design/workshop-approval-proof.mjs /work/crabbox/cbx_24d5ded707e7/fresh-pr-openclaw-openclaw-139248 .artifacts/qa-e2e/pr135406-afterThe fixture uses
createQaGatewayChildfromextensions/qa-lab/api.tswith builtdist/entry.js, fresh state, and separate unused ports. The mock provider invokes the real Workshop create/apply tools. Production policy, signing, Gateway registration, Telegram delivery, and callback handling produce the following synthetic transcript verdict:{ "head": "4ee7ea7e0c05711c0adaa3b745a276c8f1b0e711", "provider": "aws", "lease": "cbx_24d5ded707e7", "run": "run_f067cb152f81", "runtime": "openclaw", "externalAPIs": "synthetic Telegram Bot API and OpenAI Responses", "cases": [ { "origin": "dm", "originChatId": 1357, "approvalDm": 1357, "approvalOwner": "workspace-skills", "cardMessageId": 9001, "buttons": [ "Allow Once", "Deny" ], "callbackUpdateId": 2, "decision": "allow-once", "terminalReason": "user", "appliedProposal": "approval-dm-proof-20260905-d40349281f", "finalMessageId": 9002, "terminalCardButtons": [], "providerRequests": 3, "gatewayCleanup": "confirmed-stopped", "passed": true, "gatewayPort": 33369 }, { "origin": "group", "originChatId": -1002468135790, "approvalDm": 1357, "approvalOwner": "workspace-skills", "cardMessageId": 9001, "buttons": [ "Allow Once", "Deny" ], "callbackUpdateId": 2, "decision": "allow-once", "terminalReason": "user", "appliedProposal": "approval-group-proof-20260905-2917eb2617", "finalMessageId": 9003, "terminalCardButtons": [], "providerRequests": 3, "gatewayCleanup": "confirmed-stopped", "passed": true, "gatewayPort": 45547 } ] }Proof limits: this exercises a real Gateway with the OpenClaw runtime and synthetic Telegram/model APIs. The shared Codex host-tool contract was directly inspected at pinned
rust-v0.153.4; no live Codex-provider call is claimed. Native Telegram Test Server proof was unavailable because the Convex CLI is absent and this contributor-code lane remains secretless.The September 5 ClawSweeper review's delivery/callback/result/terminal-card follow-up and the original PR's signed-owner regression request are now addressed by the evidence above.
git diff --numstatagainst the main baseline:Named follow-up: Bind trusted-policy approvals to their registered plugin owner. The separate registered-policy path in
src/plugins/trusted-tool-policy.tsforwards an evaluator's optional approval owner, whereas ordinary hooks stamp the registry owner. It needs an external-plugin fixture and signed-boundary proof. No bundled production registrant was found; the core Workshop path repaired here is independent of that API.