fix(skills): stale create proposals with existing targets by sahibzada-allahyar · Pull Request #90493 · openclaw/openclaw

sahibzada-allahyar · 2026-06-05T00:53:01Z

Summary

Reconciles pending Skill Workshop create proposals when their target skill file already exists in the workspace.
Marks those proposals stale during scoped list/inspect flows so the UI and tool surface stop presenting an already-installed skill as still waiting to be used.
Adds regression coverage for both list and inspect reconciliation paths.

Verification

node scripts/run-vitest.mjs src/skills/workshop/service.test.ts
node scripts/run-vitest.mjs src/skills/workshop/service.test.ts src/gateway/server-methods/skills.proposals.test.ts src/agents/tools/skill-workshop-tool.test.ts
git diff --check

Real behavior proof

Behavior addressed: A pending Skill Workshop create proposal whose target skills/<name>/SKILL.md already exists is reconciled to stale instead of remaining actionable/waiting.
Real environment tested: macOS 15.5 arm64, Node v26.0.0, pnpm v11.2.2, current OpenClaw source checkout on branch fastino-90388-skill-proposal-stale-c.
Exact steps or command run after this patch: pnpm exec tsx --eval 'async function main() { const fs = await import("node:fs/promises"); const os = await import("node:os"); const path = await import("node:path"); const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-proof-state-")); const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-proof-workspace-")); process.env.OPENCLAW_STATE_DIR = stateDir; const service = await import("./src/skills/workshop/service.ts"); const proposal = await service.proposeCreateSkill({ workspaceDir, name: "Manual Install Proof", description: "Shows stale reconciliation", content: "# Manual Install Proof\n\nProposed content.\n" }); console.log("before", (await service.inspectSkillProposal(proposal.record.id))?.record.status); await fs.mkdir(path.dirname(proposal.record.target.skillFile), { recursive: true }); await fs.writeFile(proposal.record.target.skillFile, "---\nname: manual-install-proof\ndescription: Already installed\n---\n\n# Manual Install Proof\n", "utf8"); await service.listSkillProposals({ workspaceDir }); const after = await service.inspectSkillProposal(proposal.record.id); console.log("after", after?.record.status, after?.record.statusReason); } main();'
Evidence after fix: copied terminal output from the command:

before pending
after stale Target skill was created after proposal creation.

Observed result after fix: Listing scoped proposals after the skill file exists updates the proposal record to stale, and a later inspect call returns that stale status/reason.
What was not tested: A live Control UI browser session was not opened; the tested path exercises the shared Skill Workshop service APIs used by the UI, gateway proposal methods, and skill-workshop tool tests.

Platforms tested

macOS 15.5 arm64

clawsweeper · 2026-06-05T00:54:41Z

Codex review: needs maintainer review before merge. Reviewed June 4, 2026, 11:21 PM ET / 03:21 UTC.

Summary
The branch updates Skill Workshop list/inspect service flows to mark pending create proposals stale when their target SKILL.md already exists, and adds regression tests for list, inspect, and terminal-status preservation.

PR surface: Source +64, Tests +156. Total +220 across 2 files.

Reproducibility: yes. source-reproducible: current main's scoped list/inspect paths do not compare pending create targets with the workspace SKILL.md, so a manually created target can remain listed as pending until a later mutation path handles it.

Review metrics: 1 noteworthy metric.

Read-path status writes: 2 APIs changed. listSkillProposals and inspectSkillProposal can now persist stale, which is the state-sensitive behavior maintainers should notice before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

none.

Risk before merge

[P1] This PR intentionally turns scoped list/inspect reads into proposal-status write paths; if the pending/create guard is wrong, Skill Workshop proposal state could be reclassified unexpectedly.

Maintainer options:

Merge with state-mutation awareness (recommended)
Accept the read-path status mutation because it is scoped to pending create proposals, uses the existing target lock, and has service proof for the intended stale transition.
Ask for stronger race proof
If maintainers want more confidence before merge, ask for a true interleaving regression that changes the record after the outer read and before the lock body.

Next step before merge

No automated repair is needed; the remaining action is normal maintainer/CI merge review for a patch with no blocking findings.

Security
Cleared: The diff only changes Skill Workshop service logic and colocated tests; it does not add dependencies, CI, package scripts, secret handling, or external code execution paths.

Review details

Best possible solution:

Keep the reconciliation in the shared Skill Workshop service, guarded by the target lock and fresh record status checks, and merge after normal CI/maintainer gates are satisfied.

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

Yes, source-reproducible: current main's scoped list/inspect paths do not compare pending create targets with the workspace SKILL.md, so a manually created target can remain listed as pending until a later mutation path handles it.

Is this the best way to solve the issue?

Yes. The PR fixes the shared service read paths used by UI, gateway, CLI, and agent tool callers, and the follow-up uses existing target locks plus fresh record reads instead of a UI-only warning or duplicate state path.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies after-fix terminal proof from a real checkout showing a pending create proposal becoming stale with the expected reason after the target skill file appears.
remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

P2: This is a normal-priority Skill Workshop state bugfix with limited blast radius but visible user confusion when stale proposals remain actionable.
merge-risk: 🚨 session-state: The patch changes persisted Skill Workshop proposal lifecycle state during read-oriented list/inspect flows.
rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies after-fix terminal proof from a real checkout showing a pending create proposal becoming stale with the expected reason after the target skill file appears.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies after-fix terminal proof from a real checkout showing a pending create proposal becoming stale with the expected reason after the target skill file appears.

Evidence reviewed

PR surface:

Source +64, Tests +156. Total +220 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	80	16	+64
Tests	1	157	1	+156
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	237	17	+220

What I checked:

Repository policy applied: Root AGENTS.md was read fully, plus scoped guidance for src/agents/tools and src/gateway/server-methods; the state-sensitive PR review policy affected this verdict. (AGENTS.md:21, f3abe61b78fa)
Current main lacks stale reconciliation: On current main, scoped listSkillProposals only filters manifest entries by workspace and inspectSkillProposal hydrates the stored record without checking whether a pending create target file now exists. (src/skills/workshop/service.ts:79, f3abe61b78fa)
PR implements shared-service reconciliation: The PR adds reconcileStaleCreateProposals, calls it from scoped list, and adds inspect-time reconciliation for pending create records whose target file now exists. (src/skills/workshop/service.ts:79, 5b1e3b7c4aa3)
Race follow-up uses existing target lock: The follow-up commit re-reads proposal records inside withSkillProposalTargetLock before marking stale, matching the existing per-target lock used by proposal mutations. (src/skills/workshop/service.ts:96, 5b1e3b7c4aa3)
Shared callers use the fixed paths: Gateway proposal list/inspect methods call the shared service with a resolved workspace, so the fix reaches the Control UI path rather than only a local helper. (src/gateway/server-methods/skills.ts:328, f3abe61b78fa)
Tool caller uses the fixed paths: The agent skill_workshop tool routes list and inspect actions through the same shared service functions with a workspace scope. (src/agents/tools/skill-workshop-tool.ts:147, f3abe61b78fa)

Likely related people:

Shakker: Current-main blame for the Skill Workshop list path, gateway proposal methods, and target-lock helper points to the same grafted commit in this checkout. (role: current-main area contributor; confidence: medium; commits: 85e16da2b48a; files: src/skills/workshop/service.ts, src/skills/workshop/store.ts, src/gateway/server-methods/skills.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.

sahibzada-allahyar · 2026-06-05T00:56:43Z

@clawsweeper re-review

clawsweeper · 2026-06-05T00:56:46Z

🦞🧹
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.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26988803186
Updated: 2026-06-05T01:04:53.724Z

sahibzada-allahyar · 2026-06-05T03:13:33Z

Addressed the stale reconciliation race follow-up in commit 5b1e3b7c.

What changed:

stale create reconciliation now takes the proposal target lock and re-reads the proposal record before marking it stale
inspect-time stale reconciliation uses the same lock-and-reread path
added regression coverage for concurrent applied/rejected records so stale reconciliation does not overwrite a terminal status

Verification:

$ node scripts/test-projects.mjs src/skills/workshop/service.test.ts
[test] starting test/vitest/vitest.unit.config.ts

 RUN  v4.1.7 /Users/allahyar/Documents/fastino-tasks/openclaw-tui-90388c

 Test Files  1 passed (1)
      Tests  26 passed (26)
   Start at  04:12:39
   Duration  1.51s (transform 212ms, setup 86ms, import 319ms, tests 1.05s, environment 0ms)

[test] passed 1 Vitest shard in 4.20s

$ git diff --check
# no output

@clawsweeper re-review

clawsweeper · 2026-06-05T03:13:35Z

🦞🧹
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.

Re-review progress:

State: Complete
Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
Run: https://github.com/openclaw/clawsweeper/actions/runs/26993097514
Updated: 2026-06-05T03:22:15.008Z

sahibzada-allahyar · 2026-06-05T23:05:02Z

Checked the red CI locally at the current PR head (5b1e3b7). The failures appear stale now:\n\ntext\npnpm lint --threads=8\n# passed\n\npnpm check:architecture\n# passed\n\npnpm run lint:tmp:no-raw-channel-fetch\n# passed\n\n\nI do not have permission to rerun the failed GitHub Actions jobs directly (GitHub returns that the run requires admin rights to rerun). Could a maintainer rerun the failed CI jobs when convenient?

fix(skills): stale create proposals with existing targets

01f0e97

openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 5, 2026

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 5, 2026

hxy91819 mentioned this pull request Jun 5, 2026

fix(ci): scope PR merge diff checks to first parent #90287

Merged

fix(skills): preserve proposal status during stale reconciliation

5b1e3b7

openclaw-barnacle Bot added size: M and removed size: S labels Jun 5, 2026

clawsweeper Bot mentioned this pull request Jun 6, 2026

fix(skills): reconcile stale workshop proposals when target skill exists or is missing #90421

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(skills): stale create proposals with existing targets#90493

fix(skills): stale create proposals with existing targets#90493
sahibzada-allahyar wants to merge 2 commits into
openclaw:mainfrom
sahibzada-allahyar:fastino-90388-skill-proposal-stale-c

sahibzada-allahyar commented Jun 5, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

sahibzada-allahyar commented Jun 5, 2026

Uh oh!

clawsweeper Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

sahibzada-allahyar commented Jun 5, 2026

Uh oh!

clawsweeper Bot commented Jun 5, 2026 •

edited

Loading

Uh oh!

sahibzada-allahyar commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

sahibzada-allahyar commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Real behavior proof

Platforms tested

Uh oh!

clawsweeper Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sahibzada-allahyar commented Jun 5, 2026

Uh oh!

clawsweeper Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sahibzada-allahyar commented Jun 5, 2026

Uh oh!

clawsweeper Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sahibzada-allahyar commented Jun 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

sahibzada-allahyar commented Jun 5, 2026 •

edited

Loading

clawsweeper Bot commented Jun 5, 2026 •

edited

Loading

clawsweeper Bot commented Jun 5, 2026 •

edited

Loading

clawsweeper Bot commented Jun 5, 2026 •

edited

Loading