Skip to content

perf(skills): detach cached metadata from source strings - #139478

Merged
steipete merged 1 commit into
mainfrom
codex/perf11-skill-metadata
Sep 5, 2026
Merged

perf(skills): detach cached metadata from source strings#139478
steipete merged 1 commit into
mainfrom
codex/perf11-skill-metadata

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Resolves retained skill source bodies in the measured Node workspace-cache workload. Short display titles and frontmatter values can remain sliced strings backed by the full SKILL.md, even after loading deliberately drops content from cached entries. Limiting cache entries alone does not remove that backing storage.

Why This Change Was Made

The shared metadata producers copy their finalized results: src/skills/loading/frontmatter.ts applies native structuredClone to the normalized string record, and src/skills/loading/skill-contract.ts copies the display title through lossless UTF-16LE encoding/decoding. Parser validation/recovery, key order, fallback titles, and every string code unit remain unchanged; no universal string/key-identity guarantee is introduced.

Workspace caching, pinned library selection, and session resource loading share these producers. A cache-only repair would miss siblings; copying headings alone would miss frontmatter. An initial JSON roundtrip passed values but failed the existing prefer-structured-clone lint rule; the final native clone has fresh proof without suppression. Production: +7/-2 lines (net +5); tests/support: +0/-0. Those lines establish metadata ownership without cloning complete entries/files or adding a cache, parser, or fallback.

User Impact

Cached local skill metadata retains fewer source-body strings in the measured Node cohorts while preserving titles, metadata, prompts, pins, refresh behavior, and cache-hit entry identity. Remote skills deliberately retain content and have no claimed memory reduction. Small metadata copies add transient work on cache misses.

Evidence

The real exported workspace loader loads 100 synthetic 240,000-byte files, advances the real watcher snapshot, and loads a second generation under existing limits. Files/results leave scope, files are removed, and three explicit GCs separated by event-loop turns precede heap inspection.

In each independently run large cohort—heading plus metadata, frontmatter without H1, and heading with tiny/reconstructed metadata—the baseline cache payload retains 200 marked source bodies totaling 48,003,200 string self-bytes; the final Node candidate retains zero. Full entries/prompts and cache-hit entry identity match. The 512-byte control preserves outputs; the large-body classifier intentionally does not measure its small strings.

After 65 normal empty-snapshot insertions, neither arm retains a marked body through cache payloads. One baseline body remains engine-global in two cohorts through regexp last-match state. Earlier graph analyses incorrectly labeled that cache ownership; final v3 follows payload edges while excluding prototypes, runtime shapes, functions, and contexts. It preserves all 200 direct retained-entry paths and zero after eviction, without changing the cache or snapshots.

Whole-process heap deltas are noisy as warmed imports release memory; external memory remains 4,983,782 bytes at retained endpoints and ArrayBuffers near 2.31 MB. The attributed result is 200 source bodies → zero in these Node 26.8.1 cohorts, not universal engine/key detachment, Bun heap behavior, zero allocation, latency, RSS, or leak freedom. Bun was not executed.

All 131,390 full value results match, covering every UTF-16 code unit, YAML variants, parser errors, and policy/manifest projections. A real library/session proof preserves six pin/cache/revision/refresh/resource-replacement boundaries with a 900,429-byte fixture; normalized metadata/prompt bytes match. This sibling proof measures behavior, not heap or latency.

node scripts/run-vitest.mjs src/skills/loading/frontmatter.test.ts src/skills/loading/skill-contract.test.ts src/skills/loading/workspace-skill-loader.test.ts src/skills/loading/session.test.ts src/skills/runtime/remote-skills.test.ts src/skills/library/service.test.ts
node node_modules/oxlint/bin/oxlint --type-aware src/skills/loading/frontmatter.ts src/skills/loading/skill-contract.ts

All 84 tests across six files pass before/after. Final formatting, exact-file typed lint, and changed-plan classification pass. Independent P2 review recorded no actionable findings. Broad integration/build checks remain separate; required exact-head hosted CI/native landing gates remain mandatory.

Current-context composition at main 3dff8818ef42872cecc3899b41dffd95942fe70f preserves the upstream pure decodeSkillXml and compactSkillsPromptForContext helpers and every previously reviewed producer statement. Independent static assessment confirms the original clean P2 remains applicable to this producer-copy change. The expected composed skill-contract.ts SHA-256 is bae17b2bf0dd717ab51bb1e9a49634e2e0518c715050fff731fa84da3af3feef. This adds no current-main runtime/heap experiment or Bun heap claim; the original measured Node cache cohorts remain the performance evidence.

@clawsweeper

clawsweeper Bot commented Sep 5, 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 size: XS maintainer Maintainer-authored PR labels Sep 5, 2026
@steipete
steipete marked this pull request as ready for review September 5, 2026 22:48
@clawsweeper clawsweeper Bot added 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. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 6:53 PM ET / 22:53 UTC.

ClawSweeper review

What this changes

Copies parsed skill metadata and display titles so cached metadata does not retain entire skill files in the measured Node workload.

Merge readiness

✅ Ready for maintainer review

This remains a useful, focused performance fix: current main lacks both metadata copies, and the supplied Node measurements support the intended improvement. No actionable introduced defect was found.

Priority: P2
Reviewed head: ed30adad16815664e0dbae5d8418d66b9547551f

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A narrow producer-level repair with relevant reported heap and behavior evidence and no blocking correctness findings.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (live_output): The supplied body reports after-fix measurements through the real workspace loader on Node 26.8.1, showing cache-retained source bodies falling from 200 to zero while outputs and cache-hit identity match; library/session observations cover sibling consumers. The evidence supports the bounded Node claim and was not independently rerun.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The supplied body reports after-fix measurements through the real workspace loader on Node 26.8.1, showing cache-retained source bodies falling from 200 to zero while outputs and cache-hit identity match; library/session observations cover sibling consumers. The evidence supports the bounded Node claim and was not independently rerun.
Evidence reviewed 8 items Verified introduced patch: The pinned merge-base-to-head diff changes only the frontmatter return and display-title return. Parser validation, title selection, and fallback behavior are unchanged.
Plain-string parser boundary: The owned Markdown parser normalizes frontmatter into an ordinary Record<string, string>, including serialization of structured YAML values. The clone occurs after error handling and introduces no new parser or authorization behavior.
Cache ownership and sibling consumers: The local root loader deliberately retains skill and frontmatter fields without content. Workspace cache hits reuse entries; library selection and session loading use the same changed producers. This supports repairing the producers instead of adding a cache-specific workaround.
Findings None None.
Security None None.

How this fits together

OpenClaw loads skill files into metadata used by workspace caches, pinned library selections, and session resources. These metadata producers feed skill discovery and prompt preparation after local file contents are discarded.

flowchart TD
  A[Skill files] --> B[Parse frontmatter]
  A --> C[Resolve display title]
  B --> D[Copy metadata strings]
  C --> D
  D --> E[Workspace metadata cache]
  D --> F[Library and session resources]
  E --> G[Skill discovery and prompts]
  F --> G
Loading

Before merge

None.

Agent review details

Security

None.

PR surface

Source +5. Total +5 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 2 7 2 +5
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 7 2 +5

Review metrics

Metric Value Why it matters
Reported cache retention 200 source bodies → 0 in each of 3 Node cohorts The measurements target retained cache payloads rather than treating noisy whole-process memory deltas as proof.

Technical review

Best possible solution:

Keep metadata ownership at the shared producers while preserving string values, cache identity, and existing refresh behavior.

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

Yes for the reported Node workload: the body describes repeated real-loader runs and retained-object measurements, and source inspection confirms the ownership path. This read-only review did not independently reproduce the heap result on current main.

Is this the best way to solve the issue?

Yes. Copying the finalized string metadata at its shared producers covers workspace, library, and session consumers without changing cache limits, parsing policy, or persisted data.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This addresses measured excess memory retention in skill metadata caches without evidence of an urgent production outage.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The supplied body reports after-fix measurements through the real workspace loader on Node 26.8.1, showing cache-retained source bodies falling from 200 to zero while outputs and cache-hit identity match; library/session observations cover sibling consumers. The evidence supports the bounded Node claim and was not independently rerun.
  • 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 (live_output): The supplied body reports after-fix measurements through the real workspace loader on Node 26.8.1, showing cache-retained source bodies falling from 200 to zero while outputs and cache-hit identity match; library/session observations cover sibling consumers. The evidence supports the bounded Node claim and was not independently rerun.

Label justifications:

  • P2: This addresses measured excess memory retention in skill metadata caches without evidence of an urgent production outage.
  • 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 supplied body reports after-fix measurements through the real workspace loader on Node 26.8.1, showing cache-retained source bodies falling from 200 to zero while outputs and cache-hit identity match; library/session observations cover sibling consumers. The evidence supports the bounded Node claim and was not independently rerun.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied body reports after-fix measurements through the real workspace loader on Node 26.8.1, showing cache-retained source bodies falling from 200 to zero while outputs and cache-hit identity match; library/session observations cover sibling consumers. The evidence supports the bounded Node claim and was not independently rerun.

Evidence

What I checked:

  • Verified introduced patch: The pinned merge-base-to-head diff changes only the frontmatter return and display-title return. Parser validation, title selection, and fallback behavior are unchanged. (src/skills/loading/frontmatter.ts:33, ed30adad1681)
  • Plain-string parser boundary: The owned Markdown parser normalizes frontmatter into an ordinary Record<string, string>, including serialization of structured YAML values. The clone occurs after error handling and introduces no new parser or authorization behavior. (packages/markdown-core/src/frontmatter.ts:4, ed30adad1681)
  • Cache ownership and sibling consumers: The local root loader deliberately retains skill and frontmatter fields without content. Workspace cache hits reuse entries; library selection and session loading use the same changed producers. This supports repairing the producers instead of adding a cache-specific workaround. (src/skills/loading/skill-root-loader.ts:62, ed30adad1681)
  • Still absent from current main: Fetched main returns parsed.frontmatter directly and also returns the selected heading directly; both proposed copies remain absent. No verified merged replacement was supplied or established. (src/skills/loading/frontmatter.ts:31, 7528edb1ba5c)
  • Release inspection: The v2026.9.2 frontmatter source still returns the parser record directly. The release display-title blob could not be retrieved, so complete release coverage was not established; no shipped-fix claim is made. (src/skills/loading/frontmatter.ts:31, 3928bad9badf)
  • Reported production-loader measurements: The complete supplied PR body, captured under sourceRevision b808ce371f91869bef9714ac3aacce17cbef508601e9e1a5386112384ccb0890, reports real workspace-loader runs on Node 26.8.1: two generations of 100 large skill files retained 200 marked bodies before the fix and zero afterward in three cohorts. It also reports equal metadata/prompts, cache-hit identity, 131,390 value comparisons, and six library/session lifecycle boundaries. These are contributor-reported observations, not reviewer-executed experiments; Bun, latency, and RSS benefits are explicitly unclaimed. (ed30adad1681)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Patrick Erichsen: 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.

@steipete
steipete merged commit 266e8da into main Sep 5, 2026
248 checks passed
@steipete
steipete deleted the codex/perf11-skill-metadata branch September 5, 2026 23:26
@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Sep 5, 2026
Open
vincentkoc added a commit that referenced this pull request Sep 5, 2026
@vincentkoc
chore(ci): merge current main for validation
ebb28f7 
* origin/main: (95 commits)
  perf(build): avoid unrelated gateway artifact scans (#127757)
  fix(update): deliver terminal progress after long updates (#139289)
  fix(ui): show session hover cards for local chat URLs (#139384)
  perf(ui): reuse unchanged transcript key snapshots (#139400)
  fix(ui): make subagent sessions view-only (#139381)
  fix: open agent state on SQLite builds without extensions (#139487)
  perf(skills): detach cached metadata from source strings (#139478)
  fix(ui): show primary user in multi-agent profile hero (#136736)
  perf(sessions): stop decoding the session row four times per SQLite entry patch (#136361)
  refactor(exec): share package-manager option scans (#139482)
  fix(ci): authorize frozen mobile release candidates (#139441)
  fix: avoid session deletion slowdown in large session stores (#139467)
  perf(gateway): reuse node invocation idle timers (#139354)
  fix(auth): preserve selected profiles across plugin scopes (#139413)
  fix(discord): retain voice input until transcription completes (#139484)
  fix(agents): name Gateway storage failures in run-failure copy (#139409)
  fix: keep subagent checks responsive in large session stores (#139450)
  fix(memory): index notes when optional embeddings cannot start (#139463)
  fix: refresh pending approvals when reopening chat (#139457)
  fix(tooling): preserve Git hook choices during install (#139461)
  ...
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Sep 6, 2026
@steipete
perf(skills): detach cached metadata from source strings (openclaw#13…
82cf5ec 
…9478)
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

maintainer Maintainer-authored PR 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: XS 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

@steipete