fix(core): resolve config outside the working directory - #59
Conversation
mrsimpson
left a comment
There was a problem hiding this comment.
Reviewed #59. Implementation is clean — no monkeypatches, no any, and the test changes are proper env isolation (save/restore HOME/USERPROFILE in beforeEach/afterEach). The approach is a reasonable shipped-now fix, but it's an env-var + home-fallback workaround for a problem MCP roots was designed to solve (see the note on server.ts).
Two things I'd want resolved before merge:
- Write-safety inconsistency —
creategets{ includeHome: false }, butinitwrites without it (inline oncreate.ts). Either guardinit/refreshtoo or document the intent. - Silent home fallback — a behavior change with no opt-out for strict project-only resolution (inline on
discovery.ts).
Minor (non-blocking):
candidateConfigPaths()materializes the full ancestor list before probing; an early-return during the walk would avoid the allocation (trivial here, just noting).KNOWLEDGE_SUBDIRreads ambiguously ("subdir of.knowledge" vs "the dir holding config.yaml"); it's documented so this is fine, just slightly confusing.
Docs in README/USER_GUIDE are good — they explain the resolution order and don't hide the env-var friction.
| const configExists = await configManager.configExists(cwd); | ||
| // The home config is never a write target for a project that has none: | ||
| // creating a docset here would silently edit the user's global config | ||
| const discovery = { includeHome: false }; |
There was a problem hiding this comment.
This guard is exactly right for create. But init writes to the config too — ensureKnowledgeGitignoreSync(configPath), coreInitDocset(..., configPath, ...) (drops metadata into the .knowledge dir), and updateDocsetPaths(...) (rewrites config.yaml) — all reached via configManager.loadConfig(cwd) in init.ts with no includeHome option. So a project with no local config but a matching docset in ~/.knowledge/config.yaml will have init write into the home config — the same class of bug this fixes here. Suggest extending the guard to init (and reconsidering refresh, which also calls ensureKnowledgeGitignoreSync), or documenting why init-to-home is intended.
| } | ||
|
|
||
| if (includeHome) { | ||
| const homeConfigPath = configPathFor(homedir()); |
There was a problem hiding this comment.
Stronger take: I think home support should be opt-in, not the default. The norm is that config lives with the project so it stays up-to-date and versioned; a silent ~/.knowledge/config.yaml fallback masks a missing project config (you never notice you forgot one) and quietly changes behavior for every findConfigPath caller.
Suggest defaulting includeHome to false, and enabling it explicitly where the GUI-launch problem actually lives — i.e. the MCP server path. Two implications to wire:
create.ts's explicit{ includeHome: false }would then just be the default (can keep for clarity).- The server must opt in — either pass
{ includeHome: true }, or gate it behind an explicit env knob likeKNOWLEDGE_HOME_FALLBACK=1. If neither is done, the very bug this PR fixes regresses for GUI clients that don't setPROJECT_DIR/KNOWLEDGE_SUBDIR.
Going fully env-driven keeps it user-controlled and makes the "global docsets" case a deliberate choice rather than an implicit side effect.
| "See the search_docs tool description for example configuration.", | ||
| "Create .knowledge/config.yaml in your project root or home directory.\n" + | ||
| "See the search_docs tool description for example configuration.\n\n" + | ||
| "**Option 3: Point at an existing configuration**\n" + |
There was a problem hiding this comment.
Design question: why not use MCP roots (ListRootsRequestSchema / setRootsListChangedRequestHandler) as the primary mechanism? The SDK supports it and it's the protocol-native channel for "what is my project root" — VS Code would report the workspace folder directly. I can see the rationale for env vars (works regardless of client capability, e.g. Claude Desktop's weak/absent roots support), but it cements a per-client env-var contract that's harder to unwind later. Worth a sentence in the PR/issue on why roots isn't the primary path here.
Review follow-up on #59. - includeHome now defaults to false. Discovery falls back to ~/.knowledge/config.yaml only where a machine-wide config is a legitimate answer: the MCP server (its working directory is dictated by the GUI client that launched it) and the CLI commands that operate on an already declared docset (status, init, refresh). Anything that may create a config, i.e. create, keeps the project-only default. - init/refresh keep the fallback on purpose: docsets declared in ~/.knowledge/config.yaml must be manageable from any directory. Their writes target the config that declared the docset, which is now documented. - The init_docset MCP tool no longer passes process.cwd() explicitly, which defeated PROJECT_DIR; it resolves the config like the read path does. - updateDocsetPaths takes the config path the caller resolved instead of re-running discovery, and the ConfigManager cache is keyed by start directory and options. A clone longer than the 60s cache TTL could otherwise make init write discovered paths to a different config. - candidateConfigPaths is a generator, so probing stops at the first hit without materialising the ancestor list.
Review follow-up on #59. - includeHome now defaults to false. Discovery falls back to ~/.knowledge/config.yaml only where a machine-wide config is a legitimate answer: the MCP server (its working directory is dictated by the GUI client that launched it) and the CLI commands that operate on an already declared docset (status, init, refresh). Anything that may create a config, i.e. create, keeps the project-only default. - init/refresh keep the fallback on purpose: docsets declared in ~/.knowledge/config.yaml must be manageable from any directory. Their writes target the config that declared the docset, which is now documented. - The init_docset MCP tool no longer passes process.cwd() explicitly, which defeated PROJECT_DIR; it resolves the config like the read path does. - updateDocsetPaths takes the config path the caller resolved instead of re-running discovery, and the ConfigManager cache is keyed by start directory and options. A clone longer than the 60s cache TTL could otherwise make init write discovered paths to a different config. - candidateConfigPaths is a generator, so probing stops at the first hit without materialising the ancestor list.
5d4d1ae to
40f31be
Compare
|
Addressed in 40f31be. Done
Not done: guarding
MCP Build, typecheck, 227 tests, oxlint and prettier clean. |
This fixes #17.
findConfigPathonly walked up fromprocess.cwd(). GUI clients spawn the server with a working directory unrelated to the user's project (Claude Desktop reports/Applications, VS Code its own app bundle), so no project or user config was ever found and every tool call reported "No configuration file found".Resolution order is now the one described in the issue:
KNOWLEDGE_SUBDIR— the directory holdingconfig.yaml. If it has none,nothing is loaded; the override is never silently ignored.
PROJECT_DIR, when set.~/.knowledge/config.yamlas a shared fallback.Notes for review
packages/core/src/config/discovery.tsinstead of copyingdirectory-discovery.tsfrom prompts-mcp: this repo needs a config file path with both an async and a sync variant, and the CLI shares the same code path. A second utility inpackages/mcp-serverwould leave two mechanisms with the CLI still on the old one. Env contract and search order are identical.{ includeHome?: boolean }(defaulttrue) andcreatepassesfalse. Without it,createin a project that has no config appends the docset to the user's global~/.knowledge/config.yaml— this happened during development.HOME/USERPROFILEto a temp dir, otherwise a real~/.knowledge/config.yamlon the developer machine leaks into results.