feat(tools): lazy loading baseline with tool profiles (Sprint 4.2 Phase 0+1)#23
feat(tools): lazy loading baseline with tool profiles (Sprint 4.2 Phase 0+1)#23codewithkenzo merged 8 commits intomainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const loadTool = async (): Promise<ToolDefinition> => { | ||
| if (cached) { | ||
| return cached | ||
| } | ||
| if (loading) { | ||
| return loading | ||
| } | ||
|
|
||
| loading = (async () => { | ||
| const start = performance.now() | ||
| const tool = await opts.loader() | ||
| const elapsed = performance.now() - start | ||
| cached = tool | ||
| loading = null | ||
| opts.onFirstLoad?.(opts.name, elapsed) | ||
| return tool | ||
| })() |
There was a problem hiding this comment.
Loader rejection wedges tool
If opts.loader() rejects, loading is never cleared (it’s only set back to null on the success path). That leaves the wrapper permanently stuck returning the same rejected promise on all future execute() calls. Clearing loading in a finally (and only caching on success) avoids permanently bricking a tool after a transient import/init failure.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/lazy-tool-wrapper.ts
Line: 20:36
Comment:
**Loader rejection wedges tool**
If `opts.loader()` rejects, `loading` is never cleared (it’s only set back to `null` on the success path). That leaves the wrapper permanently stuck returning the same rejected promise on all future `execute()` calls. Clearing `loading` in a `finally` (and only caching on success) avoids permanently bricking a tool after a transient import/init failure.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: src/index.ts
Line: 342:353
Comment:
**Tool snapshot always logs**
`logToolRegistrySnapshot(...)` is called unconditionally at startup and logs the full `tools` array (~90+ entries). This will spam normal CLI logs (and any log aggregation) even when lazy loading is disabled. Consider gating this behind a config flag (or log only counts unless debug is enabled).
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
|
@greptileai review Please provide:
This PR adds feature-flagged lazy tool loading with tool profiles for oh-my-opencode. Key changes:
|
|
@greptileai review |
codewithkenzo
force-pushed
the
feat/sprint4-2-lazy-loading
branch
from
78e6e9c to
83b82ea
Compare
|
The "Loader rejection wedges tool" finding was already fixed in commit @greptileai review |
| modelCacheState, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Unconditional tool snapshot logging
When lazy_loading.enabled is false (the default), this PR still logs the full tool registry snapshot + startup timing on every startup. That’s a behavior change (and can be very noisy / large output) even though the PR claims “identical to current behavior” when disabled. Consider gating logToolRegistrySnapshot(...) and log("[startup-timing]", ...) behind a config flag (e.g. lazy_loading.log_timing or a dedicated startup.log_timing).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.ts
Line: 345:347
Comment:
**Unconditional tool snapshot logging**
When `lazy_loading.enabled` is false (the default), this PR still logs the full tool registry snapshot + startup timing on every startup. That’s a behavior change (and can be very noisy / large output) even though the PR claims “identical to current behavior” when disabled. Consider gating `logToolRegistrySnapshot(...)` and `log("[startup-timing]", ...)` behind a config flag (e.g. `lazy_loading.log_timing` or a dedicated `startup.log_timing`).
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| marks.push({ label, timestamp: performance.now() }) | ||
| } | ||
|
|
There was a problem hiding this comment.
performance may be undefined
createStartupTimer() uses performance.now() directly. If this runs in a Node/Bun environment where globalThis.performance isn’t available (or is disabled in some runtime modes), this will throw at plugin startup. Using globalThis.performance?.now?.() with a fallback to Date.now() avoids startup crashes in those environments.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/tool-timing.ts
Line: 26:28
Comment:
**`performance` may be undefined**
`createStartupTimer()` uses `performance.now()` directly. If this runs in a Node/Bun environment where `globalThis.performance` isn’t available (or is disabled in some runtime modes), this will throw at plugin startup. Using `globalThis.performance?.now?.()` with a fallback to `Date.now()` avoids startup crashes in those environments.
How can I resolve this? If you propose a fix, please make it concise.
| name, | ||
| description: tool.description, | ||
| args: tool.args, | ||
| loader: async () => tool, |
There was a problem hiding this comment.
The loader just returns the already-loaded tool object, providing no actual lazy loading benefit. Tools are all imported/evaluated at module load time via the barrel exports, so wrapping them in createLazyTool won't defer initialization. To gain startup speed, the loader should use dynamic import() for each tool module.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/index.ts
Line: 241:241
Comment:
The loader just returns the already-loaded `tool` object, providing no actual lazy loading benefit. Tools are all imported/evaluated at module load time via the barrel exports, so wrapping them in `createLazyTool` won't defer initialization. To gain startup speed, the loader should use dynamic `import()` for each tool module.
How can I resolve this? If you propose a fix, please make it concise.
codewithkenzo
force-pushed
the
feat/sprint4-2-lazy-loading
branch
from
83b82ea to
c9ce954
Compare
|
|
||
| loading = (async () => { | ||
| const start = performance.now() | ||
| try { | ||
| const tool = await opts.loader() | ||
| const elapsed = performance.now() - start | ||
| cached = tool |
There was a problem hiding this comment.
Missing performance fallback
createLazyTool() uses performance.now() for load timing, which will throw at runtime if globalThis.performance is unavailable in the CLI runtime. This makes any lazy-loaded tool crash on first execute() in those environments; consider using globalThis.performance?.now?.() with a Date.now() fallback (same fix as startup timer).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/lazy-tool-wrapper.ts
Line: 27:33
Comment:
**Missing performance fallback**
`createLazyTool()` uses `performance.now()` for load timing, which will throw at runtime if `globalThis.performance` is unavailable in the CLI runtime. This makes any lazy-loaded tool crash on first `execute()` in those environments; consider using `globalThis.performance?.now?.()` with a `Date.now()` fallback (same fix as startup timer).
How can I resolve this? If you propose a fix, please make it concise.| log("[lazy-tool] first load", { | ||
| tool: name, | ||
| loadTimeMs: Math.round(ms * 100) / 100, | ||
| }) |
There was a problem hiding this comment.
Noisy startup logging
logToolRegistrySnapshot(...) and log("[startup-timing]", ...) run unconditionally after tools-assembled, even when lazy_loading.enabled is false (the default). This changes baseline behavior and can produce very large logs on every startup; gate these behind a config flag (e.g. lazy_loading.log_timing or a dedicated startup logging option).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/index.ts
Line: 353:356
Comment:
**Noisy startup logging**
`logToolRegistrySnapshot(...)` and `log("[startup-timing]", ...)` run unconditionally after `tools-assembled`, even when `lazy_loading.enabled` is false (the default). This changes baseline behavior and can produce very large logs on every startup; gate these behind a config flag (e.g. `lazy_loading.log_timing` or a dedicated startup logging option).
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| marks.push({ label, timestamp: performance.now() }) | ||
| } | ||
|
|
||
| const report = (): StartupTimingReport => { | ||
| let previousTimestamp = startedAt | ||
|
|
There was a problem hiding this comment.
Startup crash risk
createStartupTimer() calls performance.now() directly. If globalThis.performance is undefined in the runtime, plugin startup will throw before tools are registered. Use globalThis.performance?.now?.() with a fallback to Date.now().
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/tool-timing.ts
Line: 26:31
Comment:
**Startup crash risk**
`createStartupTimer()` calls `performance.now()` directly. If `globalThis.performance` is undefined in the runtime, plugin startup will throw before tools are registered. Use `globalThis.performance?.now?.()` with a fallback to `Date.now()`.
How can I resolve this? If you propose a fix, please make it concise.| if (coreToolNames.has(name)) { | ||
| result[name] = tool | ||
| } else { | ||
| result[name] = createLazyTool({ | ||
| name, | ||
| description: tool.description, | ||
| args: tool.args, | ||
| loader: async () => tool, |
There was a problem hiding this comment.
Not actually lazy
In createBuiltinToolsWithLazyLoading(), the lazy wrapper loader is async () => tool, but tool is already imported and fully initialized by the time builtinTools is constructed. This means enabling lazy_loading.enabled won’t defer module evaluation/side effects and is unlikely to improve startup time; to get real lazy loading, the loader needs to import() the tool module (or otherwise avoid eagerly constructing builtinTools).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/index.ts
Line: 234:241
Comment:
**Not actually lazy**
In `createBuiltinToolsWithLazyLoading()`, the lazy wrapper `loader` is `async () => tool`, but `tool` is already imported and fully initialized by the time `builtinTools` is constructed. This means enabling `lazy_loading.enabled` won’t defer module evaluation/side effects and is unlikely to improve startup time; to get real lazy loading, the loader needs to `import()` the tool module (or otherwise avoid eagerly constructing `builtinTools`).
How can I resolve this? If you propose a fix, please make it concise.
codewithkenzo
force-pushed
the
feat/sprint4-1-mcp-registry-adoption
branch
from
01de010 to
12a6b3f
Compare
…luginConfigToClaudeConfig
…, scope filter, early limit - Restore per-server config validation in getUserMcpInfo() (was hardcoded valid: true) - Make toRuntimeMcpServerMap() resilient with try-catch per server - Wrap createMcpRegistry() in try-catch with fallback to builtin+custom only - Wrap loadPluginMcpServers() in try-catch for graceful degradation - Scope filter now only applies to custom sources (builtin/plugin/skill not silently dropped) - Apply limit before connecting to servers (not after)
…y command validation - Wrap pluginConfigToClaudeConfig in try-catch per server in createMcpRegistry so one bad plugin config doesn't crash the entire registry - Strengthen doctor command validation to catch empty string and empty array - Update tests: malformed plugin configs are now silently skipped, not thrown
- createStartupTimer() with mark/report/elapsed using performance.now() - getToolRegistrySnapshot() + logToolRegistrySnapshot() for tool inventory logging - Exported from shared barrel Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
- LazyLoadingConfigSchema: enabled (default false), log_timing, default_profiles - createLazyTool(): deferred execute with concurrent-call dedup and caching - 6 tool profiles: core, research, browser, native-search, external-api, local-service - createBuiltinToolsWithLazyLoading(): wraps non-core tools as lazy when enabled - Startup timing instrumentation (config-loaded, hooks-initialized, skills-discovered, tools-assembled) - Tool registry snapshot logged at startup - Feature flag wired in index.ts: lazy_loading.enabled=false preserves existing behavior - Tests: 13 passing (6 lazy-wrapper, 7 tool-profiles) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…ed tools Move loading=null to finally block so transient loader failures don't permanently brick the lazy wrapper. Adds recovery test. Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
codewithkenzo
force-pushed
the
feat/sprint4-2-lazy-loading
branch
from
c9ce954 to
a9b78d1
Compare
codewithkenzo
changed the base branch from
feat/sprint4-1-mcp-registry-adoption
to
main
1 participant
Summary
Sprint 4.2 Phase 0 + Phase 1: Feature-flagged lazy tool loading with profile-based classification.
createStartupTimer()tracks config-loaded → hooks-initialized → skills-discovered → tools-assembled phases. Tool registry snapshot logged at startup.createLazyTool()defersexecute()to first call with concurrent-call deduplication, caching, and optional timing callback.core,research,browser,native-search,external-api,local-service) classifying all ~89 builtin tools.lazy_loading.enabled(defaultfalse),lazy_loading.log_timing,lazy_loading.default_profiles.lazy_loading.enabled === true, non-core tools use lazy wrappers. When disabled (default), identical to current behavior — zero regression risk.New Files
src/shared/tool-timing.ts— startup timer utilitysrc/shared/tool-registry-snapshot.ts— tool inventory snapshot/loggingsrc/tools/lazy-tool-wrapper.ts— lazy execute wrapper factorysrc/tools/tool-profiles.ts— 6-profile tool classificationsrc/tools/lazy-tool-wrapper.test.ts— 6 testssrc/tools/tool-profiles.test.ts— 7 testsModified Files
src/shared/index.ts— barrel exports for new utilitiessrc/config/schema.ts—LazyLoadingConfigSchemaaddedsrc/tools/index.ts—createBuiltinToolsWithLazyLoading()+ re-exportssrc/index.ts— timing marks + feature flag wiringVerification
bun run typecheck✅ cleanNext (Sprint 4.2 Phase 2+)
Greptile Overview
Greptile Summary
Sprint 4.2 Phase 0+1 implements feature-flagged lazy tool loading with profile-based classification and startup timing instrumentation. When
lazy_loading.enabledis false (default), behavior is unchanged except for new startup logging.Key Changes:
core,research,browser,native-search,external-api,local-service) classify ~89 builtin toolsIssues Already Addressed (Previous Threads):
globalThis.performance)import())No New Critical Issues Found:
Confidence Score: 4/5
src/index.ts:362-363(unconditional logging) andsrc/shared/tool-timing.ts:20,26+src/tools/lazy-tool-wrapper.ts:29(performance API usage without fallback)Important Files Changed
Sequence Diagram
sequenceDiagram participant Plugin as Plugin Init participant Config as Config Handler participant Timer as Startup Timer participant Tools as Tool Factory participant Lazy as Lazy Wrapper participant Registry as MCP Registry participant Doctor as Doctor Check Plugin->>Timer: createStartupTimer() Timer-->>Plugin: timer instance Plugin->>Config: createConfigHandler() Config->>Config: Load user config Config-->>Plugin: config handler Plugin->>Timer: mark("config-loaded") Plugin->>Plugin: Initialize hooks/skills Plugin->>Timer: mark("hooks-initialized") Plugin->>Timer: mark("skills-discovered") Plugin->>Tools: Check lazy_loading.enabled alt lazy_loading.enabled === true Plugin->>Tools: createBuiltinToolsWithLazyLoading() Tools->>Tools: Classify tools by profile loop For each non-core tool Tools->>Lazy: createLazyTool({loader, name, ...}) Lazy-->>Tools: Lazy wrapper (execute deferred) end Tools-->>Plugin: Lazy-wrapped tools else lazy_loading.enabled === false Plugin->>Tools: Use builtinTools directly Tools-->>Plugin: Eager tools end Plugin->>Timer: mark("tools-assembled") Plugin->>Plugin: logToolRegistrySnapshot() Plugin->>Plugin: log startup timing Note over Plugin: Later: First tool execution Plugin->>Lazy: tool.execute(args, ctx) Lazy->>Lazy: Check if cached Lazy->>Lazy: Check if loading Lazy->>Lazy: Call loader() in finally wrapper Lazy->>Lazy: Cache result & clear loading Lazy-->>Plugin: Execute actual tool Note over Registry,Doctor: MCP Validation Flow Doctor->>Registry: createMcpRegistry(config) Registry->>Registry: Validate builtin servers Registry->>Registry: Validate custom servers Registry->>Registry: Validate plugin servers alt Invalid plugin config Registry->>Registry: Catch error, skip server end Registry-->>Doctor: Filtered server list Doctor->>Doctor: Check for empty commands Doctor->>Doctor: Check for missing urls Doctor-->>Plugin: Validation results