Repository navigation
Python: fix(lab-tau2): check the over-max warning before the half-of-max warning - #9341
Conversation
…max warning The half-of-max branch ran first and also matched every over-max case, so the red truncation warning was dead code. Swap the branch order. Fixes microsoft#9338 Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
python
|
/review |
GitHub Actions
Active
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (2 commit(s)): 624972ec549c, 0d71d4a447a7
Model: gpt-5.6-sol
Overview
The PR correctly makes the over-budget warning reachable by checking it before the overlapping half-budget condition. The strict truncation threshold and stored-history behavior remain unchanged, while the regression test demonstrates that an over-budget count emits the intended warning. No publishable correctness, compatibility, architectural, or security defect was established.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
|
/review |
Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
GitHub Actions
Active
|
The lab failures were a lint nit on my new test, one long literal. Wrapped and pushed 2faf47a. |
GitHub Actions
Active
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Manohar Paturi (@ManoharPaturi) python/packages/lab/tau2/tests/test_sliding_window.py:241 calls logger.remove() without a handler id before the assertion and again in finally. Because this is the real process-global Loguru logger, the test removes every sink for the entire pytest process and leaves later tests with mutated logging state, making the suite order-dependent.
Please avoid removing global handlers. Patch agent_framework_lab_tau2._sliding_window.logger, or retain the id returned by logger.add(...) and remove only that sink in finally.
Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
GitHub Actions
Active
|
Good catch, the test was stripping every sink for the whole process. 1bacfe2 adds its own sink and removes only that handler id in finally, so the suite's logging state stays intact. Full tau2 suite 38 passed. |
Motivation & Context
In
SlidingWindowHistoryProvider._get_token_countthe red "over max tokens, will truncate" warning could never fire. The yellow half-of-max check ran first and also matched every over-max case, so the branch was dead code and the truncation warning was silently lost. Issue #9338 has the repro.Description & Review Guide
Related Issue
Fixes #9338
AI Assistance
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.