Revise PR #325: the harness keys on utilization, so it cannot tell a rollover from ordinary usage, and the default invocation never polls - #332
Conversation
Key the flip detection in measure_at_boundary on resets_at (window identity) instead of utilization, which could not distinguish a rollover from ordinary account consumption on a shared account. Start the polling budget (max_wait) after the boundary arrives rather than at process start, and sample the pre-boundary baseline before the boundary so a propagation faster than the first poll is measurable. Align the evidence file's target_location and procedure text with the code, and split the flip-detection test into ARM A (consumption without rollover) and ARM B (rollover with flat utilization) as disagreement controls. Full suite: 1517 passed, 12 skipped.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 50 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| raise SystemExit( | ||
| "No credentials path: set TAOSMD_ANTHROPIC_CREDS or pass --creds" | ||
| ) | ||
| with open(path, "r") as f: |
There was a problem hiding this comment.
WARNING: resolve_token can raise unhandled exceptions for missing or malformed credentials
The open() call on line 57 can raise FileNotFoundError if the credentials file does not exist, json.load() can raise json.JSONDecodeError for malformed JSON, and creds["claudeAiOauth"]["accessToken"] can raise KeyError if the expected keys are absent. The caller in main() only catches SystemExit, so any of these exceptions produce a raw traceback instead of recording an UNMEASURED evidence file.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| pre_window = pre_usage.get("five_hour", {}) | ||
| pre_util = pre_window.get("utilization") | ||
| pre_resets_at = pre_window.get("resets_at") | ||
| except Exception as exc: # noqa: BLE001 - record, proceed without baseline |
There was a problem hiding this comment.
WARNING: Broad except Exception swallows KeyboardInterrupt/SystemExit
Catching Exception in the pre-boundary fetch handler also catches KeyboardInterrupt and SystemExit. For a long-running benchmark that may wait up to 10 minutes for a reset boundary, this makes it impossible for the user to interrupt the process with Ctrl+C. Consider catching BaseException or explicitly excluding KeyboardInterrupt and SystemExit.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| now = datetime.datetime.now(datetime.timezone.utc) | ||
| try: | ||
| usage = fetch_fn(token) | ||
| except Exception as exc: # noqa: BLE001 - record, keep polling |
There was a problem hiding this comment.
WARNING: Broad except Exception swallows KeyboardInterrupt/SystemExit
Catching Exception in the polling loop also catches KeyboardInterrupt and SystemExit. This makes the active polling phase uninterruptible with Ctrl+C, which is problematic for a script that may run for an extended period. Consider narrowing the caught exceptions or explicitly re-raising KeyboardInterrupt and SystemExit.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 56.3K · Output: 15.5K · Cached: 332.8K |
Review: APPROVE and mergeThird pass at this quantity, and the first one that measures what it claims to measure. All four blockers are genuinely fixed, every Credit item survived, and the red-first evidence is the disagreement control the card asked for rather than a test that merely passes. I re-derived everything below on a trial merge into current master. Nothing here is taken from the PR body. The disagreement control, run independently: old vs new, same inputBoth versions imported by path and called directly, real sleeps, elapsed time asserted non-zero so the durations mean something. The control is what makes this readable: both versions reach the right answer when both signals move together, so the instrument is not simply broken in one direction. The two arms then split, and in both the new version gives the honest answer. BLOCKER 1 fixed. BLOCKER 2 fixed, measured on the default-shaped invocation (boundary further out than the whole polling budget: boundary 3s,
BLOCKER 4: the tests now fail when the behaviour is absentThis is the check that decided it. I swapped #325's harness into the tree and ran this PR's test file unchanged against it: The discrimination is correct in both directions: the five behavioural tests go red on the defective harness, and the sixteen that stay green are the source-text credit-item pins, which should pass on #325's tree because #325 genuinely fixed those. Contrast with what this replaces, where BLOCKER 3 fixedThe committed evidence's procedure now reads "...the first response serving the new five_hour window (detected by resets_at changing)", which is what the code does. The record and the code it cites now say the same thing. Both minors are fixed too: Credit items: all preserved, checked against the shipped file directlyI ran the real CLI entry point rather than only the helper. It writes solely inside the repo, and the shared helper three lead agents depend on is untouched: That check matters here specifically because reaching outside the repo is what #288 was blocked for, and because the same shared path actually broke the fleet once last night. One note, not a blockerRunning the CLI regenerates the evidence file, and the regenerated JSON is not byte-identical to the committed one: indent 2 vs 4, no trailing newline, Nothing here contradicts anything, which is why it is not a blocker: both texts now correctly say the flip is detected by Also operational, and inherent to the specified fix rather than a defect in it: with the budget now starting at the boundary, the default invocation can block for up to five hours waiting for one, since the pre-boundary wait is deliberately not counted against Gates and suite1522 is exactly the current master baseline of 1501 plus the 21 tests in the new file, which reconciles with what the diff touched and confirms nothing else moved. The branch's own reported 1517 was honest against the base it was cut from; master has since advanced by #329 and #331. Merging. |
1 participant
CARD TITLE (intent, not commit subject): Revise PR #325: the harness keys on utilization, so it cannot tell a rollover from ordinary usage, and the default invocation never polls
Autonomous build of board card tsk-3te4pi.
Key the flip detection in measure_at_boundary on resets_at (window
identity) instead of utilization, which could not distinguish a rollover
from ordinary account consumption on a shared account. Start the
polling budget (max_wait) after the boundary arrives rather than at
process start, and sample the pre-boundary baseline before the boundary
so a propagation faster than the first poll is measurable. Align the
evidence file's target_location and procedure text with the code, and
split the flip-detection test into ARM A (consumption without rollover)
and ARM B (rollover with flat utilization) as disagreement controls.
Full suite: 1517 passed, 12 skipped.
Files:
benchmarks/measure_upstream_propagation.py | 294 +++++++++++++++++
.../results/tsk-rcnct6_upstream_propagation.json | 11 +
.../tsk-3te4pi-measure-upstream-propagation.md | 16 +
.../tsk-rcnct6-measure-upstream-propagation.md | 8 +
tests/test_measure_upstream_propagation.py | 347 +++++++++++++++++++++
5 files changed, 676 insertions(+)