fix(plugin): declare llm.maxTokens and llm.headers as first-class config keys - #2248
fix(plugin): declare llm.maxTokens and llm.headers as first-class config keys#2248kiwipaulrob wants to merge 4 commits into
Conversation
✅ Automated Test Results: PASSEDAll tests passed (7/7 executed). memos_local_plugin/unit: 7/7. Duration: 3s Branch: |
…fig keys llm.maxTokens and llm.headers are read at runtime (client.ts reads config.maxTokens, providers spread config.headers) but were absent from DEFAULT_CONFIG and LlmSchema/SkillEvolverSchema, so every boot logged "unknown config key 'llm.maxTokens'" and "unknown config key 'llm.headers.<key>'" (pruneUnknown recursed into the empty headers slot and warned per user key). Add both to defaults + schema, and teach pruneUnknown that an empty-object default slot is a free-form map that must be kept as-is, eliminating the per-key warnings. Adds a regression test covering acceptance, defaults, range validation and the free-form-map warning suppression.
4b539bb to
d59fe83
Compare
🤖 Open Code ReviewTarget: PR #2248 🔍 OpenCodeReview found 1 issue(s) in this PR. 1.
|
✅ Automated Test Results: PASSEDAll tests passed (7/7 executed). memos_local_plugin/unit: 7/7. Duration: 3s [advisory, non-gating] AI-generated tests on branch test/auto-gen-e795d6b24525860c-20260815034233: 72/72 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
…ed-slot defaults to 4096 Addresses OpenCodeReview feedback on MemTensor#2248: - The l3Llm and skillEvolver client builders constructed their clients with explicit field picks that dropped maxTokens and headers — the config keys declared by the previous commit were inert at runtime (effective cap was the hard-coded DEFAULT_MAX_TOKENS=1024 in client.ts regardless of config). - Add maxTokens+headers to DedicatedLlmConfig and pass both through in the reflectLlm (skillEvolver) and l3Llm builders so configured values actually reach the provider request. - Add headers to SkillEvolverSchema (l3Llm/skillEvolver slots) so custom HTTP headers are accepted on those slots, mirroring the llm slot. - Raise l3Llm/skillEvolver maxTokens defaults from 1024 to 4096: L3 world- model bodies span multiple L2 policies/evidence traces, and crystallized skill bodies include invocation guides + procedure steps — 1024 tokens risks silent truncation on both workloads (both slots already assume 60s timeouts, implying heavier calls). - Update config tests: default assertions now pin 4096, plus new coverage for l3Llm.maxTokens and headers on both dedicated slots.
|
Thanks for the review — all three findings are addressed in 1078640. 1. headers on SkillEvolverSchema — accepted and extended: 2. l3Llm.maxTokens default 1024 → 4096 — accepted. L3 world-model bodies are generated from multiple L2 policies and evidence traces; 1024 was a real truncation risk. Now wired AND defaulted to 4096. 3. skillEvolver.maxTokens default 1024 → 4096 — accepted. Crystallized skill bodies include invocation guides + structured procedure steps; same truncation risk, same fix (wired + 4096). Tests updated: config suite now pins the 4096 defaults and adds coverage for l3Llm.maxTokens and headers on both dedicated slots (70/70 pass in tests/unit/config). Typecheck clean. Pipeline test failures in this environment are pre-existing at the base commit (71 failed before and after), unrelated to this change. |
✅ Automated Test Results: PASSEDAll tests passed (50/50 executed). memos_local_plugin/unit: 50/50. Duration: 11s [advisory, non-gating] AI-generated tests on branch test/auto-gen-4e07394653d2e46a-20260815040834: 0/98 passed, 98 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
…s floor 100 Addresses remaining OpenCodeReview feedback on MemTensor#2248: headers was declared on SkillEvolverSchema but absent from the l3Llm/skillEvolver defaults, so setting those keys in YAML still warned unknown config key and bypassed the pruneUnknown free-form-map shortcut; maxTokens floor raised 16 to 100 to match the documented deepseek-v4-flash constraint; dedicated-slot headers now asserted warning-free, defaults pinned, out-of-range regression pinned at 50.
|
Addressed the remaining OpenCodeReview findings on the new head ( 1. maxTokens floor 16 → 100 — accepted. Both 4 + 5. 2 + 3. Arbitrary headers could override managed headers ( Validation on the new head: |
✅ Automated Test Results: PASSEDAll tests passed (51/51 executed). memos_local_plugin/unit: 51/51. Duration: 11s Branch: |
Hun-ger
left a comment
There was a problem hiding this comment.
Thanks for the updates. This PR addresses a valid configuration issue, and the runtime wiring fix for l3Llm and skillEvolver is especially valuable. However, I don’t think it is ready to merge yet.
Before merging, please:
- Redact sensitive values in
llm.headers,l3Llm.headers, andskillEvolver.headersfrom the config API, with GET/PATCH round-trip tests. - Add runtime regression tests verifying that
maxTokensandheadersactually reach both shared and dedicated LLM clients. - Align the dedicated-slot schema default with the
4096value inDEFAULT_CONFIG. - Replace the implicit “empty object means free-form map” convention with an explicit allowlist or marker.
Marking this as changes requested for now. Thanks again for the thoughtful fixes so far.
Summary
llm.maxTokensandllm.headersare read at runtime (LLM client resolvesconfig.maxTokenswith a 1024 fallback; every provider spreadsconfig.headersinto requests) but are absent fromDEFAULT_CONFIGand the config schema. Every boot logsunknown config key 'llm.maxTokens', and onceheadersis present,pruneUnknown()recurses into the empty default slot and warns for every user header key (unknown config key 'llm.headers.User-Agent').This PR declares both keys as first-class config, adds defaults, and fixes
pruneUnknown()so empty-object default slots are treated as free-form maps.Change
core/config/defaults.ts— addmaxTokens: 1024+headers: {}to thellmtree; addmaxTokens: 1024toskillEvolverandl3Llm(both shareSkillEvolverSchema).core/config/schema.ts— declaremaxTokens(range 16–131072, default 1024) +headers(Record<string, string>) inLlmSchema; declaremaxTokensinSkillEvolverSchema.core/config/index.ts—pruneUnknown(): an empty-object default slot is a free-form map (Record<string, string>), so keep the whole user object as-is instead of recursing and warning per key.tests/unit/config/llm-max-tokens-headers.test.ts— new regression suite: acceptance without warnings, defaults, range validation, non-string header rejection, unrelated-field preservation.Tests
npx vitest run tests/unit/config tests/unit/llm→ 148 passed (10 files)npx tsc -p tsconfig.json --noEmit→ clean (exit 0)Related
Fixes #2247
Environment
Type of change
How Tested
npx vitest run tests/unit/config tests/unit/llm— 148 passednpx tsc -p tsconfig.json --noEmit— 0 errorsresolveConfig({ llm: { maxTokens: 2048, headers: { "User-Agent": "test" } } })returns both values with zero warnings;resolveConfig({})yieldsmaxTokens: 1024,headers: {}Checklist