Browse Source

fix(opencode): cache bootstrap content at module level to eliminate per-step file I/O

getBootstrapContent() called fs.existsSync + fs.readFileSync + regex
frontmatter parsing on every agent step with zero caching.  The
experimental.chat.messages.transform hook fires every step in opencode's
agent loop (messages are reloaded from DB each step via
filterCompactedEffect).  A 10-step turn triggered 10 redundant file
reads + 10 regex parses for content that never changes during a session.

Changes:
- Add module-level _bootstrapCache (undefined = not loaded, null = file
  missing) so the first call reads and parses SKILL.md, all subsequent
  calls return the cached string with zero filesystem access
- Cache the null sentinel when SKILL.md is missing, preventing repeated
  fs.existsSync probes
- Add _testing export (resetCache/getCache) for test infrastructure
- Clarify the injection guard comment explaining how it interacts with
  opencode's per-step message reloading
- Add 15 regression tests covering cache behavior, fs call counts,
  injection guard, missing file sentinel, cache reset, and source audit

Fixes #1202
voidborne-d 5 months ago
parent
commit
c13a884c06
2 changed files with 427 additions and 4 deletions
  1. 39 4
      .opencode/plugins/superpowers.js
  2. 388 0
      tests/opencode/test-bootstrap-caching.sh

+ 39 - 4
.opencode/plugins/superpowers.js

@@ -46,17 +46,29 @@ const normalizePath = (p, homeDir) => {
   return path.resolve(normalized);
 };
 
+// Module-level cache for bootstrap content.
+// The SKILL.md file does not change during a session, so reading + parsing it
+// once eliminates redundant fs.existsSync + fs.readFileSync + regex work on
+// every agent step.  See #1202 for the full analysis.
+let _bootstrapCache = undefined; // undefined = not yet loaded, null = file missing
+
 export const SuperpowersPlugin = async ({ client, directory }) => {
   const homeDir = os.homedir();
   const superpowersSkillsDir = path.resolve(__dirname, '../../skills');
   const envConfigDir = normalizePath(process.env.OPENCODE_CONFIG_DIR, homeDir);
   const configDir = envConfigDir || path.join(homeDir, '.config/opencode');
 
-  // Helper to generate bootstrap content
+  // Helper to generate bootstrap content (cached after first call)
   const getBootstrapContent = () => {
+    // Return cached result on subsequent calls
+    if (_bootstrapCache !== undefined) return _bootstrapCache;
+
     // Try to load using-superpowers skill
     const skillPath = path.join(superpowersSkillsDir, 'using-superpowers', 'SKILL.md');
-    if (!fs.existsSync(skillPath)) return null;
+    if (!fs.existsSync(skillPath)) {
+      _bootstrapCache = null;
+      return null;
+    }
 
     const fullContent = fs.readFileSync(skillPath, 'utf8');
     const { content } = extractAndStripFrontmatter(fullContent);
@@ -70,7 +82,7 @@ When skills reference tools you don't have, substitute OpenCode equivalents:
 
 Use OpenCode's native \`skill\` tool to list and load skills.`;
 
-    return `<EXTREMELY_IMPORTANT>
+    _bootstrapCache = `<EXTREMELY_IMPORTANT>
 You have superpowers.
 
 **IMPORTANT: The using-superpowers skill content is included below. It is ALREADY LOADED - you are currently following it. Do NOT use the skill tool to load "using-superpowers" again - that would be redundant.**
@@ -79,6 +91,8 @@ ${content}
 
 ${toolMapping}
 </EXTREMELY_IMPORTANT>`;
+
+    return _bootstrapCache;
   };
 
   return {
@@ -98,15 +112,36 @@ ${toolMapping}
     // Using a user message instead of a system message avoids:
     //   1. Token bloat from system messages repeated every turn (#750)
     //   2. Multiple system messages breaking Qwen and other models (#894)
+    //
+    // The hook fires on every agent step (not just every turn) because
+    // opencode's prompt.ts reloads messages from DB each step.  The guard
+    // below prevents duplicate injection on the fresh in-memory objects.
     'experimental.chat.messages.transform': async (_input, output) => {
       const bootstrap = getBootstrapContent();
       if (!bootstrap || !output.messages.length) return;
       const firstUser = output.messages.find(m => m.info.role === 'user');
       if (!firstUser || !firstUser.parts.length) return;
-      // Only inject once
+
+      // Guard: skip if first user message already contains bootstrap.
+      // Because messages are loaded fresh from DB each step (the previous
+      // in-memory injection is never persisted), this guard fires on the
+      // *new* in-memory objects.  It still works correctly: the first step
+      // injects, and on subsequent steps the content-based check catches it
+      // because we're looking at the same logical first-user message that
+      // was just injected into (the DB hasn't changed between steps within
+      // a single turn, so filterCompactedEffect returns the same message
+      // contents and we re-inject the same bootstrap — the guard prevents
+      // doubling up within the same messages array).
       if (firstUser.parts.some(p => p.type === 'text' && p.text.includes('EXTREMELY_IMPORTANT'))) return;
+
       const ref = firstUser.parts[0];
       firstUser.parts.unshift({ ...ref, type: 'text', text: bootstrap });
     }
   };
 };
+
+// Exported for testing — allows tests to reset the cache between runs
+export const _testing = {
+  resetCache: () => { _bootstrapCache = undefined; },
+  getCache: () => _bootstrapCache,
+};

+ 388 - 0
tests/opencode/test-bootstrap-caching.sh

@@ -0,0 +1,388 @@
+#!/usr/bin/env bash
+# Test: Bootstrap Content Caching (#1202)
+# Verifies that getBootstrapContent() caches at module level,
+# eliminating per-step file I/O and regex parsing overhead.
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+
+echo "=== Test: Bootstrap Content Caching (#1202) ==="
+
+# Source setup to create isolated environment
+source "$SCRIPT_DIR/setup.sh"
+
+# Trap to cleanup on exit
+trap cleanup_test_env EXIT
+
+passed=0
+failed=0
+
+pass() { echo "  [PASS] $1"; passed=$((passed + 1)); }
+fail() { echo "  [FAIL] $1"; failed=$((failed + 1)); }
+
+# ──────────────────────────────────────────────────────────────
+# Test 1: Module-level _bootstrapCache variable exists
+# ──────────────────────────────────────────────────────────────
+echo "Test 1: Module-level cache variable exists..."
+if grep -q '_bootstrapCache' "$SUPERPOWERS_PLUGIN_FILE"; then
+    pass "Module-level _bootstrapCache variable found"
+else
+    fail "_bootstrapCache variable not found in plugin"
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 2: Cache is checked before fs operations
+# ──────────────────────────────────────────────────────────────
+echo "Test 2: Cache checked before filesystem access..."
+# The pattern: if (_bootstrapCache !== undefined) return should appear
+# BEFORE any fs.existsSync / fs.readFileSync in getBootstrapContent
+if grep -qP '_bootstrapCache !== undefined.*return' "$SUPERPOWERS_PLUGIN_FILE"; then
+    pass "Early return on cache hit exists"
+else
+    fail "No early return on cache hit"
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 3: Cache is populated after file read
+# ──────────────────────────────────────────────────────────────
+echo "Test 3: Cache populated after successful file read..."
+if grep -q '_bootstrapCache =' "$SUPERPOWERS_PLUGIN_FILE"; then
+    pass "Cache assignment exists"
+else
+    fail "No cache assignment found"
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 4: Missing file path also cached (null sentinel)
+# ──────────────────────────────────────────────────────────────
+echo "Test 4: Missing file case also cached (null sentinel)..."
+if grep -q '_bootstrapCache = null' "$SUPERPOWERS_PLUGIN_FILE"; then
+    pass "Null sentinel for missing file path"
+else
+    fail "Missing file path not cached — would re-check fs.existsSync every step"
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 5: No redundant fs.readFileSync on cached path
+# Verify via Node.js: call getBootstrapContent() twice, count reads
+# ──────────────────────────────────────────────────────────────
+echo "Test 5: Second call returns cached content without file I/O..."
+
+cat > "$TEST_HOME/test-cache.mjs" <<'TESTEOF'
+import { createRequire } from 'module';
+import path from 'path';
+import fs from 'fs';
+
+// Monkey-patch fs to count calls
+let readCount = 0;
+let existsCount = 0;
+const origReadFileSync = fs.readFileSync;
+const origExistsSync = fs.existsSync;
+
+fs.readFileSync = function(...args) {
+  readCount++;
+  return origReadFileSync.apply(this, args);
+};
+fs.existsSync = function(...args) {
+  existsCount++;
+  return origExistsSync.apply(this, args);
+};
+
+// Import the plugin (this sets __dirname based on plugin location)
+const pluginPath = process.argv[2];
+const mod = await import(pluginPath);
+
+// Initialize plugin
+const plugin = await mod.SuperpowersPlugin({ client: {}, directory: '.' });
+
+// Reset cache for clean test
+mod._testing.resetCache();
+
+// First call — should hit fs
+readCount = 0;
+existsCount = 0;
+const result1 = await plugin['experimental.chat.messages.transform'](
+  {},
+  { messages: [{
+    info: { role: 'user' },
+    parts: [{ type: 'text', text: 'hello' }]
+  }]}
+);
+const firstReadCount = readCount;
+const firstExistsCount = existsCount;
+
+// Reset cache state check
+const cacheAfterFirst = mod._testing.getCache();
+
+// Reset counters, call again (should be cached)
+// Need to reset the injection guard — create fresh messages
+readCount = 0;
+existsCount = 0;
+const result2 = await plugin['experimental.chat.messages.transform'](
+  {},
+  { messages: [{
+    info: { role: 'user' },
+    parts: [{ type: 'text', text: 'hello again' }]
+  }]}
+);
+const secondReadCount = readCount;
+const secondExistsCount = existsCount;
+
+// Output results
+console.log(JSON.stringify({
+  firstReadCount,
+  firstExistsCount,
+  secondReadCount,
+  secondExistsCount,
+  cachePopulated: cacheAfterFirst !== undefined && cacheAfterFirst !== null,
+  contentIncludesMarker: cacheAfterFirst?.includes('EXTREMELY_IMPORTANT') ?? false
+}));
+TESTEOF
+
+RESULT=$(node "$TEST_HOME/test-cache.mjs" "$SUPERPOWERS_PLUGIN_FILE" 2>/dev/null)
+if [ $? -ne 0 ]; then
+    fail "Cache test script failed to execute"
+else
+    SECOND_READ=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.secondReadCount))")
+    SECOND_EXISTS=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.secondExistsCount))")
+    CACHE_POP=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.cachePopulated))")
+    CONTENT_OK=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.contentIncludesMarker))")
+
+    if [ "$SECOND_READ" = "0" ] && [ "$SECOND_EXISTS" = "0" ]; then
+        pass "Second call made 0 fs.readFileSync and 0 fs.existsSync calls"
+    else
+        fail "Second call still made fs calls (read=$SECOND_READ, exists=$SECOND_EXISTS)"
+    fi
+
+    if [ "$CACHE_POP" = "true" ]; then
+        pass "Cache populated after first call"
+    else
+        fail "Cache not populated after first call"
+    fi
+
+    if [ "$CONTENT_OK" = "true" ]; then
+        pass "Cached content contains EXTREMELY_IMPORTANT marker"
+    else
+        fail "Cached content missing expected marker"
+    fi
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 6: _testing.resetCache() clears the cache
+# ──────────────────────────────────────────────────────────────
+echo "Test 6: resetCache() allows re-reading from disk..."
+
+cat > "$TEST_HOME/test-reset.mjs" <<'TESTEOF'
+import fs from 'fs';
+
+const pluginPath = process.argv[2];
+const mod = await import(pluginPath);
+
+// Initialize and populate cache
+const plugin = await mod.SuperpowersPlugin({ client: {}, directory: '.' });
+await plugin['experimental.chat.messages.transform'](
+  {},
+  { messages: [{ info: { role: 'user' }, parts: [{ type: 'text', text: 'test' }] }] }
+);
+
+const beforeReset = mod._testing.getCache();
+mod._testing.resetCache();
+const afterReset = mod._testing.getCache();
+
+console.log(JSON.stringify({
+  beforeResetDefined: beforeReset !== undefined,
+  afterResetUndefined: afterReset === undefined
+}));
+TESTEOF
+
+RESULT=$(node "$TEST_HOME/test-reset.mjs" "$SUPERPOWERS_PLUGIN_FILE" 2>/dev/null)
+if [ $? -ne 0 ]; then
+    fail "Reset test script failed to execute"
+else
+    BEFORE=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.beforeResetDefined))")
+    AFTER=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.afterResetUndefined))")
+
+    if [ "$BEFORE" = "true" ] && [ "$AFTER" = "true" ]; then
+        pass "resetCache() transitions from defined to undefined"
+    else
+        fail "resetCache() did not clear properly (before=$BEFORE, after=$AFTER)"
+    fi
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 7: Injection guard prevents double-injection in same array
+# ──────────────────────────────────────────────────────────────
+echo "Test 7: Injection guard prevents double-injection..."
+
+cat > "$TEST_HOME/test-guard.mjs" <<'TESTEOF'
+const pluginPath = process.argv[2];
+const mod = await import(pluginPath);
+mod._testing.resetCache();
+
+const plugin = await mod.SuperpowersPlugin({ client: {}, directory: '.' });
+
+// Create a message array and inject once
+const messages = [{
+  info: { role: 'user' },
+  parts: [{ type: 'text', text: 'hello' }]
+}];
+
+await plugin['experimental.chat.messages.transform']({}, { messages });
+const partsAfterFirst = messages[0].parts.length;
+
+// Call again on the SAME messages array (simulates what would happen
+// if the hook fired twice on the same in-memory messages)
+await plugin['experimental.chat.messages.transform']({}, { messages });
+const partsAfterSecond = messages[0].parts.length;
+
+console.log(JSON.stringify({
+  partsAfterFirst,
+  partsAfterSecond,
+  noDuplication: partsAfterFirst === partsAfterSecond
+}));
+TESTEOF
+
+RESULT=$(node "$TEST_HOME/test-guard.mjs" "$SUPERPOWERS_PLUGIN_FILE" 2>/dev/null)
+if [ $? -ne 0 ]; then
+    fail "Guard test script failed to execute"
+else
+    NO_DUP=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.noDuplication))")
+    FIRST=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.partsAfterFirst))")
+
+    if [ "$NO_DUP" = "true" ]; then
+        pass "Guard prevented double injection (parts count stable at $FIRST)"
+    else
+        fail "Bootstrap injected twice into same message"
+    fi
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 8: Missing skill file → null cached (no repeated fs probes)
+# ──────────────────────────────────────────────────────────────
+echo "Test 8: Missing SKILL.md file produces cached null..."
+
+cat > "$TEST_HOME/test-missing.mjs" <<'TESTEOF'
+import fs from 'fs';
+
+const pluginPath = process.argv[2];
+const mod = await import(pluginPath);
+mod._testing.resetCache();
+
+// Temporarily rename the skill file to simulate missing
+const skillDir = new URL('../../skills/using-superpowers/SKILL.md', import.meta.resolve(pluginPath));
+// We can't easily rename, so instead we'll test by checking the cache
+// behavior when _bootstrapCache is set to null
+mod._testing.resetCache();
+
+const plugin = await mod.SuperpowersPlugin({ client: {}, directory: '.' });
+
+// Monkey-patch fs.existsSync to return false for SKILL.md
+const orig = fs.existsSync;
+fs.existsSync = (p) => {
+  if (typeof p === 'string' && p.includes('using-superpowers')) return false;
+  return orig(p);
+};
+
+let readCount = 0;
+const origRead = fs.readFileSync;
+fs.readFileSync = function(...args) { readCount++; return origRead.apply(this, args); };
+
+// First call — should set null cache
+const msgs1 = [{ info: { role: 'user' }, parts: [{ type: 'text', text: 'test' }] }];
+await plugin['experimental.chat.messages.transform']({}, { messages: msgs1 });
+const cacheAfterMissing = mod._testing.getCache();
+const firstReadCount = readCount;
+
+// Second call — should hit null cache, skip fs entirely
+readCount = 0;
+const msgs2 = [{ info: { role: 'user' }, parts: [{ type: 'text', text: 'test2' }] }];
+await plugin['experimental.chat.messages.transform']({}, { messages: msgs2 });
+
+// Restore
+fs.existsSync = orig;
+fs.readFileSync = origRead;
+
+console.log(JSON.stringify({
+  cacheIsNull: cacheAfterMissing === null,
+  secondCallReads: readCount,
+  noInjection: msgs1[0].parts.length === 1
+}));
+TESTEOF
+
+RESULT=$(node "$TEST_HOME/test-missing.mjs" "$SUPERPOWERS_PLUGIN_FILE" 2>/dev/null)
+if [ $? -ne 0 ]; then
+    fail "Missing file test script failed to execute"
+else
+    IS_NULL=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.cacheIsNull))")
+    SEC_READS=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.secondCallReads))")
+    NO_INJ=$(echo "$RESULT" | node -e "const d=JSON.parse(require('fs').readFileSync(0,'utf8'));process.stdout.write(String(d.noInjection))")
+
+    if [ "$IS_NULL" = "true" ]; then
+        pass "Cache set to null for missing file"
+    else
+        fail "Cache not set to null for missing file"
+    fi
+
+    if [ "$SEC_READS" = "0" ]; then
+        pass "Second call with missing file made 0 fs reads"
+    else
+        fail "Second call with missing file still read fs ($SEC_READS times)"
+    fi
+
+    if [ "$NO_INJ" = "true" ]; then
+        pass "No bootstrap injected when file missing"
+    else
+        fail "Bootstrap somehow injected despite missing file"
+    fi
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 9: Source audit — no uncached fs.readFileSync in getBootstrapContent
+# ──────────────────────────────────────────────────────────────
+echo "Test 9: Source audit — getBootstrapContent caches all fs paths..."
+
+# Extract getBootstrapContent function body and verify cache pattern
+# The function should: check cache → fs.existsSync → fs.readFileSync → assign cache
+FUNC_BODY=$(sed -n '/const getBootstrapContent/,/^  };$/p' "$SUPERPOWERS_PLUGIN_FILE")
+
+# Verify the cache check comes before any fs call
+CACHE_LINE=$(echo "$FUNC_BODY" | grep -n '_bootstrapCache !== undefined' | head -1 | cut -d: -f1)
+EXISTS_LINE=$(echo "$FUNC_BODY" | grep -n 'fs.existsSync' | head -1 | cut -d: -f1)
+
+if [ -n "$CACHE_LINE" ] && [ -n "$EXISTS_LINE" ] && [ "$CACHE_LINE" -lt "$EXISTS_LINE" ]; then
+    pass "Cache check (line $CACHE_LINE) precedes fs.existsSync (line $EXISTS_LINE)"
+else
+    fail "Cache check does not precede fs.existsSync (cache=$CACHE_LINE, exists=$EXISTS_LINE)"
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 10: JavaScript syntax still valid after changes
+# ──────────────────────────────────────────────────────────────
+echo "Test 10: Plugin JavaScript syntax remains valid..."
+if node --check "$SUPERPOWERS_PLUGIN_FILE" 2>/dev/null; then
+    pass "Plugin JavaScript syntax is valid"
+else
+    fail "Plugin has JavaScript syntax errors"
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Test 11: _testing export exists for test infrastructure
+# ──────────────────────────────────────────────────────────────
+echo "Test 11: _testing export available..."
+if grep -q 'export const _testing' "$SUPERPOWERS_PLUGIN_FILE"; then
+    pass "_testing export exists"
+else
+    fail "_testing export not found"
+fi
+
+# ──────────────────────────────────────────────────────────────
+# Summary
+# ──────────────────────────────────────────────────────────────
+echo ""
+total=$((passed + failed))
+echo "=== Results: $passed/$total passed ==="
+
+if [ "$failed" -gt 0 ]; then
+    exit 1
+fi
+echo "=== All bootstrap caching tests passed ==="