Skip to content

fix(plugins): cache installed manifest fingerprints#86797

Open
galiniliev wants to merge 1 commit into
openclaw:mainfrom
galiniliev:bug-044-gateway-stable-metadata-probes
Open

fix(plugins): cache installed manifest fingerprints#86797
galiniliev wants to merge 1 commit into
openclaw:mainfrom
galiniliev:bug-044-gateway-stable-metadata-probes

Conversation

@galiniliev

@galiniliev galiniliev commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

What problem does this PR solve?

  • Gateway plugin metadata hot paths repeatedly resolve and stat stable installed manifest/package metadata when fingerprinting the same installed plugin index object.

Why does this matter now?

  • Redacted profiler evidence showed this path on gateway hot workloads.

What is the intended outcome?

  • Reduce repeated hot-path work while preserving current behavior contracts.

What is intentionally out of scope?

  • Broader profiler reruns and unrelated perf stacks are out of scope for this PR.

What does success look like?

  • Focused regression tests pass and the changed path avoids the repeated work described in the linked issue.

What should reviewers focus on?

  • Whether the cache/lazy path preserves existing contracts and invalidation boundaries.

Linked context

Which issue does this close?

Closes #86791

Which issues, PRs, or discussions are related?

Related #

Was this requested by a maintainer or owner?

Local performance bug extraction from redacted profiler evidence.

Real behavior proof (required for external PRs)

Behavior addressed: Gateway plugin metadata hot paths repeatedly resolve and stat stable installed manifest/package metadata when fingerprinting the same installed plugin index object.

Real environment tested: Local OpenClaw source worktree on Linux with Node v24.15.0; focused Vitest regression tests and diff hygiene were run in the isolated bug branch worktree.

Exact steps or command run after this patch:

node node_modules/vitest/vitest.mjs run src/plugins/manifest-registry-installed.fingerprint.test.ts --reporter=verbose
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode local

Evidence after fix:

node node_modules/vitest/vitest.mjs run src/plugins/manifest-registry-installed.fingerprint.test.ts --reporter=verbose

Result: 1 file passed, 1 test passed.

git diff --check

Result: passed.

.agents/skills/autoreview/scripts/autoreview --mode local

Result: clean, no accepted/actionable findings reported.

Observed result after fix: Focused regression coverage passed and autoreview reported no accepted/actionable findings.

What was not tested: No live long-running profiler rerun was performed after the patch. Other filesystem chains from session, transcript, Telegram spool, and subagent paths remain out of scope for this narrow PR.

Proof limitations or environment constraints: The original evidence is from redacted local profiler artifacts; this PR includes focused seam proof rather than a full live profiler replay.

Before evidence (optional but encouraged):

Profile snippets showed lstat and realpathSync under resolvePackageJsonPath -> buildInstalledManifestRegistryIndexKey -> resolveInstalledManifestRegistryIndexFingerprint -> resolvePluginControlPlaneContext -> getCurrentPluginMetadataSnapshot.

Tests and validation

Which commands did you run?

node node_modules/vitest/vitest.mjs run src/plugins/manifest-registry-installed.fingerprint.test.ts --reporter=verbose
git diff --check
.agents/skills/autoreview/scripts/autoreview --mode local

What regression coverage was added or updated?

src/plugins/manifest-registry-installed.ts; src/plugins/manifest-registry-installed.fingerprint.test.ts

What failed before this fix, if known?

The profiler/timeline evidence showed the hot-path behavior described in the linked issue.

If no test was added, why not?

Focused regression coverage was added or updated.

Risk checklist

Did user-visible behavior change? (Yes/No)

No

Did config, environment, or migration behavior change? (Yes/No)

No

Did security, auth, secrets, network, or tool execution behavior change? (Yes/No)

No

What is the highest-risk area?

Preserving existing hot-path behavior while reducing repeated work.

How is that risk mitigated?

Focused regression tests plus autoreview.

Current review state

What is the next action?

Maintainer review and CI.

What is still waiting on author, maintainer, CI, or external proof?

Full live profiler replay was not run locally.

Which bot or reviewer comments were addressed?

Local autoreview findings were addressed before opening this PR where applicable.

@openclaw-barnacle openclaw-barnacle Bot added size: S maintainer Maintainer-authored PR labels May 26, 2026
@clawsweeper

clawsweeper Bot commented May 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 10:09 AM ET / 14:09 UTC.

Summary
The branch adds a WeakMap cache for installed manifest registry fingerprints and a focused Vitest asserting repeated calls for the same InstalledPluginIndex object avoid restatting files.

PR surface: Source +8, Tests +66. Total +74 across 2 files.

Reproducibility: yes. for the PR-branch defect from source inspection: call the submitted fingerprint function once, mutate the same index object or change a manifest/package file when signatures are absent, and a later call can return the old hash. I did not replay the original long-running gateway profiler workload.

Review metrics: 1 noteworthy metric.

  • Fingerprint Cache Guard: 1 unconditional write, 0 cacheability guards in PR diff. The write guard determines whether plugin metadata snapshot state can go stale after index or file changes.

Stored data model
Persistent data-model change detected: persistent cache schema: src/plugins/manifest-registry-installed.fingerprint.test.ts, serialized state: src/plugins/manifest-registry-installed.fingerprint.test.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #86791
Summary: This PR is the open but stale candidate for the canonical installed manifest fingerprint hot-path issue; current main already has safer guarded cache semantics, and adjacent realpath-cache work landed separately.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • Refresh the branch onto current main and preserve the guarded cacheability checks for mutable and live-file-backed indexes.
  • [P1] Add redacted gateway, profiler, runtime log, terminal output, or linked artifact proof showing repeated filesystem probes are reduced after the patch.
  • Redact private details such as IP addresses, API keys, phone numbers, non-public endpoints, and private local paths before posting proof.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body only reports focused Vitest, diff check, and autoreview output; add redacted gateway, profiler, runtime log, terminal output, or a linked artifact showing reduced filesystem probes after the patch, then update the PR body for re-review.

Risk before merge

  • [P1] Merging this branch as submitted can reuse stale installed-index fingerprints after same-object mutation or manifest/package file changes when persisted signatures are absent.
  • [P1] The branch is dirty against current main, which already contains the safer guarded implementation and broader invalidation coverage.
  • [P1] The PR body reports focused tests and autoreview output, but it does not show redacted gateway, runtime, profiler, terminal, log, or linked artifact proof that the hot-path filesystem probes are reduced in a real setup.

Maintainer options:

  1. Keep Current Main Canonical (recommended)
    A maintainer can close this protected PR as superseded by the guarded shipped implementation after confirming the linked issue state.
  2. Refresh Onto Guarded Semantics
    If the branch remains useful, rebase it and preserve the current-main cacheability guard plus invalidation tests before review continues.
  3. Accept Source-Level Proof Only
    Maintainers may explicitly override the external proof gap, but they should record what validation replaces missing gateway or profiler evidence.

Next step before merge

  • [P1] Protected maintainer labeling, current-main/release overlap, a stale-cache correctness finding, dirty merge state, and contributor-owned proof needs make this a maintainer resolution item rather than an automated repair lane.

Security
Cleared: The diff only changes plugin metadata fingerprint caching and a focused test; I found no concrete dependency, workflow, secret, or supply-chain concern.

Review findings

  • [P1] Guard the fingerprint cache write — src/plugins/manifest-registry-installed.ts:126
Review details

Best possible solution:

Keep the guarded implementation as canonical; if maintainers want this PR to continue, refresh it to preserve those guards and add real proof, otherwise close it manually as superseded.

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

Yes for the PR-branch defect from source inspection: call the submitted fingerprint function once, mutate the same index object or change a manifest/package file when signatures are absent, and a later call can return the old hash. I did not replay the original long-running gateway profiler workload.

Is this the best way to solve the issue?

No: the submitted unconditional object-identity cache is broader than the fingerprint invalidation contract. The current-main guarded cache is the narrower maintainable fix because it only memoizes deeply frozen indexes with persisted file signatures.

Full review comments:

  • [P1] Guard the fingerprint cache write — src/plugins/manifest-registry-installed.ts:126
    This stores a WeakMap entry for every InstalledPluginIndex object. The fingerprint can include mutable record fields and live manifest/package file signatures when persisted signatures are absent, so later same-object mutations or file changes can return the old hash and let plugin metadata snapshot state be reused incorrectly. Preserve the current-main guard that only caches deeply frozen indexes with persisted file signatures.
    Confidence: 0.93

Overall correctness: patch is incorrect
Overall confidence: 0.93

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against be94853de0d0.

Label changes

Label justifications:

  • P2: This is a bounded plugin metadata performance/correctness PR with limited blast radius and no reported outage, data loss, or security bypass.
  • merge-risk: 🚨 session-state: A stale installed-index fingerprint can make plugin metadata snapshot compatibility reuse state after the underlying index or file state changes.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body only reports focused Vitest, diff check, and autoreview output; add redacted gateway, profiler, runtime log, terminal output, or a linked artifact showing reduced filesystem probes after the patch, then update the PR body for re-review.
Evidence reviewed

PR surface:

Source +8, Tests +66. Total +74 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 9 1 +8
Tests 1 66 0 +66
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 75 1 +74

What I checked:

  • Repository policy applied: Read the full root policy and scoped plugin policy; the relevant guidance requires whole-path PR review and allows plugin metadata caches only when lifecycle-owned and compatible with stable metadata boundaries. (AGENTS.md:20, be94853de0d0)
  • Submitted branch caches unconditionally: The PR diff writes every InstalledPluginIndex fingerprint into a WeakMap without checking immutability or persisted file signatures. (src/plugins/manifest-registry-installed.ts:126, 8fb15dd60b21)
  • Current main guards the cache write: Current main computes the fingerprint first, then writes the WeakMap only when isInstalledManifestRegistryIndexFingerprintCacheable(index) passes. (src/plugins/manifest-registry-installed.ts:278, be94853de0d0)
  • Current tests preserve invalidation boundaries: Current tests require recomputation for mutable same-object indexes, shallow-frozen nested mutation, and frozen indexes that depend on live file state. (src/plugins/manifest-registry-installed.test.ts:186, be94853de0d0)
  • Caller impact reaches metadata snapshot reuse: Plugin metadata snapshot memo state and memo keys call resolveInstalledManifestRegistryIndexFingerprint, so a stale fingerprint can make snapshot state look reusable after the underlying index or file state changes. (src/plugins/plugin-metadata-snapshot.ts:308, be94853de0d0)
  • Release contains guarded implementation: The v2026.6.10 tag contains the guarded cache write, so the safer implementation is already shipped in the latest release provided for this review. (src/plugins/manifest-registry-installed.ts:256, aa69b12d0086)

Likely related people:

  • steipete: GitHub commit metadata shows steipete authored the guarded stable plugin index fingerprint cache and adjacent installed package path cache work in the central files. (role: introduced current guarded cache; confidence: high; commits: 2babe03bf521, f927e532da24; files: src/plugins/manifest-registry-installed.ts, src/plugins/manifest-registry-installed.test.ts)
  • obuchowski: Recent history shows plugin metadata snapshot performance work on the caller surface that consumes installed-index fingerprints. (role: adjacent hot-path contributor; confidence: medium; commits: 16ea3f272fe5; files: src/plugins/plugin-metadata-snapshot.ts)
  • vincentkoc: Recent merged work preserved the adjacent installed manifest realpath cache optimization and touched the same plugin manifest registry area. (role: recent adjacent contributor; confidence: medium; commits: 94ae918d8f40, 8246e8dace5f; files: src/plugins/manifest-registry-installed.ts, src/plugins/manifest-registry-installed.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.
Review history (1 earlier review cycle)
  • reviewed 2026-06-24T18:29:45.193Z sha 8fb15dd :: needs real behavior proof before merge. :: [P1] Guard the fingerprint cache write

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels May 26, 2026
@clawsweeper

clawsweeper Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat.

Where did the egg go?
  • The egg game starts only after the PR passes the real-behavior proof check.
  • Before that, no creature or rarity is rolled. The treat waits for real proof.
  • This is still just collectible flavor: proof affects review readiness, not creature quality.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels May 29, 2026
@galiniliev galiniliev self-assigned this May 30, 2026
@galiniliev

galiniliev commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

Manual review fallback for PR #86797 (fix(plugins): cache installed manifest fingerprints)

The requested openclaw-pr-maintainer skill is not installed in this Codex session, so I performed the requested manual deep review of the PR diff.

Intent

The PR adds an object-identity WeakMap<InstalledPluginIndex, string> cache around resolveInstalledManifestRegistryIndexFingerprint() to avoid repeating filesystem work (statSync/realpath resolution) when gateway metadata hot paths fingerprint the same installed plugin index object multiple times. It also adds a focused Vitest that asserts repeated calls for the same index object do not re-stat manifest/package files.

Findings

Blocking: the WeakMap cache breaks existing fingerprint invalidation semantics

src/plugins/manifest-registry-installed.ts:116-127

The new cache keys only by the InstalledPluginIndex object identity and returns the first computed fingerprint forever for that object. That is broader than the current contract of resolveInstalledManifestRegistryIndexFingerprint(): the fingerprint is derived from mutable index fields and, when persisted file signatures are absent, from live manifest/package file signatures via safeFileSignature().

Existing tests in src/plugins/manifest-registry-installed.test.ts document those invalidation boundaries:

  • recomputes installed-index fingerprints for mutable index objects mutates record.manifestHash on the same index object and expects a different fingerprint.
  • does not cache shallow-frozen installed-index fingerprints with mutable nested records mutates a nested startup.agentHarnesses array on the same top-level frozen index and expects a different fingerprint.
  • does not cache frozen installed-index fingerprints that depend on live file state updates a manifest file mtime for a frozen index without persisted signatures and expects a different fingerprint.
  • reconstructs installed-index manifest registries when manifest files change depends on file-state fingerprint changes to rebuild registry state.

With this PR, all of those same-object changes are hidden after the first call. That can cause plugin metadata snapshots and registry memo keys to keep using stale fingerprints, because callers such as plugin-metadata-snapshot.ts compare and memoize indexes through resolveInstalledManifestRegistryIndexFingerprint(). The performance optimization is valid only for inputs that are proven immutable and independent of live file signatures, or if the cache key also includes a version/signature component that changes when the fingerprint inputs change.

Recommended fix: do not cache every InstalledPluginIndex by object identity. Either keep recomputing, cache only a known-safe normalized/deep-frozen index whose records contain persisted fileSignature values, or split the hot path so the gateway can reuse a precomputed fingerprint at the point where immutability and file-signature stability are guaranteed.

Per-file Risk Review

src/plugins/manifest-registry-installed.ts

  • Correctness: high risk. The new cache changes resolveInstalledManifestRegistryIndexFingerprint() from a function of current index/file state into a function of first-observed object identity. This breaks mutable-index and live-file invalidation behavior.
  • Error handling: unchanged directly, but stale cache entries can mask safeFileSignature() transitions between existing and missing files after the first call.
  • Concurrency: JavaScript execution is single-threaded here, but asynchronous gateway flows can interleave metadata lookups with index or file updates. The cache makes later lookups stale for the lifetime of the index object.
  • Security: no direct new input parsing or privilege boundary, but stale plugin metadata can delay recognition of changed plugin manifests/package metadata. In a plugin trust boundary, stale metadata is a correctness risk with possible policy implications.
  • Performance: the goal is sensible, and repeated filesystem probes are avoided. The current implementation buys that performance by removing required invalidation.

src/plugins/manifest-registry-installed.fingerprint.test.ts

  • Correctness: the new test asserts the desired hot-path behavior for a mutable object without establishing that this object is safe to cache. It conflicts with existing tests that assert same-object mutations and live file changes must affect the fingerprint.
  • Error handling: cleanup is reasonable; temporary directories are removed in afterEach().
  • Concurrency: not relevant for this unit test.
  • Security: no new security-sensitive behavior.
  • Performance: the test verifies no second statSync() call, but only by spying on filesystem calls. It does not prove the cache is semantically safe.

Test Coverage Gaps

The added test covers only repeated reads of an unchanged same-object index. It does not cover:

  • Same-object record mutation after the first fingerprint call.
  • Same-object nested mutation after the first fingerprint call.
  • Manifest/package file mtime or size changes when fileSignature is absent.
  • Registry reconstruction or plugin metadata memo invalidation after a live manifest change.

Those scenarios are already represented by existing manifest-registry-installed.test.ts tests, and this PR appears to contradict them.

I attempted to run:

node node_modules/vitest/vitest.mjs run src/plugins/manifest-registry-installed.test.ts src/plugins/manifest-registry-installed.fingerprint.test.ts --reporter=verbose

but the local checkout does not have node_modules/vitest/vitest.mjs, so the command failed before executing tests.

gh pr checks also reports check-test-types failing on the PR.

Verdict

Request changes. The performance issue is real, but caching by object identity at this exported fingerprint function is too broad and breaks existing invalidation contracts. Please narrow the cache to a proven immutable/stable input path or carry a precomputed fingerprint from the place that owns the stable installed index object.

@clawsweeper clawsweeper Bot added status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 10, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Open
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jun 15, 2026
This was referenced Jun 21, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@galiniliev galiniliev

Labels

maintainer Maintainer-authored PR merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Plugin metadata hot paths repeat installed manifest filesystem probes

2 participants

@galiniliev @RomneyDa