fix: standardize token estimation heuristic between file_operations and base_agent#249
Conversation
📝 WalkthroughWalkthroughA new test suite validates token estimation consistency by comparing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code_puppy/tools/file_operations.py`:
- Around line 516-517: The token estimation in file_operations.py currently
computes num_tokens = math.floor(len(content) / 2.5) which can yield 0 for short
content; update this to mirror BaseAgent.estimate_token_count by enforcing a
minimum of 1 (e.g., set num_tokens = max(1, math.floor(len(content) / 2.5))) so
short strings never produce zero tokens and remain consistent with BaseAgent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63969bdc-03a9-4258-b293-86ee402d7316
📒 Files selected for processing (1)
code_puppy/tools/file_operations.py
RyanLHicks
force-pushed
the
fix/standardize-token-estimation-heuristic
branch
from
b1e4f82 to
463d7c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/agents/test_token_estimation_consistency.py (2)
1-10: Consider importing and testingfile_operations._read_filedirectly.The test suite claims to verify consistency between
file_operations._read_fileandBaseAgent.estimate_token_count, but only imports and testsCodePuppyAgent. If someone later modifies the token estimation infile_operations.py, these tests won't catch the regression.Consider importing the actual file_operations function (or extracting the shared heuristic into a common utility) and asserting both implementations produce identical results.
💡 Suggested approach
import math from code_puppy.agents.agent_code_puppy import CodePuppyAgent +from code_puppy.tools.file_operations import _estimate_tokens # or similar shared functionAlternatively, extract the heuristic into a shared utility function that both modules import, then test that single source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/agents/test_token_estimation_consistency.py` around lines 1 - 10, The test currently only imports CodePuppyAgent; update the test to import file_operations._read_file (or the shared heuristic utility if you extracted one) and assert its token estimate matches BaseAgent.estimate_token_count (or CodePuppyAgent.estimate_token_count) for the same inputs; specifically, add an import for file_operations._read_file and include assertions that file_operations._read_file(...) output (or the heuristic function result) produces the same chars-per-token based estimate as BaseAgent.estimate_token_count (or CodePuppyAgent.estimate_token_count) for the sample files/strings used in the test so changes in file_operations are detected.
16-27: Variable namefile_ops_estimateis misleading.The variable
file_ops_estimateis computed from a hardcoded formula, not from the actualfile_operationsmodule. This naming could mislead future maintainers into thinking the test verifies the real implementation.✏️ Suggested rename for clarity
base_agent_estimate = agent.estimate_token_count(content) - file_ops_estimate = math.floor(len(content) / 2.5) + expected_heuristic = math.floor(len(content) / 2.5) - assert base_agent_estimate == file_ops_estimate + assert base_agent_estimate == expected_heuristic🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/agents/test_token_estimation_consistency.py` around lines 16 - 27, Rename the misleading variable file_ops_estimate in test_estimate_token_count_matches_file_operations_heuristic to reflect that it is derived from the hardcoded 2.5 chars/token formula (e.g., heuristic_estimate or expected_estimate); update the variable name wherever used and keep the calculation math.floor(len(content) / 2.5) and the assertion comparing agent.estimate_token_count(content) to that renamed variable so the test clearly documents it's validating the 2.5 chars/token heuristic rather than calling file_operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/agents/test_token_estimation_consistency.py`:
- Around line 45-52: Docstring and assertion are inconsistent: update the test
for CodePuppyAgent. Change the docstring to state that CodePuppyAgent enforces a
minimum of 1 token, and tighten the assertion to expect exactly 1 by replacing
"assert result >= 1" with "assert result == 1" when calling
CodePuppyAgent().estimate_token_count("") (referencing CodePuppyAgent and
estimate_token_count).
---
Nitpick comments:
In `@tests/agents/test_token_estimation_consistency.py`:
- Around line 1-10: The test currently only imports CodePuppyAgent; update the
test to import file_operations._read_file (or the shared heuristic utility if
you extracted one) and assert its token estimate matches
BaseAgent.estimate_token_count (or CodePuppyAgent.estimate_token_count) for the
same inputs; specifically, add an import for file_operations._read_file and
include assertions that file_operations._read_file(...) output (or the heuristic
function result) produces the same chars-per-token based estimate as
BaseAgent.estimate_token_count (or CodePuppyAgent.estimate_token_count) for the
sample files/strings used in the test so changes in file_operations are
detected.
- Around line 16-27: Rename the misleading variable file_ops_estimate in
test_estimate_token_count_matches_file_operations_heuristic to reflect that it
is derived from the hardcoded 2.5 chars/token formula (e.g., heuristic_estimate
or expected_estimate); update the variable name wherever used and keep the
calculation math.floor(len(content) / 2.5) and the assertion comparing
agent.estimate_token_count(content) to that renamed variable so the test clearly
documents it's validating the 2.5 chars/token heuristic rather than calling
file_operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06e0f0cb-3c6d-4426-a123-7e1d7d1bb9cb
📒 Files selected for processing (1)
tests/agents/test_token_estimation_consistency.py
| def test_minimum_token_count_is_one(self): | ||
| """ | ||
| Both estimators enforce a minimum of 1 token even for empty content. | ||
| """ | ||
| agent = CodePuppyAgent() | ||
|
|
||
| result = agent.estimate_token_count("") | ||
| assert result >= 1 No newline at end of file |
There was a problem hiding this comment.
Docstring claims to test "Both estimators" but only tests one.
The docstring states "Both estimators enforce a minimum of 1 token" but the test only verifies BaseAgent.estimate_token_count. Additionally, the assertion >= 1 is weaker than necessary—the implementation (per context snippet) returns exactly 1 for empty input via max(1, ...).
🔧 Suggested fix
def test_minimum_token_count_is_one(self):
"""
- Both estimators enforce a minimum of 1 token even for empty content.
+ estimate_token_count enforces a minimum of 1 token even for empty content.
"""
agent = CodePuppyAgent()
result = agent.estimate_token_count("")
- assert result >= 1
+ assert result == 1Or better, actually test both estimators as the original docstring intended.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_minimum_token_count_is_one(self): | |
| """ | |
| Both estimators enforce a minimum of 1 token even for empty content. | |
| """ | |
| agent = CodePuppyAgent() | |
| result = agent.estimate_token_count("") | |
| assert result >= 1 | |
| def test_minimum_token_count_is_one(self): | |
| """ | |
| estimate_token_count enforces a minimum of 1 token even for empty content. | |
| """ | |
| agent = CodePuppyAgent() | |
| result = agent.estimate_token_count("") | |
| assert result == 1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/agents/test_token_estimation_consistency.py` around lines 45 - 52,
Docstring and assertion are inconsistent: update the test for CodePuppyAgent.
Change the docstring to state that CodePuppyAgent enforces a minimum of 1 token,
and tighten the assertion to expect exactly 1 by replacing "assert result >= 1"
with "assert result == 1" when calling CodePuppyAgent().estimate_token_count("")
(referencing CodePuppyAgent and estimate_token_count).
|
Closing — the token estimation fix and tests are included in |
1 participant
Uh oh!
There was an error while loading. Please reload this page.