fix(consolidation): default ON when LLM provider is configured (#612)#696
Merged
Conversation
Users with a provider configured (Anthropic / OpenAI / OpenAI-compatible local endpoint via OPENAI_BASE_URL / OpenRouter / Gemini / Minimax / agent-sdk) were silently getting zero graph nodes, lessons, and crystals because CONSOLIDATION_ENABLED defaulted to false. The auto-consolidate pipeline only fires when the flag is true, so 100+ session corpora sat unprocessed unless the user knew to flip the flag manually. The fix: - `isConsolidationEnabled()` now returns true by default whenever a non-noop LLM provider is detectable from env (API key set, OPENAI_BASE_URL set, or AGENTMEMORY_PROVIDER=agent-sdk). - Explicit `CONSOLIDATION_ENABLED=false` or `AGENTMEMORY_PROVIDER=noop` still opt out — no behavior change for BM25-only / noop users. - Skip-reason text on the pipeline function updated to point at the env vars users should set. Closes the silent-no-graph case reported in discussion #612 (franklyfresh: 100 sessions, OpenAI-compatible LLM via Open WebUI / vLLM, summarization working, graph empty). 8 unit tests cover: no-provider default-off, every supported provider default-on, explicit `=false` override, explicit `=true` override, and AGENTMEMORY_PROVIDER=noop wins over API-key presence.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughConsolidation now defaults to enabled when an LLM provider is configured; ChangesConsolidation default enabling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/consolidation-default.test.ts (1)
70-104: ⚡ Quick winAdd a regression test for
OPENAI_API_KEY_FOR_LLM=false.This suite should also assert that consolidation stays OFF when only
OPENAI_API_KEYis present but LLM usage is explicitly disabled viaOPENAI_API_KEY_FOR_LLM=false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/consolidation-default.test.ts` around lines 70 - 104, Add a regression test in the same suite (test/consolidation-default.test.ts) that uses writeEnv to set OPENAI_API_KEY=sk-test-123 and OPENAI_API_KEY_FOR_LLM=false, calls freshConfig(), and asserts cfg.isConsolidationEnabled() is false; place it alongside the existing cases (referencing writeEnv, freshConfig, and isConsolidationEnabled) to ensure consolidation remains off when the LLM-specific flag disables usage even if OPENAI_API_KEY is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.ts`:
- Around line 333-345: hasLLMProviderConfigured is incorrectly treating
OPENAI_API_KEY as a valid LLM provider even when OPENAI_API_KEY_FOR_LLM is set
to "false"; update hasLLMProviderConfigured to only count OPENAI_API_KEY when
env["OPENAI_API_KEY_FOR_LLM"] !== "false" (case-insensitive), leaving other keys
(ANTHROPIC_API_KEY, OPENROUTER_API_KEY, GEMINI_API_KEY, GOOGLE_API_KEY,
MINIMAX_API_KEY, OPENAI_BASE_URL) and provider === "agent-sdk" unchanged, and
keep the noop provider behavior the same so detection aligns with
detectProvider/detectLlmProviderKind.
In `@src/functions/consolidation-pipeline.ts`:
- Line 53: Update the skip reason returned by the consolidation pipeline so it
lists all actual supported enablement paths; modify the return object in the
consolidation pipeline (the line returning { success: false, skipped: true,
reason: ... } in consolidation-pipeline.ts) to include GOOGLE_API_KEY,
OPENAI_BASE_URL and AGENTMEMORY_PROVIDER=agent-sdk alongside the existing LLM
provider keys (ANTHROPIC_API_KEY / OPENAI_API_KEY / OPENROUTER_API_KEY /
GEMINI_API_KEY / MINIMAX_API_KEY) and the CONSOLIDATION_ENABLED flag to keep
guidance accurate and consistent with the provider detection logic.
---
Nitpick comments:
In `@test/consolidation-default.test.ts`:
- Around line 70-104: Add a regression test in the same suite
(test/consolidation-default.test.ts) that uses writeEnv to set
OPENAI_API_KEY=sk-test-123 and OPENAI_API_KEY_FOR_LLM=false, calls
freshConfig(), and asserts cfg.isConsolidationEnabled() is false; place it
alongside the existing cases (referencing writeEnv, freshConfig, and
isConsolidationEnabled) to ensure consolidation remains off when the
LLM-specific flag disables usage even if OPENAI_API_KEY is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b587e81-251c-405b-baee-a2aa4bd0f813
📒 Files selected for processing (4)
README.mdsrc/config.tssrc/functions/consolidation-pipeline.tstest/consolidation-default.test.ts
Review fixes on #696: 1. hasLLMProviderConfigured() now scopes OPENAI_API_KEY to LLM use the same way detectProvider() does: the key only counts when OPENAI_API_KEY_FOR_LLM is not "false" (case-insensitive). Without this, a user who explicitly scoped their OpenAI key to embeddings only would still get consolidation enabled by default. Other keys (ANTHROPIC, OPENROUTER, GEMINI, GOOGLE, MINIMAX, OPENAI_BASE_URL) and AGENTMEMORY_PROVIDER=agent-sdk unchanged. 2. consolidation-pipeline skip-reason text now lists every env path that flips the default: ANTHROPIC_API_KEY / OPENAI_API_KEY / OPENROUTER_API_KEY / GEMINI_API_KEY / GOOGLE_API_KEY / MINIMAX_API_KEY / OPENAI_BASE_URL / AGENTMEMORY_PROVIDER=agent-sdk. Aligns with the actual detection logic so users see all valid ways to enable when they hit the skipped result. 3. Regression test in test/consolidation-default.test.ts asserts OPENAI_API_KEY=sk-... + OPENAI_API_KEY_FOR_LLM=false → isConsolidationEnabled() === false. Pairs with the existing OPENAI_API_KEY-on-by-default case to lock the contract.
rohitg00
added a commit
that referenced
this pull request
May 28, 2026
Bumps version across 9 files + adds CHANGELOG entry summarizing the 18 commits since v0.9.22. Highlights: - GitHub Copilot CLI first-class support (#534) — plugin + hooks + MCP with LSP-style Content-Length framing on the standalone stdio transport. - Five new MCP adapters: Warp, Cline, Continue, Zed, Droid (#677); ADAPTERS count 11 → 17. - Three silent DX bugs fixed: graph extraction never fired on session end (#666 / #698), status reported zero memories (#666), consolidation defaulted off even with an LLM provider configured (#612 / #696). - Nine telemetry hooks switched to fire-and-forget so they don't block Claude Code's next-prompt boundary (#573 / #688). - Hook project field now sends repo basename instead of full filesystem path so auto-injected context isn't silently filtered out (#474 / #687). - Local-LLM docs: Ollama / LM Studio / vLLM section added (#671 / #697). Version-bump files: package.json, plugin/.claude-plugin/plugin.json, plugin/plugin.json, plugin/.codex-plugin/plugin.json, packages/mcp/package.json, src/version.ts, src/types.ts, src/functions/export-import.ts, test/export-import.test.ts.
rohitg00
added a commit
that referenced
this pull request
May 28, 2026
* chore(release): v0.9.23 Bumps version across 9 files + adds CHANGELOG entry summarizing the 18 commits since v0.9.22. Highlights: - GitHub Copilot CLI first-class support (#534) — plugin + hooks + MCP with LSP-style Content-Length framing on the standalone stdio transport. - Five new MCP adapters: Warp, Cline, Continue, Zed, Droid (#677); ADAPTERS count 11 → 17. - Three silent DX bugs fixed: graph extraction never fired on session end (#666 / #698), status reported zero memories (#666), consolidation defaulted off even with an LLM provider configured (#612 / #696). - Nine telemetry hooks switched to fire-and-forget so they don't block Claude Code's next-prompt boundary (#573 / #688). - Hook project field now sends repo basename instead of full filesystem path so auto-injected context isn't silently filtered out (#474 / #687). - Local-LLM docs: Ollama / LM Studio / vLLM section added (#671 / #697). Version-bump files: package.json, plugin/.claude-plugin/plugin.json, plugin/plugin.json, plugin/.codex-plugin/plugin.json, packages/mcp/package.json, src/version.ts, src/types.ts, src/functions/export-import.ts, test/export-import.test.ts. * chore(release): add #701 + #709 to v0.9.23 CHANGELOG
|
Hmmm... but no graph or consolidation happens using Hermes or Open Code I'm using Linux. I'll wait the 0.9.23v to see if something changes. Thank you! |
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
2 participants
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.
Summary
Users with an LLM provider configured (Anthropic / OpenAI / OpenAI-compatible local endpoint via
OPENAI_BASE_URL/ OpenRouter / Gemini / Minimax / agent-sdk) were silently getting zero graph nodes, lessons, and crystals becauseCONSOLIDATION_ENABLEDdefaulted tofalse. The auto-consolidate pipeline only fires when the flag istrue, so 100+ session corpora sat unprocessed unless the user knew to flip the flag manually.Reported in discussion #612:
Compression worked (it's wired via
PostToolUseand only needs the provider). Consolidation didn't fire (gated behind the flag). Classic DX landmine pattern flagged in the feature-flag-visibility lesson — disabled-by-default LLM features that produce empty tabs.Changes
src/config.ts—isConsolidationEnabled()now treats unset as "on when a non-noop LLM provider is detectable from env". Detection: any ofANTHROPIC_API_KEY/OPENAI_API_KEY/OPENROUTER_API_KEY/GEMINI_API_KEY/GOOGLE_API_KEY/MINIMAX_API_KEY/OPENAI_BASE_URLis set, orAGENTMEMORY_PROVIDER=agent-sdk. Explicit=false/=0/AGENTMEMORY_PROVIDER=noopstill wins.src/functions/consolidation-pipeline.ts— skip-reason text updated from generic "CONSOLIDATION_ENABLED is not set to true" to point users at the env vars they should set.README.md— updated both occurrences ofCONSOLIDATION_ENABLED=trueso docs reflect the new default.test/consolidation-default.test.ts— 8 new tests covering: no-provider default-off, each supported provider default-on, explicit=falseoverride, explicit=trueoverride,AGENTMEMORY_PROVIDER=noopwins over API-key presence.Behavior matrix
ANTHROPIC_API_KEY=…(or any provider key)OPENAI_BASE_URL=…(local OpenAI-compatible)AGENTMEMORY_PROVIDER=agent-sdkAGENTMEMORY_PROVIDER=noopCONSOLIDATION_ENABLED=falseCONSOLIDATION_ENABLED=trueTest plan
npx vitest run test/consolidation-default.test.ts— 8/8 passnpx vitest run test/env-loader.test.ts test/consolidation-pipeline.test.ts— both green (existing assertions still hold)npx vitest run— 1276/1276 pass (integration.test.ts skipped, needs running server, pre-existing)npm run buildcleanCloses #612.
Summary by CodeRabbit