Skip to content

fix(skill-workshop): honor pending approval for tool suggestions [AI]#78516

Merged
pgondhi987 merged 7 commits intoopenclaw:mainfrom
pgondhi987:fix/fix-565
May 7, 2026
Merged

fix(skill-workshop): honor pending approval for tool suggestions [AI]#78516
pgondhi987 merged 7 commits intoopenclaw:mainfrom
pgondhi987:fix/fix-565

Conversation

@pgondhi987
Copy link
Copy Markdown
Contributor

@pgondhi987 pgondhi987 commented May 6, 2026

Summary

  • Problem: skill_workshop tool suggestions could request immediate application even when the plugin was configured for pending review.
  • Why it matters: pending review should be the authoritative operator-controlled write gate for workspace skills.
  • What changed: routed tool suggestions through the shared proposal apply/store helper, limited the apply parameter to an auto-mode opt-out, and added regression coverage for pending mode with apply: true.
  • What did NOT change (scope boundary): no new permissions, config keys, network calls, or skill storage locations were added.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor required for the fix
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

  • Related: N/A
  • This PR addresses a bug or regression

Real behavior proof (required for external PRs)

  • Behavior or issue addressed: pending-mode skill suggestions remain queued and do not write skills/<name>/SKILL.md when the tool call includes apply: true.
  • Real environment tested: Not run in this metadata step.
  • Exact steps or command run after this patch: Not run in this metadata step.
  • Evidence after fix: Regression test added in extensions/skill-workshop/index.test.ts.
  • Observed result after fix: Not run in this metadata step.
  • What was not tested: Runtime command verification was not run as part of this metadata-only step.
  • Before evidence: N/A

Root Cause (if applicable)

  • Root cause: the tool suggestion path had separate apply/store logic and treated the tool-supplied apply: true argument as sufficient to apply a proposal before checking whether the configured approval policy allowed automatic writes.
  • Missing detection / guardrail: existing tests covered auto mode with apply: false, but not pending mode with apply: true.
  • Contributing context (if known): the agent-end auto-capture path already used the shared approval-policy helper, while the tool suggestion path duplicated similar logic.

Regression Test Plan (if applicable)

  • Coverage level that should have caught this:
    • Unit test
    • Seam / integration test
    • End-to-end test
    • Existing coverage already sufficient
  • Target test or file: extensions/skill-workshop/index.test.ts
  • Scenario the test should lock in: pending mode plus action: "suggest" plus apply: true returns status: "pending", does not create a workspace skill file, and leaves no applied proposal entry.
  • Why this is the smallest reliable guardrail: it exercises the registered plugin tool with real temp workspace and state directories without requiring a broader gateway run.
  • Existing test that already covers this (if any): existing auto-mode opt-out coverage remains in place.
  • If no new test is added, why not: N/A

User-visible / Behavior Changes

In pending mode, tool-created skill suggestions remain queued for review even if the tool call includes apply: true. In auto mode, apply: false still queues instead of applying.

Diagram (if applicable)

Before:
[tool suggest + pending mode + apply:true] -> [workspace skill written]

After:
[tool suggest + pending mode + apply:true] -> [proposal queued] -> [no workspace skill write]

Security Impact (required)

  • New permissions/capabilities? (Yes/No): No
  • Secrets/tokens handling changed? (Yes/No): No
  • New/changed network calls? (Yes/No): No
  • Command/tool execution surface changed? (Yes/No): Yes
  • Data access scope changed? (Yes/No): No
  • If any Yes, explain risk + mitigation: the existing tool behavior now honors pending review for workspace-skill writes; the change narrows write behavior and preserves the existing auto-mode opt-out.

Repro + Verification

Environment

  • OS: Not run in this metadata step
  • Runtime/container: Not run in this metadata step
  • Model/provider: N/A
  • Integration/channel (if any): skill workshop plugin tool
  • Relevant config (redacted): approvalPolicy: "pending"

Steps

  1. Register the skill workshop plugin with approvalPolicy: "pending".
  2. Invoke skill_workshop with action: "suggest", apply: true, and a new skill body.
  3. Inspect the returned status and workspace skill path.

Expected

  • The result is pending.
  • No skills/<name>/SKILL.md file is written.
  • The proposal is stored in the pending list, not the applied list.

Actual

  • After this patch, the added regression test encodes the expected behavior. Runtime command verification was not run in this metadata step.

Evidence

Attach at least one:

  • Failing test/log before + passing after
  • Trace/log snippets
  • Screenshot/recording
  • Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

  • Verified scenarios: reviewed the changed helper and tool path so auto-apply is gated by approvalPolicy === "auto"; reviewed the regression test for pending mode with apply: true.
  • Edge cases checked: auto mode with apply: false continues to use the existing opt-out behavior; agent-end proposal handling still uses default helper behavior.
  • What you did not verify: did not run package tests, typecheck, formatter, or a live OpenClaw setup in this metadata step.

Review Conversations

  • I replied to or resolved every bot review conversation I addressed in this PR.
  • I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

  • Backward compatible? (Yes/No): Yes
  • Config/env changes? (Yes/No): No
  • Migration needed? (Yes/No): No
  • If yes, exact upgrade steps: N/A

Risks and Mitigations

  • Risk: auto-mode callers that expected apply: true to force writes in pending mode will now receive queued proposals.
    • Mitigation: this matches the pending policy contract; auto mode without apply: false still applies safe proposals automatically.

AI-assisted: yes

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels May 6, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 6, 2026 •
edited
Loading

Codex review: needs real behavior proof before merge.

Summary
The PR routes skill_workshop suggestions through the shared proposal helper, adds pending-mode and apply-approval coverage, and updates prompt/docs wording for apply semantics.

Reproducibility: yes. source-reproducible: current main sets shouldApply from raw.apply === true before pending policy can queue the proposal. The focused scenario is pending-mode skill_workshop suggest with apply: true; no live command was run in this read-only review.

Real behavior proof
Needs real behavior proof before merge: The PR body says no real setup or runtime command was run after the patch, and the PR head has a failing Real behavior proof check. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Contributor or maintainer action is needed for external real-behavior proof and protected-label/changelog handling; automation cannot supply the contributor's runtime proof.

Security
Cleared: The diff narrows an existing workspace-skill write path and adds an approval gate, with no new dependency, workflow, secret, network, or permission surface found.

Review findings

  • [P3] Add the required changelog entry — extensions/skill-workshop/src/tool.ts:145-154
Review details

Best possible solution:

Keep the shared-helper fix, add a concise active-version changelog entry or maintainer-approved merge-time entry, and require redacted after-fix runtime proof before merge.

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

Yes, source-reproducible: current main sets shouldApply from raw.apply === true before pending policy can queue the proposal. The focused scenario is pending-mode skill_workshop suggest with apply: true; no live command was run in this read-only review.

Is this the best way to solve the issue?

Yes for the implementation direction: routing the tool path through the shared apply/store helper is the narrow maintainable fix. The PR is not merge-ready until real behavior proof and changelog handling are resolved.

Full review comments:

  • [P3] Add the required changelog entry — extensions/skill-workshop/src/tool.ts:145-154
    This user-visible Skill Workshop fix changes tool write behavior and public docs, but the PR does not update CHANGELOG.md. Add a single-line active-version entry for the pending-approval behavior before merge; no bot or maintainer Thanks @... handle is required.
    Confidence: 0.86

Overall correctness: patch is correct
Overall confidence: 0.87

What I checked:

Likely related people:

  • steipete: GitHub path history shows Peter Steinberger introduced the experimental Skill Workshop plugin, including the central tool/helper/docs surface that contains the pending-approval behavior. (role: introduced current-main implementation; confidence: high; commits: c742a706bf2b, b2472d65607a; files: extensions/skill-workshop/src/tool.ts, extensions/skill-workshop/src/workshop.ts, docs/plugins/skill-workshop.md)
  • vincentkoc: Recent Skill Workshop history includes live hook/config fixes and docs maintenance around the plugin, making Vincent Koc a useful follow-up routing candidate for the approval-policy and docs surface. (role: recent adjacent maintainer; confidence: medium; commits: 4955e57024b3, 861a593921e2; files: extensions/skill-workshop/index.ts, docs/plugins/skill-workshop.md)

Remaining risk / open question:

  • The external PR still has no after-fix real behavior proof, and the required proof check is failing.
  • The changelog requirement is unresolved unless a maintainer explicitly accepts handling this entry at merge time.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 42ecd5d95eae.

@pgondhi987
Copy link
Copy Markdown
Contributor Author

pgondhi987 commented May 7, 2026 •
edited by clawsweeper Bot
Loading

Not applicable; changelog is handled at merge time.

Quoted comment from @clawsweeper:

Codex review: needs real behavior proof before merge.

Summary
The PR routes skill_workshop tool suggestions through the shared proposal apply/store helper so pending approval mode queues suggestions even when apply: true is supplied, with a regression test and prompt wording update.

Reproducibility: yes. from source inspection: on current main, action: "suggest" with approvalPolicy: "pending" and apply: true reaches the immediate apply branch. I did not run a live command, so this is source-reproducible rather than locally reproduced.

Real behavior proof
Needs real behavior proof before merge: The PR body explicitly says no real setup or runtime command was run after the patch, and tests alone do not satisfy the external-PR proof gate. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
The protected maintainer label, missing external real-behavior proof, stale docs, and missing changelog require contributor or maintainer follow-up before merge.

Security
Cleared: The diff narrows an existing workspace-skill write path and does not add permissions, dependencies, network calls, scripts, workflows, package metadata, or secret handling.

Review findings

  • [P2] Update the public suggest docs — extensions/skill-workshop/src/prompt.ts:6-7
  • [P3] Add the required changelog entry — extensions/skill-workshop/src/tool.ts:151-154
Review details

Best possible solution:

Keep pending mode as the authoritative write gate, update the Skill Workshop docs and changelog to match the new apply semantics, and require real runtime proof before maintainer merge review.

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

Yes, from source inspection: on current main, action: "suggest" with approvalPolicy: "pending" and apply: true reaches the immediate apply branch. I did not run a live command, so this is source-reproducible rather than locally reproduced.

Is this the best way to solve the issue?

No, not as-is. Routing through the shared helper is the narrow maintainable code fix, but merge should wait for docs/changelog alignment and real behavior proof.

Full review comments:

  • [P2] Update the public suggest docs — extensions/skill-workshop/src/prompt.ts:6-7
    After this change, apply: true no longer forces a write in pending mode, but the public Skill Workshop docs still show a suggest example titled “Force a safe write (apply: true)” without limiting that behavior to auto mode. Please update docs/plugins/skill-workshop.md so operators do not follow stale instructions.
    Confidence: 0.88
  • [P3] Add the required changelog entry — extensions/skill-workshop/src/tool.ts:151-154
    This is a user-visible skill-workshop fix/security hardening, but the PR changes only plugin code/tests/prompt and does not touch CHANGELOG.md. Add a single-line Unreleased entry for the pending-approval behavior change before merge.
    Confidence: 0.84

Overall correctness: patch is incorrect
Overall confidence: 0.86

What I checked:

  • Current-main root cause: On current main, the tool suggestion path sets shouldApply when raw.apply === true before the pending approval policy can gate the write. (extensions/skill-workshop/src/tool.ts:153, 99b17263a12d)
  • Shared helper contract: The existing shared helper applies automatically only when approvalPolicy === "auto", so routing the tool path through it is the right code seam for pending mode. (extensions/skill-workshop/src/workshop.ts:63, 99b17263a12d)
  • Docs contract and mismatch: The Skill Workshop docs say pending mode queues instead of writing, but the same suggest section still presents apply: true as a forced safe write example, which becomes stale after this PR. Public docs: docs/plugins/skill-workshop.md. (docs/plugins/skill-workshop.md:346, 99b17263a12d)
  • PR changed files: The public PR file list contains only skill-workshop code/test/prompt files, with no docs or changelog update. (ad30017e4856)
  • Real behavior proof gate: The PR body says no real environment or runtime command was run after the patch, and the head SHA has a failing Real behavior proof check. (ad30017e4856)

Likely related people:

  • vincentkoc: Current-main blame for the central skill-workshop tool and shared apply/store helper lines points to commit 5e218b4, making this the best maintainer routing candidate from available history. (role: likely follow-up owner; confidence: medium; commits: 5e218b402f83; files: extensions/skill-workshop/src/tool.ts, extensions/skill-workshop/src/workshop.ts, extensions/skill-workshop/index.ts)

Remaining risk / open question:

  • The external PR has no after-fix real behavior proof and the required proof check is failing.
  • At review time, 26 check runs for the head SHA were still queued, so CI was not complete.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 99b17263a12d.

Re-review progress:

@openclaw-barnacle openclaw-barnacle Bot added the docs Improvements or additions to documentation label May 7, 2026
@pgondhi987 pgondhi987 merged commit 2d65ead into openclaw:main May 7, 2026
12 of 13 checks passed
@github-actions github-actions Bot mentioned this pull request May 7, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request May 9, 2026
@pgondhi987
fix(skill-workshop): honor pending approval for tool suggestions [AI] (…
5270d45 
…openclaw#78516)

* fix: honor pending skill workshop approvals

* addressing review-skill

* addressing codex review

* addressing codex review

* fix: require approval before skill workshop apply

* docs: add changelog entry for PR merge
rogerdigital pushed a commit to rogerdigital/openclaw that referenced this pull request May 9, 2026
@pgondhi987 @rogerdigital
fix(skill-workshop): honor pending approval for tool suggestions [AI] (…
eb89819 
…openclaw#78516)

* fix: honor pending skill workshop approvals

* addressing review-skill

* addressing codex review

* addressing codex review

* fix: require approval before skill workshop apply

* docs: add changelog entry for PR merge
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

docs Improvements or additions to documentation maintainer Maintainer-authored PR size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@pgondhi987