Skip to content

fix(skills): retire missing Workshop drafts safely - #141009

Merged
obviyus merged 5 commits into
mainfrom
codex/workshop-stale-suggestions
Sep 7, 2026
Merged

fix(skills): retire missing Workshop drafts safely#141009
obviyus merged 5 commits into
mainfrom
codex/workshop-stale-suggestions

Conversation

@obviyus

@obviyus obviyus commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What Problem This Solves

Skill Workshop could show usable-looking suggestions whose drafts no longer existed. Ownership migration could also make old proposals appear newly edited, and dismissal could strand unfinished apply recovery.

Why This Change Was Made

Doctor marks missing drafts stale only when no rollback remains. Listing checks draft availability through the store owner, while Inspect and Apply still validate the full bundle. Reject and Quarantine check recovery within their state-change transaction. Ownership-only moves preserve edit time.

User Impact

Missing drafts show as unavailable with content actions disabled. Safe rejection remains possible; unfinished recovery must complete first. Restore the draft and use Inspect to finish recovery. Doctor repair preserves remaining files and installed skills. No schema or configuration change is added.

Consumers: proposal lists, Workshop actions, Doctor repair, and recovery dismissal share draft availability and recovery state.

Evidence

The original Doctor left missing drafts pending. Compiled UI/Gateway/Doctor checks verified retirement, rejection, valid siblings, preserved dates, and restart persistence. Follow-up checks preserved unfinished recovery and kept healthy suggestions visible despite damaged support files. Restored drafts completed recovery through Inspect. Doctor correctly refused during active Gateway maintenance ownership. Final mechanical test/lint fixes retain earlier proof.

@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: web-ui App: web-ui gateway Gateway runtime commands Command implementations size: M maintainer Maintainer-authored PR labels Sep 7, 2026
@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 7, 2026
@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 7, 2026, 6:16 AM ET / 10:16 UTC.

ClawSweeper review

What this changes

Skill Workshop identifies missing drafts, disables unavailable content actions, preserves unfinished recovery during dismissal, and lets Doctor retire unrecoverable proposals without resetting ownership-migration edit dates.

Merge readiness

⛔ Blocked before merge - 2 items remain

This remains a useful fix absent from inspected current main. The prior correctness findings are addressed, and the updated body records acceptance of the recovery contract; no new blocking defect was found.

Priority: P2
Reviewed head: 137a324dc3b5229eb4d77ebae341967d8b7c1523

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused owner-level repair with meaningful regression coverage, reported production observations, and resolved prior findings.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The captured body reports compiled UI/Gateway/Doctor behavior, restart persistence, and follow-up recovery through real Inspect and dismissal paths on explicitly identified builds. Those scenarios cover the changed owners; screenshot retrieval and one prior source blob were unavailable to this reviewer, and the earlier builds are not represented as final-head executions.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The captured body reports compiled UI/Gateway/Doctor behavior, restart persistence, and follow-up recovery through real Inspect and dismissal paths on explicitly identified builds. Those scenarios cover the changed owners; screenshot retrieval and one prior source blob were unavailable to this reviewer, and the earlier builds are not represented as final-head executions.
Evidence reviewed 9 items Current main still needs the fix: Current main still checks missing drafts only for pending creates using the full bundle reader. Its Doctor relocation entrypoint has no missing-draft retirement argument. The store blob also matches the pinned merge-base store exactly.
Prior dismissal finding resolved: Both Reject and Quarantine reach updateSkillProposalRecord, which now checks for any rollback row in the same transaction before changing a pending proposal's status. The existing service retains its revision check and target lock.
Prior listing and transition findings resolved: Listing checks the main draft through the shared reader without validating support files; Inspect still reads the full bundle. The stale-transition caller no longer supplies the removed workspaceDir argument.
Findings None None.
Security None None.

How this fits together

Skill Workshop stores proposed skill changes and presents them through the Gateway, agent tools, and Suggestions UI. Its recovery layer coordinates proposal metadata, draft files, and rollback evidence before installed skills can change.

flowchart TD
  A[Proposal metadata and draft files] --> B[Workshop availability check]
  B --> C[Suggestions and agent tools]
  C --> D[Revision guarded decisions]
  A --> E[Doctor recovery]
  E --> F{Rollback evidence exists}
  F -->|Yes| G[Preserve pending recovery]
  F -->|No draft or rollback| H[Mark proposal stale]
Loading

Before merge

  • Resolve merge risk (P1) - The accepted recovery policy changes operator behavior: Doctor-retired drafts do not become pending when their files are restored, and proposals with unfinished rollback evidence cannot be dismissed until recovery completes.
  • Add data-model compatibility proof - Confirm migration or upgrade compatibility proof before merge.
Agent review details

Security

None.

PR surface

Source +108, Tests +427, Docs +10. Total +545 across 20 files.

View PR surface stats
Area Files Added Removed Net
Source 13 186 78 +108
Tests 6 469 42 +427
Docs 1 10 0 +10
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 20 665 120 +545

Review metrics

Metric Value Why it matters
Recovery contract 2 dismissal actions guarded; 0 schema-version or configuration additions The repair changes durable recovery behavior while reusing existing states and transaction owners.
Production versus regression coverage Production net +107 lines; tests net +427 lines The introduced growth is justified by draft availability, transactional recovery protection, and regression coverage.

Stored data model

Persistent data-model change detected: migration/backfill/repair: src/commands/doctor-skill-workshop-relocation.ts, migration/backfill/repair: src/commands/doctor-skill-workshop-sqlite.ts, migration/backfill/repair: src/infra/state-migrations.doctor.ts, migration/backfill/repair: src/skills/workshop/store.ts, unknown-data-model-change: src/commands/doctor-skill-workshop-sqlite.ts. Confirm migration or upgrade compatibility proof before merge.

Merge-risk options

Maintainer options:

  1. Retain the accepted recovery contract (recommended)
    Land with the documented terminal stale state and recovery-before-dismissal behavior, preserving the recorded upgrade and restart evidence.

Technical review

Best possible solution:

Retain the documented, accepted recovery policy with atomic dismissal guards, preserved installed content, and the demonstrated existing-state recovery behavior.

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

Yes: removing a pending proposal's draft exposes the source-proven availability gap on inspected main; the body also reports a baseline compiled-runtime reproduction. This review did not execute that scenario.

Is this the best way to solve the issue?

Yes: the patch repairs availability and recovery at their existing owners, preserves full-bundle validation for content actions, and addresses the previous findings without introducing a parallel lifecycle.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against de844c19cc1e.

Labels

Label justifications:

  • P2: This repairs a bounded Workshop Suggestions and Doctor recovery defect.
  • merge-risk: 🚨 compatibility: Existing missing-draft proposals gain terminal retirement semantics, and unfinished recovery now prevents dismissal.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The captured body reports compiled UI/Gateway/Doctor behavior, restart persistence, and follow-up recovery through real Inspect and dismissal paths on explicitly identified builds. Those scenarios cover the changed owners; screenshot retrieval and one prior source blob were unavailable to this reviewer, and the earlier builds are not represented as final-head executions.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured body reports compiled UI/Gateway/Doctor behavior, restart persistence, and follow-up recovery through real Inspect and dismissal paths on explicitly identified builds. Those scenarios cover the changed owners; screenshot retrieval and one prior source blob were unavailable to this reviewer, and the earlier builds are not represented as final-head executions.

Evidence

What I checked:

  • Current main still needs the fix: Current main still checks missing drafts only for pending creates using the full bundle reader. Its Doctor relocation entrypoint has no missing-draft retirement argument. The store blob also matches the pinned merge-base store exactly. (src/skills/workshop/service-query.ts:43, de844c19cc1e)
  • Prior dismissal finding resolved: Both Reject and Quarantine reach updateSkillProposalRecord, which now checks for any rollback row in the same transaction before changing a pending proposal's status. The existing service retains its revision check and target lock. (src/skills/workshop/store.ts:393, 137a324dc3b5)
  • Prior listing and transition findings resolved: Listing checks the main draft through the shared reader without validating support files; Inspect still reads the full bundle. The stale-transition caller no longer supplies the removed workspaceDir argument. (src/skills/workshop/service-query.ts:194, 137a324dc3b5)
  • Doctor preserves recovery evidence: Under the existing database coordinator, Doctor retires only pending proposals with missing drafts and no rollback row. Rollback-bearing proposals remain pending with a recovery warning; retirement uses the existing conditional record-and-event transaction. (src/commands/doctor-skill-workshop-sqlite.ts:178, 137a324dc3b5)
  • Captured acceptance and production proof: The supplied full PR body, context sourceRevision a52b3defe04f3511b9ace767128902d48e39f829f9e4c1f299495fd2c59d23a9, records the maintainer-approved recovery contract. It reports compiled UI/Gateway/Doctor observations on 707569a and follow-up recovery observations on 213f433, including restored-draft recovery, blocked dismissal, intact installed content, and restart persistence. It explicitly disclaims Doctor success while the fixture Gateway held its lifecycle lock. (213f43308280)
  • Final-head continuity and inspection limits: Raw tree comparison shows the final commit changes only the agent-tool test and service-query.ts; the test expectation correction was inspected. The previous service-query blob was unavailable, preventing independent verification of the body's claim that its only changes are three explicit undefined returns. GitHub network reads and the screenshot fetch also failed. These are reviewer limitations, not missing contributor proof. The supplied test merge is stale and was not used to infer merge deletions. (src/skills/workshop/service-query.ts, 137a324dc3b5)

Likely related people:

  • Ayaan Zaidi: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (4 earlier review cycles)
  • reviewed 2026-09-07T08:18:14.597Z sha 707569a :: blocked before merge. :: [P2] Update the existing caller to match the narrowed transition input | [P2] Keep damaged update bundles from aborting the proposal list
  • reviewed 2026-09-07T08:41:50.499Z sha 03392ea :: blocked before merge. :: [P1] Preserve unfinished recovery before enabling missing-draft rejection | [P2] Keep damaged update bundles from aborting the proposal list
  • reviewed 2026-09-07T09:07:43.811Z sha e485347 :: blocked before merge. :: [P1] Preserve unfinished recovery before enabling missing-draft rejection | [P2] Keep damaged update bundles from aborting the proposal list
  • reviewed 2026-09-07T09:46:25.437Z sha 213f433 :: blocked before merge. :: none

@obviyus
obviyus marked this pull request as ready for review September 7, 2026 08:59
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. label Sep 7, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 7, 2026
@openclaw-barnacle openclaw-barnacle Bot added the agents Agent runtime and tooling label Sep 7, 2026
@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label Sep 7, 2026
@obviyus

obviyus commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Final-head CI triage for 137a324dc3b5229eb4d77ebae341967d8b7c1523, run 34109260887, attempt 1.

The six failed child jobs are unrelated to this patch. The actual tested merge is 94086770fe68267533f13743ac9df80492a5cbe0, based on main 78cbe2999b3974143d6de8ad2256aad016d02a19. Comparisons below use those immutable commits, not the moving PR base field.

Failed job Evidence and causal boundary
check-lint Telegram progress test has 1012 counted lines against a 1000-line limit. #122221, commit 3612625c4ca8074981c9ddeb47a06d5e92d3e5ef, added 26 counted lines to a 986-line parent. Exact failing file and rule match main; main job 101700454110 independently fails the same rule.
check-npm-lock Lock resolution reported up to date. Installation then hit ECONNRESET downloading Matrix crypto 0.6.6; its pinned installer catches ex but logs undeclared err. Lock validation never ran. Dependency/config inputs match main exactly. #128414 introduced the 0.6.6 pin; this identifies dependency provenance, not the cause of the network reset.
checks-ui (1/3) Six native Mermaid browser cases fail. Renderer, normalization, browser setup/config, and dependency inputs match main exactly. This separate browser project imports no Workshop code or translations; this PR changes no browser-test files. The actual native error was not logged, so no precise exception or introducing-failure PR is claimed. These same cases passed in earlier main job 101654284542; that counterevidence is retained.
checks-node-compact-small-5 Unchanged session-reclamation test passed deletion, archive, and unrelated-session integrity assertions, then observed a 785.6 ms heartbeat gap against its 500 ms threshold. It seeds session data and calls sessions.delete, not Workshop. Its exact test passed in main job 101700454689. No specific resource bottleneck is claimed.
checks-node-compact-small-22 Unchanged update-finalization fixture's one-second budget expired in targetConfigValidation, before its intended completion-cache hang. Doctor/config/plugin operations are stubbed; Workshop migration is not reached. The fixture and owner inputs are identical to main. #140648 introduced this fixture and budget, not a verified new runtime regression.
checks-node-compact-small-29 Unchanged CI-planner inventory-growth test exceeded 120 seconds, with no failed layout assertion. It uses tracked test filenames and committed timing data; this PR only modifies existing files and changes neither planner nor those inputs. #140512 introduced the test; no specific scheduling cause is claimed.

The separate, non-required actionlint job never ran its linter or checkout: GitHub codeload returned HTTP 429 three times for the unchanged action pin introduced by #132921.

No unrelated source, limits, workflow, dependency, or test budgets were changed. This PR's two attributable CI issues were fixed in 137a324. Focused checks and independent compiled behavior validation passed; exact-head ClawSweeper review has zero findings and cleared security. The maintainer-authorized unrelated-CI exception is being recorded against this completed run; CI is not being represented as green.

@obviyus

obviyus commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

landpr-approve-unrelated-red
pr: 141009
head: 137a324
run: 34109260887 attempt: 1
failed: check-lint job=101701888874 conclusion=failure
failed: check-npm-lock job=101701888947 conclusion=failure
failed: checks-node-compact-small-22 job=101701890681 conclusion=failure
failed: checks-node-compact-small-29 job=101701895717 conclusion=failure
failed: checks-node-compact-small-5 job=101701890186 conclusion=failure
failed: checks-ui (1/3) job=101701888607 conclusion=failure
check: openclaw/ci-gate

@obviyus
obviyus merged commit e3e2953 into main Sep 7, 2026
184 of 192 checks passed
@obviyus
obviyus deleted the codex/workshop-stale-suggestions branch September 7, 2026 10:32
@obviyus

obviyus commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Landed as e3e2953f4b9a from reviewed head 137a324dc3b5, with Ayaan's attribution preserved.

Proof: 103 focused owner/controller tests plus seven agent-tool checks passed. Independent compiled CLI/Gateway acceptance passed all four recovery clauses. Original UI/Doctor evidence remains bound to its recorded build; the follow-up build is 213f433, and final 137a324 contains only reviewed return-style/test-message corrections. Exact-head ClawSweeper review passed with zero findings and cleared security.

Before After
Missing drafts remained actionable and ownership moves reset edit dates. Doctor retires unrecoverable drafts without deleting evidence; ownership moves preserve edit dates.
Dismissal could strand interrupted apply recovery. Decisions refuse until recovery finishes; restored drafts recover and normal decisions work.
One damaged support file could break listing. Healthy suggestions stay visible; Inspect and Apply still enforce integrity.

CI exception: CI was not green. The guarded dry run and merge validated the owner-approved exception for completed run 34109260887, attempt 1. Full causal evidence and exact base/merge comparisons were posted before merging.

The separate, non-required actionlint job stopped before checkout after GitHub returned HTTP 429 for the unchanged action pin from #132921. No unrelated code, limits, dependencies, or workflow policies were changed.

The owned test Gateway and hosted Testbox are stopped. This landing does not deploy or run Doctor on production.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 8, 2026
@obviyus
fix(skills): retire missing Workshop drafts safely (openclaw#141009)
0540f2f 
Retire missing-draft suggestions through Doctor without deleting recovery evidence or installed skills. Preserve edit dates during ownership moves.

Keep suggestion listing independent of support-file integrity while retaining full validation for Inspect and Apply. Refuse Reject and Quarantine during unfinished apply recovery, then allow normal decisions after recovery completes.

Verified with focused regression tests and independent compiled CLI, Gateway, and retained UI/Doctor behavior evidence.

Co-authored-by: Ayaan Zaidi <hi@obviy.us>
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

agents Agent runtime and tooling app: web-ui App: web-ui commands Command implementations docs Improvements or additions to documentation gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: L status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@obviyus