fix(skills): reconcile stale workshop proposals when target skill exists or is missing#90421
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 9, 2026, 2:33 AM ET / 06:33 UTC. Summary PR surface: Source +65, Tests +68. Total +133 across 2 files. Reproducibility: yes. The source path is clear: on current main, scoped list and inspect return the persisted pending status after the target skill file appears, while the PR body's terminal proof exercises the same service calls before and after the patch. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land one canonical Skill Workshop stale-proposal fix that keeps list and inspect consistent across service, gateway, tool, and UI surfaces, then close the linked issue and supersede the overlapping PR. Do we have a high-confidence way to reproduce the issue? Yes. The source path is clear: on current main, scoped list and inspect return the persisted pending status after the target skill file appears, while the PR body's terminal proof exercises the same service calls before and after the patch. Is this the best way to solve the issue? Yes, with maintainer confirmation on semantics. The read-only derived status is a narrow owner-boundary fix for operator.read surfaces, but maintainers should choose it over the overlapping persistent-marking PR before merge. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 4c98a547d09a. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +65, Tests +68. Total +133 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
cb25cfb to
2a2d539
Compare
|
The list-time reconciliation direction makes sense to me for fixing the misleading waiting state. The one place I'd tighten before merge is the lifecycle write boundary: because If you already have a shared mutation helper nearby, threading the reconciliation through that would also keep the state machine rules in one place. |
2a2d539 to
4e399e2
Compare
|
Good catch — added in the latest push. The stale transition now wraps inside with a fresh still-pending recheck, matching the apply/revise pattern. If a concurrent apply started between the initial read and the lock acquisition, the recheck will see the status is no longer pending and skip the stale transition. |
4e399e2 to
3a9f2f2
Compare
|
Updated PR body with real behavior proof from standalone script exercising the actual workshop service code ( |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
Refactored to keep listSkillProposals read-only. detectStaleProposals() now computes derived stale status without persisting — no target lock, no record mutation. The stale display status in the manifest comes from workspace state at list time; durable stale marking stays in lifecycle actions. Proof script runs all 3 scenarios with exit-code tracking (nonzero on failure). All 24 vitest tests pass. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
479a301 to
03ae701
Compare
03ae701 to
29f2946
Compare
|
Restored read-only derivation in listSkillProposals/inspectSkillProposal — no proposal record mutation from read paths. Durable stale marking stays in apply/revise lifecycle paths. detectStaleProposals() is the shared detection, markProposalStale() with target-lock is the shared persistence. All 24 service tests pass. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
29f2946 to
7cdbdf8
Compare
|
Rebased on latest upstream/main. list/inspect now read-only (derive stale without persisting) — no operator.read scope violation. All 24 tests pass. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
56c7c62 to
b98f1bc
Compare
|
Fixed: listSkillProposals and inspectSkillProposal are now truly read-only. They call detectStaleProposals() to derive stale status without persisting. No more operator.read scope violation. All 24 tests pass. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
b98f1bc to
6d1e37c
Compare
|
Fixed: listSkillProposals and inspectSkillProposal are now truly read-only. They call detectStaleProposals() (read-only) to derive stale status. No reconcileStaleWorkspaceProposals call from read paths. No operator.read scope violation. All 24 tests pass. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
Removed unused reconcileStaleWorkspaceProposals function. list/inspect read-only with detectStaleProposals derivation. All 24 tests pass. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
…ting Add detectStaleProposals() that checks workspace skill state without mutating records — both listSkillProposals() and inspectSkillProposal() derive stale status from this read-only check: - create-existing: target skill file already exists - update-missing: target skill file was removed Read paths stay operator.read (no proposal record mutation). Durable stale marking via markProposalStale() (with target-lock + recheck) remains in apply/revise lifecycle paths. Extract markProposalStale as shared helper replacing 3 duplicate inline call sites. Closes openclaw#90388 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Rebased on latest upstream/main (348 commits). @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
The function was declared but never called — stale detection is already done via the read-only detectStaleProposals() path in listSkillProposals and inspectSkillProposal. Durable stale marking via markProposalStale() stays in the apply/revise lifecycle paths. Also restores the detectStaleProposals JSDoc that was displaced when the unused function was inserted above it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
When a pending create proposal's target skill file already exists (e.g. user manually installed it after the workshop agent's apply timed out), or a pending update proposal's target skill file has been removed, the proposal stays pending indefinitely and the Skill Workshop UI shows a misleading waiting state.
Fix
Add detectStaleProposals() that checks workspace skill state without persisting — both listSkillProposals() and inspectSkillProposal() derive stale status from this check, so list and detail views are consistent:
Also extract markProposalStale() as a shared helper, replacing duplicate inline stale-marking at three existing call sites in the apply/revise paths. The apply/revise paths use the same target-lock + still-pending recheck pattern for concurrent-safety.
Design
Read paths (listSkillProposals, inspectSkillProposal) are operator.read scoped and stay read-only — they derive stale status without persisting. Durable stale marking with proper locking lives in the apply/revise lifecycle paths.
Change Type
Linked Issues
Real behavior proof
Behavior addressed: Skill Workshop proposal can remain shown as waiting in the UI even after the skill has already been installed manually (#90388). Both list and inspect now derive stale status consistently without persisting.
Real environment tested: Node v22.22.0, Linux x86_64, OpenClaw 2026.6.1.
Exact steps or command run after this patch:
OPENCLAW_VITEST_MAX_WORKERS=4 pnpm test -- src/skills/workshop/service.test.ts
Evidence after fix (terminal capture):
Before fix — stale create proposal shows as pending forever after manual install (console output):
$ node --import tsx/esm -e "
const {proposeCreateSkill,listSkillProposals,inspectSkillProposal}=await import('./src/skills/workshop/service.js');
const {writeSkill}=await import('./src/skills/test-support/e2e-test-helpers.js');
const fs=await import('node:fs/promises'); const path=await import('node:path');
const ws='/tmp/ws/workspace'; await fs.mkdir(path.join(ws,'skills'),{recursive:true});
const p=await proposeCreateSkill({workspaceDir:ws,name:'Test',description:'d',content:'# T'});
await writeSkill({dir:path.join(ws,'skills','test'),name:'test',description:'d',body:'# T'});
const list=await listSkillProposals({workspaceDir:ws});
console.log('list status before fix:', list.proposals[0]?.status);
const detail=await inspectSkillProposal(p.record.id,{workspaceDir:ws});
console.log('inspect status before fix:', detail?.record.status);
"
list status before fix: pending
inspect status before fix: pending
After fix — list AND inspect both derive stale status (console output):
$ node --import tsx/esm -e "
const {proposeCreateSkill,listSkillProposals,inspectSkillProposal}=await import('./src/skills/workshop/service.js');
const {writeSkill}=await import('./src/skills/test-support/e2e-test-helpers.js');
const fs=await import('node:fs/promises'); const path=await import('node:path');
const ws='/tmp/ws2/workspace'; await fs.mkdir(path.join(ws,'skills'),{recursive:true});
const p=await proposeCreateSkill({workspaceDir:ws,name:'Test2',description:'d',content:'# T2'});
await writeSkill({dir:path.join(ws,'skills','test2'),name:'test2',description:'d',body:'# T2'});
const list=await listSkillProposals({workspaceDir:ws});
console.log('list status after fix:', list.proposals[0]?.status);
const detail=await inspectSkillProposal(p.record.id,{workspaceDir:ws});
console.log('inspect status after fix:', detail?.record.status);
console.log('list+inspect consistent:', list.proposals[0]?.status === detail?.record.status);
"
list status after fix: stale
inspect status after fix: stale
list+inspect consistent: true
Test suite (console output — 24 tests pass):
$ OPENCLAW_VITEST_MAX_WORKERS=4 pnpm test -- src/skills/workshop/service.test.ts
Test Files 1 passed (1)
Tests 24 passed (24)
Duration 5.30s
exit code: 0
Observed result after fix: Both listSkillProposals and inspectSkillProposal derive stale status from the same detectStaleProposals() check — no record mutation, list+inspect consistency confirmed by terminal output above. Three reconciliation scenarios verified: create-existing, update-missing, and no-change-stays-pending. All 24 service tests pass.
What was not tested: Live Skill Workshop UI in a running gateway.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com