Skip to content

Add tokenizer tests and fix auto-compact calculation to account for max_tokens #7342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Aug 23, 2025

Conversation

sestinj
Copy link
Contributor

@sestinj sestinj commented Aug 22, 2025

Summary by cubic

Fix auto-compact to properly account for model max_tokens and prevent context overflow. Add unit tests for tokenizer functions and compaction logic.

  • Bug Fixes

    • Reserve output tokens using maxTokens; default to 35% when unset.
    • Compute usage against available input (contextLength - reservedForOutput) and respect the 80% threshold.
    • Throw when maxTokens >= contextLength.
    • Improve debug logging with input, limits, and decision details.
  • Tests

    • Add tokenizer.test.ts covering message/chat token counts, usage %, and compaction with/without maxTokens, including edge cases.
    • Include real-world scenarios (e.g., Claude 200k with 4k max, GPT-4 128k with 4k) and mocks for logger/tokenizer.

@sestinj sestinj requested a review from a team as a code owner August 22, 2025 18:19
@sestinj sestinj requested review from Patrick-Erichsen and removed request for a team August 22, 2025 18:19
Copy link

⚠️ PR Title Format

Your PR title doesn't follow the conventional commit format, but this won't block your PR from being merged. We recommend using this format for better project organization.

Expected Format:

<type>[optional scope]: <description>

Examples:

  • feat: add changelog generation support
  • fix: resolve login redirect issue
  • docs: update README with new instructions
  • chore: update dependencies

Valid Types:

feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert

This helps with:

  • 📝 Automatic changelog generation
  • 🚀 Automated semantic versioning
  • 📊 Better project history tracking

This is a non-blocking warning - your PR can still be merged without fixing this.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 22, 2025
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files

React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.


// Ensure we have positive space available for input
if (availableForInput <= 0) {
throw new Error(`max_tokens is larger than context_length, which should not be possible. Please check your configuration.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error message is misleading when triggered by small context limits with default 35% reservation

Prompt for AI agents
Address the following comment on extensions/cli/src/util/tokenizer.ts at line 140:

<comment>Error message is misleading when triggered by small context limits with default 35% reservation</comment>

<file context>
@@ -117,20 +117,37 @@ export function calculateContextUsagePercentage(
 /**
  * Check if the chat history exceeds the auto-compact threshold
  * @param chatHistory The chat history to check
- * @param modelName The model name
+ * @param model The model configuration
  * @returns Whether auto-compacting should be triggered
  */
 export function shouldAutoCompact(
   chatHistory: ChatCompletionMessageParam[],
</file context>

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Aug 23, 2025
@sestinj sestinj merged commit 7e30a63 into main Aug 23, 2025
80 of 84 checks passed
@sestinj sestinj deleted the nate-3 branch August 23, 2025 18:14
@github-project-automation github-project-automation bot moved this from Todo to Done in Issues and PRs Aug 23, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant