Skip to content

feat: harden chat history and add tests#167

Open
Eruis2579 wants to merge 4 commits intovirattt:mainfrom
Eruis2579:feat-history-hardening
Open

feat: harden chat history and add tests#167
Eruis2579 wants to merge 4 commits intovirattt:mainfrom
Eruis2579:feat-history-hardening

Conversation

@Eruis2579
Copy link

Summary

Hardens chat history utilities with history limits and adds test coverage for history-context and long-term-chat-history.

Changes

buildHistoryContext (history-context.ts)

  • Introduced BuildHistoryContextParams interface for a clearer API
  • Added optional historyLimit parameter (default DEFAULT_HISTORY_LIMIT, 10)
  • When historyLimit > 0, only the last N entries are used in the prompt
  • When historyLimit <= 0, all entries are included (no trimming)

LongTermChatHistory (long-term-chat-history.ts)

  • Added maxEntries (default 200) to cap stored messages
  • Constructor now accepts optional options?: { maxEntries?: number }
  • load(): rewrites file on parse error and trims to maxEntries if needed
  • addUserMessage(): trims messages to maxEntries after adding a new entry
  • Avoids unbounded growth of .dexter/messages/chat_history.json

Tests

  • history-context.test.ts: Tests empty history, formatting, markers, custom line breaks, default limit, explicit historyLimit override, and disabling limit with historyLimit <= 0
  • long-term-chat-history.test.ts: Tests creation of history file, stack ordering, updateAgentResponse, deduplication in getMessageStrings, maxEntries on add, and trimming on load

Backward Compatibility

  • Existing buildHistoryContext({ entries, currentMessage }) calls behave as before (default limit of 10)
  • Existing LongTermChatHistory(baseDir) usage is unchanged (default maxEntries of 200)

Testing

bun test

@Eruis2579
Copy link
Author

Hi, @virattt, Could you review my PR, Please?

…erm-chat-history, adopt upstream debugLog in outbound
@Eruis2579 Eruis2579 force-pushed the feat-history-hardening branch from c426271 to 2aeb499 Compare March 10, 2026 20:27
@Eruis2579
Copy link
Author

Hi, @virattt, Could you review my PR, Please?

@Eruis2579
Copy link
Author

@virattt, Please review my PR. Thank you

1 similar comment
@Eruis2579
Copy link
Author

@virattt, Please review my PR. Thank you

@virattt
Copy link
Owner

virattt commented Mar 14, 2026

Thanks for the contribution — nice additions, especially the test coverage and the maxEntries cap on LongTermChatHistory. I spotted a couple of issues worth fixing before we merge.

buildHistoryContext default limit silently halves context

The new DEFAULT_HISTORY_LIMIT (10) in buildHistoryContext counts entries, but the same constant is already used by InMemoryChatHistory.getRecentTurns() to limit turns. Each turn produces two entries (user + assistant), so:

  1. getRecentTurns(10) → up to 20 HistoryEntry items
  2. buildHistoryContext now slices to the last 10 entries → 5 turns

The call in agent.ts doesn't pass historyLimit, so it picks up the default and quietly drops half the conversation context. Easiest fix is probably to pass historyLimit: 0 at that call site, or remove the default from buildHistoryContext entirely and let callers opt in.

maxEntries doc says <= 0 means "no limit", code says otherwise

The JSDoc on the constructor option says values <= 0 are treated as "no limit", but the implementation falls through to DEFAULT_MAX_ENTRIES (200):

this.maxEntries =
  typeof requestedMax === 'number' && requestedMax > 0
    ? Math.floor(requestedMax)
    : DEFAULT_MAX_ENTRIES;

Either update the doc or add a code path that actually disables trimming when <= 0 is passed.

save() on parse error overwrites corrupt file

Currently, a corrupt chat_history.json just resets in-memory state without touching the file — users could still recover manually. The PR adds await this.save() in the catch block, which silently replaces the file with []. This is a judgment call, but worth being intentional about.

Minor style nits

  • Both test files end the describe block with }) instead of }); (project uses semicolons everywhere).
  • The PR changes .map(entry => to .map((entry) => in history-context.ts — the rest of the codebase uses the no-parens style for single params.
  • The long-term history test writes to a relative .dexter-long-term-history-tests dir; os.tmpdir() would be more robust.

Issue Severity
Default historyLimit halves agent context Bug — needs fix
maxEntries doc/code mismatch Minor bug
save() on corrupt file Design choice, worth discussing
Semicolons / arrow parens / test dir Nit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants