Skip to content

Address PR review feedback for CountTokensTask introduction - #218

Merged
sroussey merged 3 commits into
token-countingfrom
copilot/sub-pr-217
Feb 20, 2026
Merged

Address PR review feedback for CountTokensTask introduction#218
sroussey merged 3 commits into
token-countingfrom
copilot/sub-pr-217

Conversation

Copilot AI commented Feb 20, 2026

Copy link
Copy Markdown
Contributor

Fixes correctness issues, silent failures, and missing test coverage flagged in the review of the CountTokensTask PR.

Bug fixes

  • Infinite loop protection (HierarchicalChunkerTask): Upfront validation now throws if reservedTokens >= maxTokensPerChunk or overlapTokens >= effectiveMaxTokens; the chunking loop also clamps endOffset = Math.max(boundary, startOffset + 1) to guarantee forward progress even when a single character exceeds the token budget
  • Silent CountTokensTask failures (HierarchicalChunkerTask): Wrapped provider calls in try/catch to fall back to estimateTokens instead of propagating errors from unsupported providers
  • Tiktoken encoder hotspot (OpenAI_JobRunFns): Replaced per-call create/free with a module-level _encoderCache map keyed by model name; unknown models share the cached cl100k_base encoder

Schema corrections

  • HierarchicalChunkerTask: Added doc_id to required (was runtime-validated but invisible to schema consumers)
  • ContextBuilderTask: totalTokens description corrected to "Estimated token count"; docstring purged of references to non-existent countTokensModel/buildCountTokensFn
  • HierarchicalChunkerTask: Docstring updated to reference CountTokensTask and estimateTokens accurately

Test coverage

Added focused unit tests for the three new provider run functions:

  • OpenAI_CountTokens — verifies known-model encoder selection and cl100k_base fallback path
  • Anthropic_CountTokens — asserts messages.countTokens call shape and input_tokens → count mapping
  • Gemini_CountTokens — asserts countTokens call and totalTokens → count mapping

Test regression fix

Restored the rate-limiter assertion to >= 12 (was loosened to >= 10 in the original PR).


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: sroussey <127349+sroussey@users.noreply.github.com>
Copilot AI changed the title [WIP] Add support for token counting in AI tasks Address PR review feedback for CountTokensTask introduction Feb 20, 2026
Copilot AI requested a review from sroussey February 20, 2026 06:24
@sroussey
sroussey marked this pull request as ready for review February 20, 2026 06:24
Comment thread packages/ai/src/task/CountTokensTask.ts Outdated
import { TypeModel } from "./base/AiTaskSchemas";

const modelSchema = TypeModel("model");
const modelSchema = TypeModel("model:CountTokensTask");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

revert, there are no models that register that way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reverted in 8d7687cCountTokensTask is back to TypeModel("model").

Co-authored-by: sroussey <127349+sroussey@users.noreply.github.com>
Copilot AI requested a review from sroussey February 20, 2026 06:28
@sroussey
sroussey merged commit 1ae87f1 into token-counting Feb 20, 2026
0 of 2 checks passed
@sroussey
sroussey deleted the copilot/sub-pr-217 branch February 25, 2026 22:05
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