fix(skills): clear workspaceVersions entry when skills watcher is disabled by fede-kamel · Pull Request #77998 · openclaw/openclaw

fede-kamel · 2026-05-05T18:11:18Z

Summary

Bounds skills watcher and snapshot-version state to the active workspace set on the current src/skills/runtime implementation.

Adds clearSkillsSnapshotVersionForWorkspace() and uses it from watcher teardown while preserving pending invalidation by promoting the workspace version into globalVersion before deleting the workspace key.
Keeps future workspace-local bumps monotonic against both workspace and global counters so same-millisecond or clock-backward changes cannot be swallowed after cleanup.
Adds a shared disposeWorkspaceWatchState() path for explicit watch: false teardown and idle eviction. Disposal unsubscribes the workspace from shared path watchers, clears target/cache/last-ensure state, marks the workspace dirty, then clears the per-workspace version key.
Adds opportunistic idle eviction for workspace subscriptions not re-ensured for 1 hour. Shared path watchers remain alive while another workspace still subscribes.
Removes the old CHANGELOG.md edit from the PR; release generation owns changelog updates.

Verification

node scripts/run-vitest.mjs src/skills/runtime/refresh.test.ts src/skills/runtime/session-snapshot.test.ts -> 34/34 pass.
pnpm exec oxfmt --check --threads=1 src/skills/runtime/refresh.ts src/skills/runtime/refresh-state.ts src/skills/runtime/refresh.test.ts -> pass.
.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main -> clean, no accepted/actionable findings.
AWS Crabbox changed gate on fresh PR checkout: run_5057e6eaab4f, provider aws, lease cbx_4afdccef9561, command env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed -> pass.
AWS Crabbox live filesystem/chokidar E2E on fresh PR checkout: run_512d4b6c1717, provider aws, lease cbx_4afdccef9561 -> pass.
GitHub PR checks for head 67bd2df6c131ff12fc12ab65ebdc63e2c6898193 -> green; gh pr checks 77998 --watch=false returned all relevant checks passing/skipped.

Real behavior proof

Behavior addressed: workspaceVersions retained workspace entries forever, and current src/skills/runtime/refresh.ts retained per-workspace watch target/cache/subscriber state after sessions stopped touching a workspace. A long-lived gateway that sees many distinct workspaces can accumulate stale workspace snapshot-version keys and shared watcher subscriber bookkeeping.
Real environment tested: AWS Crabbox fresh checkout of this PR (run_512d4b6c1717) on Linux with Node 24.15.0. The proof used real temp workspaces, real chokidar watchers, real SKILL.md writes, and the public runtime APIs from src/skills/runtime/refresh.ts.
Exact steps or command run after this patch: uploaded and ran a tsx script via Crabbox that created temp workspaces, called ensureSkillsWatcher(), wrote SKILL.md files, waited for real watch events, disabled watching, simulated idle TTL by controlling Date.now() around ensureSkillsWatcher(), re-enabled a workspace, and checked getSkillsSnapshotVersion() / shouldRefreshSnapshotForVersion() after each lifecycle transition.
Evidence after fix:

{"result":"PASS","watchedVersion":1780252438684,"disabledVersion":1780252438735,"followupVersion":1780252438736,"idleVersion":1780252440024,"evictedVersion":1780256040087,"reenabledVersion":1780256040087}

Observed result after fix: a real watch event advanced the watched workspace version; explicit watch disable marked the snapshot dirty and kept follow-up workspace bumps monotonic; disabled workspaces stopped emitting file-watch events; idle eviction marked the stale workspace dirty; active workspace watching still worked after eviction; re-enabling the idle workspace kept the next-turn refresh decision true.
What was not tested: waiting a real wall-clock hour for the idle TTL. The E2E used real file I/O and chokidar events but simulated the TTL clock edge with Date.now() so the lifecycle path could be exercised without a one-hour test.

clawsweeper · 2026-05-05T18:16:17Z

Codex review: needs maintainer review before merge. Reviewed May 31, 2026, 2:32 PM ET / 18:32 UTC.

Summary
This PR clears per-workspace skills snapshot version state during watcher teardown, evicts idle skills watcher subscriptions after one hour, and adds focused regression tests.

PR surface: Source +43, Tests +94. Total +137 across 3 files.

Reproducibility: yes. source-reproducible: current main keeps workspaceVersions and workspace watcher subscriptions without a production per-workspace clear or idle eviction path, while session turns call the watcher before reading snapshot versions. I did not run tests because this review was constrained to read-only inspection.

Review metrics: none identified.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
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:

Run or confirm exact-head skills runtime test and formatter/check results before landing.

Risk before merge

[P1] The diff deliberately changes skills snapshot version promotion and idle watcher disposal; the provided proof covers the deterministic Map path and mocked watcher lifecycle, not a long-lived real chokidar gateway run.

Maintainer options:

Land After Exact-Head Checks (recommended)
Accept the bounded session-state risk once the exact PR head has required skills runtime tests and formatting/checks green, because the current diff preserves monotonic invalidation and has no blocking findings.
Ask For A Gateway Soak
If maintainers want extra confidence, request a short long-lived gateway/chokidar smoke that shows idle watcher eviction closes subscriptions without stale skills snapshots.

Next step before merge

No automated repair is needed; remaining work is normal exact-head validation and maintainer merge handling.

Security
Cleared: No concrete security or supply-chain concern found; the diff only changes in-process skills watcher state and colocated tests, with no dependency, CI, secret, network, or package-resolution changes.

Review details

Best possible solution:

Keep this focused runtime cleanup and land it after exact-head checks, with watcher lifecycle ownership staying in src/skills/runtime and no new config surface.

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

Yes, source-reproducible: current main keeps workspaceVersions and workspace watcher subscriptions without a production per-workspace clear or idle eviction path, while session turns call the watcher before reading snapshot versions. I did not run tests because this review was constrained to read-only inspection.

Is this the best way to solve the issue?

Yes. The PR fixes the owner runtime path, preserves monotonic snapshot invalidation, and adds focused tests; a session-end hook would be tighter, but the established current entry point is per-turn ensureSkillsWatcher.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit 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 provides sufficient terminal proof for this Map lifecycle change, showing unpatched 5,000 workspace entries retained and patched cleanup returning them to zero, with tests covering watcher teardown and idle eviction.
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: The PR fixes a bounded in-process skills watcher/session snapshot lifecycle leak without evidence of emergency data loss, security bypass, or unusable runtime.
merge-risk: 🚨 session-state: The patch changes skills snapshot version promotion and watcher disposal, where a regression could leave sessions using stale or over-refreshed skill snapshots.
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 (terminal): The PR body provides sufficient terminal proof for this Map lifecycle change, showing unpatched 5,000 workspace entries retained and patched cleanup returning them to zero, with tests covering watcher teardown and idle eviction.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides sufficient terminal proof for this Map lifecycle change, showing unpatched 5,000 workspace entries retained and patched cleanup returning them to zero, with tests covering watcher teardown and idle eviction.

Evidence reviewed

PR surface:

Source +43, Tests +94. Total +137 across 3 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	2	51	8	+43
Tests	1	94	0	+94
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	3	145	8	+137

What I checked:

Repository policy read: Root AGENTS.md was read fully and its OpenClaw PR review requirements guided the source, caller, test, proof, and history checks. (AGENTS.md:1, 01ef169004bc)
Current main still has retained per-workspace version state: Current main keeps workspaceVersions as a module-level Map and only clears it in test reset; there is no production per-workspace clear helper. (src/skills/runtime/refresh-state.ts:8, 01ef169004bc)
Current main watcher teardown is limited: Current main unsubscribes watcher targets only when skills.load.watch is false and does not track last-use time or evict abandoned workspace subscriptions. (src/skills/runtime/refresh.ts:535, 01ef169004bc)
Session caller proves user-facing cache impact: Session snapshot resolution calls ensureSkillsWatcher before reading the snapshot version, so watcher lifecycle and version promotion directly affect whether persisted skills snapshots refresh on later turns. (src/skills/runtime/session-snapshot.ts:58, 01ef169004bc)
PR head preserves monotonic versioning while clearing entries: The PR head bumps workspace versions from Math.max(globalVersion, local) and promotes a removed local version into globalVersion before deleting the workspace key, which addresses the prior stale-snapshot suppression concern. (src/skills/runtime/refresh-state.ts:46, 67bd2df6c131)
PR head owns cleanup in the skills watcher runtime: The PR adds workspaceWatchLastEnsuredAt, centralizes disposal through disposeWorkspaceWatchState, bumps dirty state before clearing, and evicts idle subscriptions during later ensureSkillsWatcher calls. (src/skills/runtime/refresh.ts:63, 67bd2df6c131)

Likely related people:

steipete: Current-main blame in this checkout points the skills refresh runtime to Peter Steinberger, and the latest PR repair commits on the same files are authored/committed by the same maintainer handle. (role: recent area contributor and branch repair author; confidence: high; commits: 6f2fbaaaf86d, c1c0dbe1d13d, 67bd2df6c131; files: src/skills/runtime/refresh.ts, src/skills/runtime/refresh-state.ts, src/skills/runtime/refresh.test.ts)
fede-kamel: The initial PR commits for this skills cleanup were authored by Federico Kamelhar, and the PR body ties the work to the same memory-leak audit series as the recently merged Discord cache fix. (role: source fix contributor and related audit contributor; confidence: medium; commits: a6cf88b57700, 9e5563a9e43e; files: src/skills/runtime/refresh.ts, src/skills/runtime/refresh-state.ts, src/skills/runtime/refresh.test.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.

fede-kamel · 2026-05-06T22:19:05Z

Rebased onto current origin/main (was 372 commits behind). The previous "Real behavior proof" CI failure was a missing-script artifact — scripts/github/real-behavior-proof-check.mjs was added in #77622 after this branch was first pushed, so the workflow could not load on the old base. The script is on HEAD now and CI is re-running.

@vincentkoc — flagging for review when convenient.

openclaw-barnacle · 2026-05-31T05:03:05Z

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

…abled refresh-state.ts kept a module-level Map<workspaceDir, number> populated on every skills file event, but had no production removal path. The sibling watchers Map in refresh.ts cleans up on watcher teardown (watchEnabled=false config change), but workspaceVersions was forgotten. Add clearSkillsSnapshotVersionForWorkspace and call it from the existing watcher-disable teardown path so workspaceVersions follows watchers' lifecycle. Removal is safe: subsequent getSkillsSnapshotVersion reads fall back to globalVersion, and shouldRefreshSnapshotForVersion forces a refresh whenever a consumer was holding a non-zero cached version. Note: this is the lifecycle-gap fix on the existing teardown path; the broader fact that ensureSkillsWatcher itself never tears down on session-end is a separate concern not addressed here.

Add an opportunistic idle sweep to ensureSkillsWatcher: every call walks watchers and disposes those with lastEnsuredAt < now - 1h, closing the chokidar instance, clearing the debounce timer, and clearing the matching workspaceVersions entry. Sessions re-call ensureSkillsWatcher on every turn, so any workspace with an active session refreshes its lastEnsuredAt and stays attached. Only abandoned watchers (workspaces no longer touched by any session for \>=1h) get torn down. This bounds watchers.size to the working set instead of growing one entry per distinct workspaceDir over the gateway lifetime. The previous narrow fix on the watchEnabled=false teardown path is also unified through the new disposeSkillsWatcher helper so version-entry cleanup is consistent across all teardown sites.

steipete · 2026-05-31T18:35:24Z

Land-ready after maintainer rewrite/rebase.

What changed:

Rebased the PR onto current origin/main and moved the fix from the deleted src/agents/skills/* paths to current src/skills/runtime/*.
Kept shared path watchers as the owner boundary; the fix now evicts idle workspace subscriptions instead of reverting to one watcher per workspace.
Watcher disposal now marks the workspace dirty before clearing per-workspace version state, and workspace-local bumps are monotonic against both local and global version counters so cleanup cannot swallow a pending refresh.
Removed the release-owned CHANGELOG.md edit from the PR branch.

Verification:

node scripts/run-vitest.mjs src/skills/runtime/refresh.test.ts src/skills/runtime/session-snapshot.test.ts -> 34/34 pass.
pnpm exec oxfmt --check --threads=1 src/skills/runtime/refresh.ts src/skills/runtime/refresh-state.ts src/skills/runtime/refresh.test.ts -> pass.
.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main -> clean, no accepted/actionable findings.
AWS Crabbox changed gate: run_5057e6eaab4f, provider aws, lease cbx_4afdccef9561, command env OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed -> pass.
AWS Crabbox live filesystem/chokidar E2E: run_512d4b6c1717, provider aws, lease cbx_4afdccef9561 -> pass with real temp workspaces, real SKILL.md writes, watch-disable, idle-eviction, and re-enable checks.
gh pr checks 77998 --watch=false -> green for head 67bd2df6c131ff12fc12ab65ebdc63e2c6898193.

Known proof gap: the live E2E simulates the 1-hour TTL clock edge with Date.now() instead of waiting a real wall-clock hour; filesystem events and chokidar behavior were real.

openclaw-barnacle Bot added triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. agents Agent runtime and tooling size: XS and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 5, 2026

clawsweeper Bot mentioned this pull request May 5, 2026

Bug: skills refresh-state workspaceVersions map retains entries after watcher teardown #77997

Closed

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

fede-kamel force-pushed the security/skills-workspace-versions-leak branch from 376a9e0 to 3434053 Compare May 6, 2026 21:57

openclaw-barnacle Bot added the proof: supplied External PR includes structured after-fix real behavior proof. label May 6, 2026

clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 6, 2026

openclaw-barnacle Bot added the stale Marked as stale due to inactivity label May 31, 2026

steipete self-assigned this May 31, 2026

fede-kamel and others added 5 commits May 31, 2026 14:22

docs(skills): explain watcher idle eviction

b896b5d

fix(skills): refresh after watcher disposal

c1c0dbe

fix(skills): keep workspace version bumps monotonic

67bd2df

steipete force-pushed the security/skills-workspace-versions-leak branch from 3434053 to 67bd2df Compare May 31, 2026 18:25

openclaw-barnacle Bot removed the agents Agent runtime and tooling label May 31, 2026

clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels May 31, 2026

clawsweeper Bot added status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels May 31, 2026

openclaw-barnacle Bot removed the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 31, 2026

steipete merged commit e76df69 into openclaw:main May 31, 2026
177 of 184 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(skills): clear workspaceVersions entry when skills watcher is disabled#77998

fix(skills): clear workspaceVersions entry when skills watcher is disabled#77998
steipete merged 5 commits into
openclaw:mainfrom
fede-kamel:security/skills-workspace-versions-leak

fede-kamel commented May 5, 2026 •

edited by steipete

Loading

Uh oh!

clawsweeper Bot commented May 5, 2026 •

edited

Loading

Uh oh!

fede-kamel commented May 6, 2026

Uh oh!

openclaw-barnacle Bot commented May 31, 2026

Uh oh!

steipete commented May 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

fede-kamel commented May 5, 2026 • edited by steipete Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Real behavior proof

Uh oh!

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

Uh oh!

Uh oh!

fede-kamel commented May 6, 2026

Uh oh!

openclaw-barnacle Bot commented May 31, 2026

Uh oh!

steipete commented May 31, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fede-kamel commented May 5, 2026 •

edited by steipete

Loading

clawsweeper Bot commented May 5, 2026 •

edited

Loading