fix(tracker): scope review time estimates to the provider in use #83
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/duration-estimate-per-provider"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
EstimateDuration regressed duration_ms against diff_chars over every
completed review row, with no provider filter, and widened from
tier-scoped to all tiers when a tier was short on data. With A/B fan-out
running several model configs against every PR, that pool mixes
providers whose throughput differs by an order of magnitude, so the
"a quick scan like this typically takes X" line in the pending comment
described an average across inference services rather than the one
actually doing the work.
Confine the sample to the primary config's provider and never fall back
across providers: widen from exact config + tier, to exact config, to
provider + tier, to provider, then give up and let the caller print the
existing "not enough data" line. The config-to-provider step matters on
first boot, where FallbackConfig mints a fresh UUID per run and so has
no config-level history to draw on.
Two population fixes fall out of the same read:
Restrict to primary runs. recordConfigResult stores whole-pipeline
wall time on primary rows but LLM-call time alone on shadow rows;
pooling them averaged two different measurements, and the primary
one is what a PR reader is waiting on.
Drop truncated runs. A generation bounded by max_tokens says
nothing about how long the diff takes, as reviewer.IsTruncated
already documents. Filtered in Go rather than SQL because migration
000009 stores finish_reason unnormalized across two wire
vocabularies and directs callers to that predicate instead of
literals. This adds tracker -> reviewer; reviewer is a leaf, so
there is no cycle.
Scope selection and query construction are split into durationScopes and
durationQuery so the filter composition is testable without a database,
which the package has no harness for.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
EstimateDuration regressed duration_ms against diff_chars over every completed review row, with no provider filter, and widened from tier-scoped to all tiers when a tier was short on data. With A/B fan-out running several model configs against every PR, that pool mixes providers whose throughput differs by an order of magnitude, so the "a quick scan like this typically takes X" line in the pending comment described an average across inference services rather than the one actually doing the work. Confine the sample to the primary config's provider and never fall back across providers: widen from exact config + tier, to exact config, to provider + tier, to provider, then give up and let the caller print the existing "not enough data" line. The config-to-provider step matters on first boot, where FallbackConfig mints a fresh UUID per run and so has no config-level history to draw on. Two population fixes fall out of the same read: - Restrict to primary runs. recordConfigResult stores whole-pipeline wall time on primary rows but LLM-call time alone on shadow rows; pooling them averaged two different measurements, and the primary one is what a PR reader is waiting on. - Drop truncated runs. A generation bounded by max_tokens says nothing about how long the diff takes, as reviewer.IsTruncated already documents. Filtered in Go rather than SQL because migration 000009 stores finish_reason unnormalized across two wire vocabularies and directs callers to that predicate instead of literals. This adds tracker -> reviewer; reviewer is a leaf, so there is no cycle. Scope selection and query construction are split into durationScopes and durationQuery so the filter composition is testable without a database, which the package has no harness for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Automated review by pr-reviewer v0.41.0 | Safety Check | Mistral Small | tracking id
r-667de1-b7c5dfThis is an AI-generated review and may contain mistakes.
Status: ✅ Completed
✅ Verdict: LGTM — No obvious bugs, security issues, accidentally committed files, or breaking changes detected.
The changes are narrowly scoped to fixing duration estimate logic and add unit tests for the new scope selection and query construction. No red flags detected.