Repository navigation
fix(skills): carry the workspace-skills approval owner - #135406
gaoanze888 wants to merge 2 commits into
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 7:39 PM ET / 23:39 UTC. ClawSweeper reviewWhat this changesThe PR adds the workspace-skills owner to pending Skill Workshop approvals and adds focused policy and agent-hook tests. Regression provenancePossible regression — probable (reviewed change; failure trace). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 5 items remain The producer-side change matches the Gateway’s signed-owner contract, but the revised regression still does not observe that signed owner or a delivered Telegram approval. Priority: P1 Review scores
Verification
How this fits togetherSkill Workshop creates pending lifecycle approvals; the agent layer signs them for the Gateway, which routes an interactive decision to the authorized channel owner. flowchart LR
A[Skill Workshop action] --> B[Pending approval policy]
B --> C[Agent approval transport]
C --> D[Signed Gateway request]
D --> E[Telegram owner DM]
E --> F[Approval decision]
Before merge
Findings
Agent review detailsSecurityNone. PR surfaceSource +3, Tests +18. Total +21 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 the producer-side owner field, add one regression that observes workspace-skills in the signed Gateway request from a real Skill Workshop apply path, and attach a redacted Telegram Test Server run or supported mock-Gateway delivery-and-resolution verdict. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main omits the owner, its transport signs that value, and the Gateway rejects signed requests with no owner; no real Telegram run was performed in this read-only review. Is this the best way to solve the issue? Yes for the producer location: the policy creates the approval object and the transport deliberately signs its pluginId, while moving ownership into transport code would detach it from the policy that owns it. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 80ae248de1c0. 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 (2 earlier review cycles)
|
This comment was marked as spam.
This comment was marked as spam.
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
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>
|
Landed in #139248 as 911d7d5b0c10. Thanks @gaoanze888; your contributor credit is preserved in the landed commit. The takeover retains the producer-side owner repair, consolidates lifecycle metadata, and adds regressions that fail before the fix at the signed-request boundary. All 86 focused tests, changed checks, build, reviews, and exact-head CI passed. AWS Crabbox proof with a real Gateway and mock Telegram/model APIs verified both DM and group origins through the private owner card, callback, applied proposal, and terminal card update. Closing this PR as superseded by the landed repair. |
) 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>
|
🦞👀 Command router queued. I will update this comment with the next step. |
Closes #135291
What Problem This Solves
Fixes a 2026.8.1 regression where a Skill Workshop lifecycle approval (
approvalPolicy: "pending") requested from a Telegram group produced a proposal but no approval/rejection buttons were delivered to the authorized owner DM, because the Gateway rejected the request withsigned plugin approval owner is unavailable.Why This Change Was Made
resolveSkillWorkshopToolApprovalbuilt arequireApprovalobject without apluginId. The approval transport then calledwithGatewayToolApprovalOwner(undefined, ...), leavingapprovalOwnerPluginIdunset, and the Gateway's intentional fail-closed authority check rejected the ownerless request before Telegram delivery. This change carries the coreworkspace-skillsowner on the approval so the signed request reaches the delivery path; the Gateway fail-closed validation is unchanged.User Impact
Skill Workshop lifecycle approvals initiated in a Telegram group now reach the authorized owner's DM with approval/rejection buttons, so pending proposals can be approved or rejected instead of silently stalling.
Evidence
Exact head:
7c97cef9d5c1421a6a9fd92804839e7ef1ef9a8e.src/skills/workshop/policy.test.ts— 9 passed, including apluginId: "workspace-skills"assertion on the pending approval result.src/agents/agent-tools.before-tool-call.embedded-mode.test.ts— new boundary regression "routes skill_workshop lifecycle approvals through the workspace-skills owner": withapprovalPolicy: "pending", a realskill_workshopapplycall reachesplugin.approval.requestthrough the production approval path (not a mocked hook result). 26 passed across both files.check:changedpass; the only local lane failure is unchangedui/vite.config.tsmissing apakodeclaration in the reused dependency tree.AI Assistance
AI-assisted implementation. I manually verified the approval registration boundary, the owner propagation, the focused producer and boundary regressions, and the changed-file gates.