🔊 fix: warn when summarization.trigger.type is unrecognized#108
🔊 fix: warn when summarization.trigger.type is unrecognized#108danny-avila merged 2 commits intomainfrom
summarization.trigger.type is unrecognized#108Conversation
`shouldTriggerSummarization` silently returned `false` whenever the configured `trigger.type` was not one of the three it implements (`token_ratio`, `remaining_tokens`, `messages_to_refine`). That turned any upstream schema/SDK drift into a silent no-op — summarization just never fired, with no signal to the operator. Emit a one-time `console.warn` per unrecognized type so the misconfiguration surfaces in logs without spamming every agent turn. Behavior is unchanged for all valid trigger types and for the no-trigger-configured default.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52ee91ac74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (warnedUnrecognizedTriggerTypes.has(type)) { | ||
| return; | ||
| } | ||
| warnedUnrecognizedTriggerTypes.add(type); |
There was a problem hiding this comment.
warnedUnrecognizedTriggerTypes is process-global and every previously unseen trigger.type is retained forever via warnedUnrecognizedTriggerTypes.add(type). Because SummarizationTrigger.type accepts arbitrary strings and this path runs on normal request flow, deployments that ingest dynamic or user-provided agent configs can accumulate unbounded unique keys over time, creating a memory-growth vector in long-lived workers. Consider bounding this cache (size cap/LRU) or using a time-based eviction policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d60ec1e — the dedup set is now capped at 32 entries with FIFO eviction of the oldest key on overflow (Set preserves insertion order). Added a regression test that floods the function with 500 unique unrecognized types and verifies the dedup contract still holds.
Per Codex review: the dedup `Set` for warned trigger types was process-global and grew unboundedly. Consumers that thread dynamic or user-provided values through `trigger.type` could accumulate unique keys in long-lived workers — a minor but real memory vector. Cap the set at 32 entries and evict the oldest on overflow (Set preserves insertion order, so this is FIFO-ish). 32 is well above the handful of typos a legitimate deployment would ever see, and the dedup contract still holds: we never warn twice for a type that is currently in the set.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Defense-in-depth companion to LibreChat#12735, which fixes an upstream Zod schema that was rejecting all three documented
summarization.trigger.typevalues.Today,
shouldTriggerSummarizationsilently returnsfalsefor anytrigger.typethat isn't one oftoken_ratio,remaining_tokens, ormessages_to_refine. That turned the recent LibreChat schema/docs/runtime drift into a silent no-op — summarization just never fired, with no signal to the operator.This PR emits a one-time
console.warnper unrecognized type so the misconfiguration surfaces in logs without spamming every agent turn:Behavior is unchanged for every valid trigger type and for the no-trigger-configured default.
The helper is deduped via a module-level
Set<string>so an admin who deploys a typo doesn't get one warning per LLM round-trip. An internal_resetUnrecognizedTriggerWarningsexport is provided for test reset.Test plan
src/summarization/__tests__/trigger.test.tscovers:falsenpx jest src/summarization/__tests__/trigger.test.ts— 5 passed.npx tsc --noEmit— clean.npx eslint src/summarization/index.ts src/summarization/__tests__/trigger.test.ts— 0 errors (1 expectedno-consolewarning, consistent with existingconsole.warnusage insrc/graphs/Graph.ts).