refactor: delegate StateManager knowledge loading to KnowledgeTracker (plan 010)#84
Open
DavertMik wants to merge 6 commits into
Open
refactor: delegate StateManager knowledge loading to KnowledgeTracker (plan 010)#84DavertMik wants to merge 6 commits into
DavertMik wants to merge 6 commits into
Conversation
… (plan 010)
StateManager duplicated the entire knowledge directory scan (readdir + gray-matter
parse + URL match) that KnowledgeTracker already owns — a second scanner with its
own 30s cache, drifting independently. Per the code-ownership map, KnowledgeTracker
is the single owner of reading ./knowledge.
- StateManager takes an injected KnowledgeTracker and delegates getRelevantKnowledge
to it; deletes scanKnowledgeFiles, knowledgeCache, lastKnowledgeScan, knowledgeDir
and the config-relative dir resolution. Explorer now constructs the KnowledgeTracker
and passes the same instance in (one owner, one cache).
- KnowledgeTracker: recursive .md scan (matches the removed scanner), a 30s refresh
TTL so long sessions see edits, and addKnowledge() invalidates the cache. Knowledge
interface is exported from KnowledgeTracker and re-exported from StateManager for
back-compat.
The constructor accepts (but does not act on) `incognito` so Explorer keeps passing
it; the actual incognito wiring is plan 009's ExperienceTracker({ disabled }) change.
This keeps the two PRs independent off main and regression-free in either merge order.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DavertMik
force-pushed
the
refactor/knowledge-single-owner-standalone
branch
from
4f92c12 to
be2662b
Compare
Explorbot Self-RegressionCommit
Attempt details
Session analysis — basic (native): Test Session SummarySession Results: 5/5 tests passed (100% success rate) Test Details:
Key Achievements:
All pilot verifications confirmed expected behaviors. No remaining expectations or failures to address. |
…ngle-owner-standalone # Conflicts: # src/state-manager.ts
Knowledge is persistent. The 30s TTL mirrored the old StateManager scanner's throttle, but that throttle only existed because the scanner naively re-read the directory on every getRelevantKnowledge call. KnowledgeTracker loads once (isLoaded) and addKnowledge() invalidates the single shared instance's cache on writes, so there is nothing to poll for — the TTL just re-read disk every 30s for data that doesn't change mid-session. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
KnowledgeTracker derives everything from ConfigParser (app-global) and reads the same persistent ./knowledge dir, so there is never a reason for more than one. The app had ~10 `new KnowledgeTracker()` sites, each with its own cache — the exact duplication plan 010 set out to remove, and the injected-param on StateManager was a footgun (a missing arg silently spawned a divergent instance). Mirror ConfigParser: private constructor + static getInstance() + resetForTesting(). StateManager drops the constructor param and just calls KnowledgeTracker.getInstance(); every other call site (Explorer, ExperienceTracker, Navigator, historians, AddKnowledge, explorbot, CLI) does the same. One instance, one cache app-wide; addKnowledge() invalidation is now global. Tests reset the singleton in beforeEach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… singleton)
Replaces the KnowledgeTracker.getInstance() singleton with ownership in the DI
container. ExplorBot exposes experienceTracker() / knowledgeTracker() (lazy
||=-cached, matching the agentXxx() factory pattern) and injects both DOWN through
Explorer into new StateManager(experienceTracker, knowledgeTracker) — lower layers
never import the container, so no explorbot<->state-manager import cycle and no
process-global static state.
- KnowledgeTracker reverts to a plain class (no static instance/getInstance/reset).
- ExperienceTracker takes the shared KnowledgeTracker as a constructor arg.
- StateManager takes both trackers injected; keeps its get*Tracker() getters so the
~25 consumers reading stateManager.getExperienceTracker() are unchanged. Adds
getKnowledgeTracker().
- Consumers that used to new-up or getInstance() a tracker now route through the
shared instance: Navigator/Historian mixins via explorer/stateManager, Captain via
explorBot.experienceTracker(). Standalone one-shot contexts with no ExplorBot
(CLI `learn`, standalone AddKnowledge which now takes a knowledgeTracker prop)
construct their own.
- ExplorBot get{Experience,Knowledge}Tracker() renamed to {experience,knowledge}Tracker().
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Navigator already holds `explorer`, and its only caller passed exactly `explorer.getStateManager().getExperienceTracker()` — so the optional param plus `|| fallback` was pure redundancy (and the same divergent-instance footgun). Get it from explorer directly, matching the knowledgeTracker line right above it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
DavertMik
requested a deployment
to
regression
GitHub Actions
In progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements plan 010 (
plans/010-*.md) — makeKnowledgeTrackerthe single owner of./knowledge, soStateManagerstops running its own duplicate scanner.The problem
StateManagerduplicated the entire knowledge-directory scan (readdir+ gray-matter + URL match) thatKnowledgeTrackeralready owns. Worse,KnowledgeTrackeritself wasnew'd in ~10 places (Explorer, ExperienceTracker, Navigator, historians, AddKnowledge, CLI…), each with its own cache — since it derives everything fromConfigParser(app-global) and reads the same persistent files, there is never a reason for more than one.The fix: singleton
KnowledgeTrackerbecomes a singleton mirroringConfigParser—private constructor+static getInstance()+static resetForTesting(). Every call site usesgetInstance();StateManagerdrops its constructor param entirely and just callsKnowledgeTracker.getInstance(), delegatinggetRelevantKnowledge()to it.Result: one instance, one cache, app-wide.
addKnowledge()invalidation is now global, and the injected-param footgun (a missing arg silently spawning a divergent instance) is gone.Also:
StateManagerdeletesscanKnowledgeFiles/knowledgeCache/lastKnowledgeScan/knowledgeDir;KnowledgeTrackergains a recursive.mdscan (parity with the removed scanner) and${env.*}/${config.*}interpolation. No refresh TTL — knowledge is persistent; load-once +addKnowledgeinvalidation is sufficient.Verification
bun run lintclean;bunx tsc --noEmit724 (= baseline, no new errors in touched files); unit 764/0; integration 63/0. Tests reset the singleton inbeforeEach.🤖 Generated with Claude Code