fix(providers): guard compaction against token estimate drift

[hertznsk]

Summary

• add an independent UTF-8 byte safety estimate for token-dense text after a provider usage anchor
• keep the existing reserve trigger driven by the primary token estimate, using the conservative estimate only near the hard context window
• fall back to the conservative estimate when the primary gate measurement fails
• cover the real tier chain so the safety gate cannot be satisfied by delegation alone

Why

The primary compaction estimator combines provider-reported usage with a four-characters-per-token estimate for later messages. Token-dense suffixes can grow past a known context window while that estimate remains below the compaction trigger. In the observed shape, the primary estimate was about 106k while the independent byte estimate exceeded the 200k window.

The conservative estimate is intentionally not used for the inner tier reclaim calculation. Mixing byte counts with provider token anchors would overstate reclaimed tokens and could stop compaction too early.

This shared capability is used by both the OpenAI and Anthropic Pydantic AI providers.

Verification

• uv run pytest tests/test_providers/test_pydantic_ai_compaction.py tests/test_providers/test_openai.py tests/test_providers/test_claude.py (155 passed)
• make lint
• make typecheck
• library driver: primary=106004, conservative=200519, messages=31->21, primary_after=22055

[Jason Robert (jrob5756)]

This PR doesn't fix the bug it's named for. The window guard delegates to TieredCompaction.before_model_request, which re-gates on its own byte estimate and silently no-ops whenever that estimate is at or below target_tokens — exactly the region a token-dense payload lands in. On top of that, the byte bound is miscalibrated for ordinary English text (it fires around a quarter of the window) and gets promoted into before_estimate, which corrupts every telemetry field downstream (tokens_before, tokens_saved, still_over_trigger) on every guard-triggered compaction. The one test meant to pin the fix hard-codes a target_tokens the production resolver never produces, so it passes against a configuration that can't occur in the real system.

Blocking:

• src/conductor/providers/_pydantic_ai/compaction.py:321 — window guard is a no-op whenever the primary estimate is at or below target_tokens
• src/conductor/providers/_pydantic_ai/compaction.py:323 — byte bound trips at roughly a quarter of the window on plain text, producing a per-request no-op loop and a spurious warning
• src/conductor/providers/_pydantic_ai/compaction.py:328 — before_estimate gets overwritten with a byte count, inverting still_over_trigger on successful compactions
• tests/test_providers/test_pydantic_ai_compaction.py:343 — the regression test hard-codes a target_tokens the resolver never produces; under the real formula the motivating scenario doesn't compact at all

The recommended findings cover the fallback estimator's lack of independence from the primary, zero coverage on both new error branches, a test assertion pinning an unrelated harness constant instead of the actual safety property, docstrings and naming that no longer match what the code does, ASCII-only test coverage that would still pass if byte counting were swapped for a character count, and a duplicated try/except block that should collapse into one.

Findings that could not be anchored inline

These name a line outside this pull request's diff, so GitHub cannot attach them to a specific line.

src/conductor/providers/_pydantic_ai/compaction.py

RECOMMENDED

Several docstrings in the file now describe behaviour the change removed.

The diff deleted exactly one comment (# Zone (a): gate measurement.) and added one, leaving the rest of the file documenting the old contract. These are outside the diff hunks, which is why they'll be missed:

• Class docstring, "Gate measurement failure" bullet — "Log a warning and return the context unchanged; no event and no disable latch." Now false in the common case: a primary failure falls through to the conservative bound and can emit both lifecycle events. The PR's own test rename proves it — ..._skips_without_latch_or_event became ..._uses_fallback_without_latch, and assert not events became an assertion that both events fire. This bullet is the first thing a maintainer reads when triaging a compaction incident, and it tells them an event they're looking at can't exist.
• Dangling zone lettering — # Zone (b) and # Zone (c) survive with no (a). A reader searching for the missing label can't tell whether it was removed deliberately or lost in a merge.
• "measuring the context once per request ... halves the estimator work" (class docstring) and "owns the token gate (measured once per request)" (build_tiered_compaction) — the wrapper now measures twice unconditionally, and the inner strategy still re-gates on a third measurement. The stated rationale for putting the gate in this class has evaporated, which matters because it's what kept the inner re-gate invisible.
• Module docstring item 3 — "any unexpected error in the gate or tier chain ... returns the original context unchanged" is now true only when both estimators fail.
• _estimate_after_compaction_tokens — its closing claim that the helper "reports the same numbers the escalation loop acted on" stops holding whenever before_estimate arrives byte-derived.
• events.py::emit_compaction_start — "fires ... on a request whose estimated context size is above the trigger threshold" is contradicted by the may_exceed_window branch, which fires it well below the trigger. This is the contract JSONL consumers rely on.

Suggested fix: split the "Gate measurement failure" bullet into two — primary-failure-falls-back-to-the-byte-bound (may compact, emits events) and both-estimators-failed (unchanged, no event, no latch). Drop the zone letters and keep the descriptive text after the colon, so the scheme can't desynchronise again. Replace the "halves the estimator work" rationale with an accurate description of the two-estimate design, and extend the module docstring's item 1 to mention the second trigger condition. Note on _estimate_after_compaction_tokens that a byte-derived baseline inflates the reported figures — or, better, fix the unit mixing so the note is unnecessary.

[Jason Robert (jrob5756)]

BLOCKING

The window guard is a no-op whenever the primary estimate is at or below target_tokens.

When may_exceed_window fires, the wrapper delegates to self._inner.before_model_request(...), the harness's TieredCompaction. That capability re-gates on its own estimate, computed with tokenizer=None — the same four-characters-per-token heuristic the primary estimator uses — and returns the context unchanged once that estimate is already <= target_tokens. The byte bound never reaches the component that decides when to stop.

So the new branch only ever compacts in the narrow band target_tokens < primary <= trigger_tokens. Outside it, the guard fires, nothing gets dropped, and the request goes out at full size — while the wrapper still emits agent_compaction_start and a success-shaped agent_compaction_complete. That's worse than not guarding at all: an operator whose run dies on context_length_exceeded will see a completed compaction right beforehand and rule the estimator out as the cause.

This is anti-correlated with the bug being fixed. Content the heuristic undercounts is exactly what pushes the primary estimate down into the no-op region. I verified this against the real build_tiered_compaction stack (window 200,000 / trigger 96,000 / target 86,000): a 205,110-byte token-dense history — the exact shape this PR targets — gives primary 51,277 and conservative 205,110. The guard fires, and 40 messages come back as 40 messages, with both lifecycle events emitted anyway.

Suggested fix: the bound has to reach the escalation loop, not just the outer gate. Either build a second orchestrator over the same tier wrappers with the byte tokenizer and delegate to it when the window bound is what fired:

tiered_bytes = TieredCompaction(
    tiers=[clear_tier, summarize_tier, slide_tier],
    target_tokens=config.target_tokens,
    tokenizer=_count_utf8_bytes,  # baseline and reclaim in the same unit
)

or bypass TieredCompaction.before_model_request on that path and drive the tier chain directly with a target derived from the conservative measure, so the inner re-gate can't veto it. At minimum, don't report success for a no-op: after delegation, compare list(result.messages) against before_messages, and when the window guard fired but nothing changed, log at error and emit a degraded — not success-shaped — agent_compaction_complete.

[hertznsk]

Confirmed and reproduced before changing anything: under the production-resolved plan (resolve_compaction_plan(window=200_000, output_limit=64_000, tool_buffer=15_000) → trigger 121,000 / target 110,000) the motivating scenario returns 31 of 31 messages while both lifecycle events fire — the inner TieredCompaction re-gates on the same heuristic that under-counted the content and declines.

Fixed in e8490bf by taking your second option: on the guard path the wrapper now drives the tier chain directly (_drive_tiers_under_window_guard) instead of delegating to TieredCompaction.before_model_request. It escalates through the same tier wrappers, re-measuring with the density-calibrated estimate after each tier and stopping only once that estimate fits target_tokens, so the inner strategy's heuristic gate can no longer veto the safety compaction. The tiers receive the request's model context (mirroring the harness's context_for_request, so the summarizer resolves the same model it would on the normal path).

Your "at minimum" is covered as well: when the guard fired but the history is unchanged, the wrapper logs at error, and the complete event carries still_over_window: true instead of reading as success.

[Jason Robert (jrob5756)]

BLOCKING

Byte bound fires at roughly a quarter of the window on ordinary text, producing a per-request no-op loop and a spurious user-facing warning.

A UTF-8 byte count runs about 4x a real token count for English prose, and the guard fires at conservative_estimate >= window_tokens. With no usage anchor, the conservative value is a pure byte count of the whole history, so the guard trips at roughly window/4 real tokens — far below trigger_tokens. The inline comment three lines above claims this avoids "premature compaction of ordinary text"; the arithmetic says the opposite.

Measured against the real stack with the project's own resolver (resolve_compaction_plan(window=200_000, output_limit=64_000, tool_buffer=40_000) → trigger 96,000, target 86,000), 216,000 characters of plain English prose (~54,000 real tokens, 27% of the window, 56% of the trigger) trips the guard. On a 128k window it trips at ~32,000 tokens against a 79,616 trigger.

Since 54,000 is below target_tokens, the inner strategy declines (see the companion finding), so this becomes a no-op that repeats on every model request for the rest of the run, each time emitting agent_compaction_start plus a complete event carrying tokens_before=216000 against a 200,000 window, tokens_saved=0, and still_over_trigger=True. cli/run.py renders that last flag as a user-visible WARNING: context compacted ... history remains above the trigger. A healthy run at a quarter of its context budget ends up producing a warning per request, forever.

The anchored path is milder but still reachable: the byte overcount applies to the post-anchor suffix, so the guard fires below the trigger once that suffix reaches about a third of the reserve. Worth noting: the PR's own reported evidence (primary ~106k, bytes >200k) reproduces exactly from plain ASCII prose with a ~74.5k anchor — it isn't by itself evidence of token density.

Suggested fix: a raw byte count overshoots real tokens by ~4x for English, while the heuristic's undercount on dense content is only ~1.3-2x — the correction is bigger than the error it's correcting for. Replace the absolute byte-vs-window comparison with something calibrated: a real tokenizer for the post-anchor suffix, or a density signal (bytes-per-character, non-ASCII fraction) that only escalates the estimate for suffixes that are actually dense. Whatever the mechanism, add the negative test that's missing today, because it fails against the PR as written:

@pytest.mark.asyncio
async def test_ordinary_prose_below_trigger_is_not_compacted(self) -> None:
    events: list[tuple[str, dict[str, Any]]] = []
    cfg = _make_config(  # window_tokens defaults to 200_000
        trigger_tokens=96_000, target_tokens=86_000,
        event_callback=lambda t, d: events.append((t, d)),
    )
    capability = build_tiered_compaction(cfg)
    prose = "The quick brown fox jumps over the lazy dog and then writes a "
    messages = [
        ModelRequest(parts=[UserPromptPart(content=(prose * 70)[:4_000])])
        for _ in range(50)
    ]  # 200_000 bytes ~= 50_000 real tokens
    request_context = _request_context_with_messages(messages)

    result = await capability.before_model_request(_make_run_context(), request_context)

    assert len(result.messages) == len(messages)
    assert not events

And rewrite the comment at lines 317-319 so it states the real trade-off instead of ruling out the failure mode it actually introduces.

[hertznsk]

Confirmed — measured on the previous revision: 50 ordinary prose messages of 4,000 characters each gives primary 50,000 and conservative 200,000, so the guard tripped at 25% of the window on plain English, exactly as you calculated. The point about the PR's own evidence (primary ~106k / bytes >200k) reproducing from plain ASCII with a ~74.5k anchor was also correct.

The raw byte count is gone in e8490bf. The guard now uses a density-calibrated bound (_density_text_token_bound): it keeps the ~4-characters-per-token heuristic for ordinary prose, counts text with a substantial non-ASCII share at ~1 token per character, and whitespace-poor ASCII blobs at ~2 characters per token. It therefore escalates only for content that is measurably dense — base64's density comes from character distribution, which the whitespace check approximates, while CJK/non-Latin scripts are caught by the non-ASCII share. Both density signals are measured on a bounded leading sample so the per-request cost stays flat.

Your negative test is added nearly verbatim as test_ordinary_prose_below_trigger_is_not_compacted — it fails against the previous revision — plus direct unit tests for the bound (CJK → chars, base64-like → chars/2, prose → chars/4). The comment above the gate was rewritten to state the real trade-off instead of ruling out the failure mode it introduced.

[Jason Robert (jrob5756)]

BLOCKING

before_estimate is overwritten with a byte count and reported as tokens, inverting still_over_trigger on successful compactions.

before_estimate = max(before_estimate, conservative_estimate) silently changes the variable's unit. conservative_estimate isn't tokens — it's provider tokens for the anchored prefix plus UTF-8 bytes for everything after. That value then flows into _on_before(estimate=...) as tokens_before, into _estimate_after_compaction_tokens(...), into _on_after(...) as tokens_after/tokens_saved, and into still_over_trigger.

_estimate_after_compaction_tokens subtracts a reclaim measured with estimate_token_count(..., None) — the four-characters-per-token heuristic, in token units — from a byte-scale baseline. Its own docstring justifies that subtraction as mirroring what TieredCompaction._escalate acted on; that justification is void once the baseline is in a different unit from what the loop acted on.

Measured on the PR's own new test scenario, where compaction genuinely succeeds and drops 10 of 31 messages:

field	emitted	actual
tokens_before	200,519	106,004
tokens_after	191,065	22,055
tokens_saved	9,454	~83,900
still_over_trigger	True	false — 22,055 is far under the 121,000 trigger

Because conservative >= window_tokens is the precondition for reaching this line, window_tokens > trigger_tokens by construction, and a char-scale reclaim can't close a 4x unit gap — still_over_trigger will read True for essentially every byte-guarded compaction. cli/run.py and the dashboard store both branch on that flag to print a warning, so the PR turns every successful guard-triggered compaction into a false alarm and understates the reclaim by roughly 9x. tokens_before also renders as >100% of the context window in the console percentage.

Raising before_estimate buys nothing functionally either — it's never passed to the inner strategy, which re-measures internally. It's purely a telemetry baseline, and a wrong one.

Suggested fix: drop the promotion and keep before_estimate on the token scale for all telemetry:

window_guard_tripped = (
    conservative_estimate is not None
    and conservative_estimate >= self._config.window_tokens
)
if not window_guard_tripped and before_estimate <= self._config.trigger_tokens:
    return request_context

If the safety bound is useful for diagnostics, surface it as its own explicitly-named field on the start event (byte_bound, plus a trigger_reason="window_guard" | "trigger") rather than overloading tokens_before. If a byte-scale after-value is genuinely wanted, produce it with the same tokenizer that produced the before-value — _estimate_conservative_context_tokens(after_messages, ...) — never by subtracting a char-heuristic reclaim.

[hertznsk]

Confirmed — on the PR's own scenario the emitted payload read tokens_before=200,519, tokens_saved=9,454, still_over_trigger=true against a real after-value of ~22,000 tokens, and you're right that the promotion bought nothing functionally since the inner strategy re-measures anyway.

The promotion is removed in e8490bf. before_estimate stays on the primary token scale (or the fallback estimate when the primary is lost) and is never mixed with the safety measurement: the density-calibrated value is a gate input only, reported on the start event as its own field — density_tokens — alongside trigger_reason ("trigger" / "window_guard"), which is your suggested shape with a name that matches the new mechanism. On the same scenario the complete event now reads tokens_before=106,026, tokens_after=96,564, still_over_trigger=false.

The genuine failure mode you identified — a guard compaction that cannot get back under the window — is now reported honestly via still_over_window on the complete event instead of leaking through a unit-mismatched still_over_trigger.

[Jason Robert (jrob5756)]

BLOCKING

The regression test hard-codes a target_tokens the resolver never produces; under the real formula the motivating scenario doesn't compact.

target_tokens isn't a free parameter in production. compaction_window.target_tokens() computes max(1, min(int(window * 0.55), trigger - 1)), so for window_tokens=200_000 (the _make_config default) and trigger_tokens=121_000 the resolver produces 110,000, not the 100,000 hard-coded here. Confirmed against resolve_compaction_plan(window=200_000, output_limit=64_000, tool_buffer=15_000) → trigger=121000, target=110000.

That 10,000-token difference is decisive, because the inner TieredCompaction gates on target_tokens. I ran this test's exact message history against both configurations:

PR test config:      trigger=121000 target=100000 | 31 -> 21   COMPACTED
Production config:   trigger=121000 target=110000 | 31 -> 31   *** NO-OP ***

The test passes only because it picks a target just below the scenario's primary estimate of 106,004. Under the configuration Conductor actually ships, the reported bug isn't fixed — the guard fires, the inner strategy declines, and the oversized request goes out unchanged behind a success-shaped agent_compaction_complete. The one test that pins the new behaviour certifies a parameterisation the product never produces.

The same hard-coded pair shows up again at line 373 for the second new test.

Suggested fix: derive the test configuration from the real resolver so a regression test can't pass against an unreachable config:

plan = resolve_compaction_plan(
    window=200_000, output_limit=64_000, tool_buffer=15_000
)
capability = build_tiered_compaction(
    _make_config(
        trigger_tokens=plan.trigger_tokens,
        target_tokens=plan.target_tokens,
    )
)

Then re-run the scenario. It'll fail — that's the correct signal — and resolve it as part of the delegation fix above.

[hertznsk]

Confirmed — target_tokens() resolves 110,000 for window 200,000 / trigger 121,000, and with the hard-coded 100,000 removed, the old test's scenario no-ops (31 → 31), which I reproduced before the fix.

The regression test now derives its configuration exactly as you suggested, from resolve_compaction_plan(window=200_000, output_limit=64_000, tool_buffer=15_000), so it cannot pass against a parameterisation the product never produces. It failed against the previous revision for precisely the reason you identified, and passes against the rework (e8490bf) because the guard path drives the tier chain against the density-calibrated measurement directly and no longer depends on the inner strategy's target gate. The same change is applied to the second test in the class, which previously hard-coded the same pair.

[Jason Robert (jrob5756)]

RECOMMENDED

The conservative estimator isn't independent of the primary, so the fallback is unreachable for realistic failures.

_estimate_conservative_context_tokens and _estimate_context_tokens differ in exactly one argument: tokenizer. Both funnel into the same pydantic_ai_harness.compaction.estimate_context_tokens with the same messages and the same model_request_parameters, so they share the whole text-collection path — part extraction, the usage-anchor lookup, instruction text, tool-schema serialization. Every realistic failure of the primary (a part whose str(...) raises, a malformed usage anchor, an unserialisable parameters_json_schema) is tokenizer-independent and takes the fallback down with it. The only failure surface unique to the fallback is _count_utf8_bytes, which can't raise for a str.

I verified this by patching the shared harness function the way a real estimator bug would fail: both warnings fire, compaction is skipped, no events, no latch — byte-for-byte the pre-PR outcome, after fifteen extra lines and a second full traversal.

Both new tests hide this by patching conductor.providers._pydantic_ai.compaction._estimate_context_tokens, Conductor's own one-line wrapper — the single layer the two estimators do not share. That constructs a failure mode that can't occur in production and asserts a compaction cycle for it.

Suggested fix: either drop the fallback branch and restore the simple skip, or make it genuinely independent — walk request_context.messages directly and sum len(str(part).encode()) inside a per-part try, so one bad part degrades rather than zeroing the estimate. If the branch stays, add a test that patches pydantic_ai_harness.compaction.estimate_context_tokens (the shared layer) and asserts the skip path, so the shared-failure reality is pinned rather than papered over.

[hertznsk]

Confirmed — the old pair differed only in the tokenizer argument, so both funneled through the same harness text-collection path and any realistic failure (a part whose str(...) raises, a malformed anchor, an unserialisable schema) took the fallback down with the primary. Patching Conductor's own one-line wrapper in the tests was constructing a failure mode that can't occur in production, as you said.

The fallback is now genuinely independent in e8490bf: _estimate_context_tokens_independent walks the message list directly with attribute-level access (latest usage anchor found via getattr, per-part extraction inside a per-part try so one bad part degrades the estimate instead of zeroing it) and never imports the harness estimator. Your pinning test exists as test_shared_harness_failure_still_compacts_via_independent_fallback: it patches pydantic_ai_harness.compaction.estimate_context_tokens — the shared layer itself — and asserts the run still compacts via the fallback.

[Jason Robert (jrob5756)]

RECOMMENDED

Both new error branches are entirely uncovered, and the only surviving documented behaviour is untested.

Branch coverage on this module is 96%, missing exactly lines 294-301 and 309-310 — the two error paths this PR introduces.

Lines 294-301 (both estimators failed → return unchanged, no event, no latch) is now the only path that still behaves the way the class docstring describes, and the only remaining safety net against an exception escaping into the model request path. Lines 309-315 (primary succeeded, conservative raised) silently disarms the window guard for that request; if it were wrong — say return request_context instead of falling through — compaction would stop happening on the primary trigger whenever the safety estimator hiccups, and every existing test would still pass.

Separately, neither degradation is visible outside stderr. Conductor installs no logging handlers in src/, so these warnings reach logging.lastResort as unattributed stderr — absent from the JSONL log, the dashboard, and replay, and under --web-bg written to a file nobody was told to read. When compaction then runs on the primary alone, the emitted agent_compaction_complete is indistinguishable from a fully guarded one. Contrast the tier-failure path, which names degraded_tiers in the payload precisely so consumers don't read a degraded outcome as success. The engine has a precedent for the remedy in pricing_hook_silent.

Suggested fix: add both tests (patch _estimate_conservative_context_tokens with side_effect=RuntimeError(...) for the 309 branch, and patch both estimators for the 294 branch), asserting the emitted events and capability._disabled is False in each. For visibility, mirror the existing degraded_tiers convention: add degraded_estimators: list[str] to the complete payload, populated with "safety_bound" when the conservative estimate was lost, and emit a dedicated agent_compaction_skipped event with {"reason": "estimate_unavailable"} from the double-failure branch.

[hertznsk]

Both branches are now covered in e8490bf, and the visibility gap is closed the way you suggested:

• test_double_estimator_failure_emits_skipped_event_without_latch — both estimators raise; the context is returned unchanged, agent_compaction_skipped fires with {"reason": "estimate_unavailable"}, and _disabled stays False.
• test_density_failure_compacts_on_primary_and_reports_degradation — the density measurement raises while the primary succeeds; compaction proceeds on the primary alone, the complete event carries degraded_estimators: ["density"], and the latch stays off.

degraded_estimators follows the existing degraded_tiers convention on the complete payload (always present, empty when nothing was lost; "primary" when the fallback drove the request, "density" when the window guard was disarmed), and the console subscriber renders both it and the new skipped event, so a degraded or skipped gate is visible outside stderr.

[Jason Robert (jrob5756)]

RECOMMENDED

assert len(result.messages) == 21 pins an unrelated harness constant instead of the window property.

The class states its requirement as "known context windows are hard pre-request boundaries," but == 21 proves nothing about a window. It's keep_messages=20 from SlidingWindowCompaction in build_tiered_compaction, plus one. Change that tier parameter and this test breaks with an opaque failure while the safety behaviour is perfectly intact; conversely, it passes on real regressions.

It also hides which tier actually ran. The summarizing tier raises UserError: model must either be set on the agent... because SummarizingCompaction(..., model=None), degrades to the sliding-window fallback, and sets degraded_tiers == ["summarizing"] on every run — never asserted. So the test silently exercises only the deterministic fallback rather than the escalation its name implies.

The actual safety post-condition is never checked at all. The compacted result measures conservative = 88,220, comfortably under the 200,000 window — that's the assertion the test should be making.

Suggested fix:

assert len(result.messages) < len(messages)
assert (
    _estimate_conservative_context_tokens(
        list(result.messages), request_context.model_request_parameters
    )
    < cfg.window_tokens
), "compaction must bring the byte bound back under the known window"

Also assert degraded_tiers == ["summarizing"], or wire a stub summarizer model, so the test is honest about which tier it exercises. Consider asserting primary <= trigger < conservative up front so the test documents that it's isolating the byte bound — the current margin is 519 tokens out of 200,519 (0.26%), so any change to the harness's overhead accounting would silently flip its meaning without failing it.

[hertznsk]

All three points addressed in e8490bf:

• == 21 is gone. The test asserts len(result.messages) < len(messages) and then re-measures the result with the density estimator, requiring it to come back under cfg.window_tokens — the actual safety post-condition, essentially your suggested snippet adapted to the new name.
• degraded_tiers == ["summarizing"] is now asserted, so the test is honest about exercising the deterministic fallback rather than the escalation its name implies.
• The precondition is pinned up front: primary_before <= plan.trigger_tokens < density_before and density_before >= cfg.window_tokens, so the test documents that it isolates the window guard, and a change in harness overhead accounting that would flip its meaning now fails it loudly instead of silently.

[Jason Robert (jrob5756)]

RECOMMENDED

Both new helpers claim token units for values that are bytes or mixed, which is the proximate cause of the unit mixing.

_count_utf8_bytes is documented as "Return a one-byte-per-token safety bound for token-dense text" — it returns a byte count. Whether that bounds tokens is the caller's assumption, not this function's contract, and describing a primitive by its caller's role is the comment most likely to rot.

_estimate_conservative_context_tokens is worse: the name ends in _tokens and the return type is a bare int, but the value is neither tokens nor bytes. When a usage anchor is present, the harness returns provider tokens for the anchored prefix plus UTF-8 bytes for everything after — a mixed-unit quantity valid only as a one-sided upper bound. Nothing anywhere says "do not treat this as tokens," and three lines later it's assigned straight into before_estimate and shipped as tokens_before.

Also worth flagging: _estimate_context_tokens is async def while its new sibling is a plain def, though both wrap the same synchronous harness call. Given the project's "async/await for all provider operations" convention, the asymmetry reads as meaningful when it isn't.

Suggested fix: rename to _utf8_byte_length and _estimate_context_utf8_bytes, and state the unit plainly:

def _utf8_byte_length(text: str) -> int:
    """Return the UTF-8 byte length of ``text``.

    Used as a stand-in ``tokenizer``: no tokenizer emits more tokens than the
    text has bytes. The result is in BYTES, roughly 4x a real token count for
    English prose - never compare it with, or substitute it for, a token value.
    """

and for the estimator, document that with an anchor it returns anchor tokens + post-anchor bytes, so it's comparable only against the raw window and must not be reported as a token count. Under those names, before_estimate = max(before_estimate, byte_bound) reads immediately as the bug it is.

[hertznsk]

Done in e8490bf, with names that state the mechanism now that the byte count is gone entirely:

• _density_text_token_bound(text) — the docstring states the result is in TOKENS, documents the three content classes (ordinary prose → chars/4, substantial non-ASCII share → chars, whitespace-poor ASCII → chars/2), and notes the residual multi-token-per-character emoji case as a known undercount rather than claiming a hard bound.
• _estimate_context_tokens_density(messages, params) — the harness-anchored safety estimate; the docstring states it is comparable against the window and the target and must never be mixed into token telemetry.
• _estimate_context_tokens_independent(messages) — the fallback, documented as sharing no code with the primary path.

The async/sync asymmetry you flagged is removed as well: all three estimators are uniformly async (the primary's docstring notes the underlying harness call is synchronous and the signature is convention).

[Jason Robert (jrob5756)]

RECOMMENDED

No non-ASCII coverage: replacing the UTF-8 encode with a character count passes the whole suite.

The stated justification for a byte bound is content the four-chars-per-token heuristic undercounts — CJK, emoji, non-Latin scripts, base64. None of it is tested. Mutating _count_utf8_bytes from len(text.encode("utf-8")) to len(text) passes all 30 tests, because every payload in the file is pure ASCII, where the two are identical.

The fixture in the new test compounds this: f"turn-{index}-" + "!" * 4_193 is a run of ASCII exclamation marks. Repeated single-byte punctuation is among the least token-dense content there is — BPE merges long ! runs into very few tokens — so its real token count sits far below the heuristic, the opposite of the case the test name describes. The test demonstrates the ordinary-text false-positive path and asserts it as correct behaviour.

The >= boundary at line 323 is also unpinned: mutating it to > passes every test.

Suggested fix: add a direct unit test for the encoder and a boundary test for the comparison:

def test_conservative_estimator_counts_utf8_bytes_not_characters(self) -> None:
    messages = [ModelRequest(parts=[UserPromptPart(content="日本語" * 1_000)])]
    assert _estimate_conservative_context_tokens(
        messages, ModelRequestParameters()
    ) == 9_000

and rebuild the token-dense fixture from content that's genuinely dense relative to the heuristic — CJK text or base64 — rather than repeated punctuation, so the scenario matches the failure the PR describes.

[hertznsk]

Both gaps are closed in e8490bf:

• _density_text_token_bound("日本語" * 1_000) == 3_000 pins byte-vs-character behavior at the unit level — mutating the implementation toward a plain character count now fails this test, as does the suite-wide CJK coverage in TestKnownWindowSafety, whose fixtures are rebuilt from CJK text ("日本" * 2_098 per turn) instead of repeated ! runs. You're right that the old fixture was among the least token-dense content possible and was demonstrating the false-positive path.
• The >= boundary is pinned by test_window_guard_fires_at_exact_window_boundary: a density estimate exactly equal to the window trips the guard (asserting trigger_reason == "window_guard" and density_tokens == 200_000), and a one-character-short control produces no events.

[Jason Robert (jrob5756)]

RECOMMENDED

The duplicated conservative-estimate handler makes control flow do the work of a value.

The try/except/else shape forces the same nested try: conservative_estimate = _estimate_conservative_context_tokens(...) except Exception: warn to be written twice, in the except arm and again in the else arm. The two copies differ only in the log suffix and in whether before_estimate gets seeded. A future edit to one arm will silently miss the other.

One side effect worth flagging: the second copy raises its warning from inside an active exception handler, so its exc_info traceback carries a spurious "During handling of the above exception, another exception occurred" chain that'll mislead whoever reads it.

One caution on the neighbouring cleanup — the apparently redundant conservative_estimate is not None at line 327 is load-bearing, not dead. ty doesn't propagate narrowing through a boolean stored in a local, so deleting the conjunct while keeping the may_exceed_window local breaks make typecheck with Argument to function max is incorrect: Expected int, found int | None. Restructure rather than simply deleting it.

Suggested fix: let both estimates be int | None and resolve once, which removes a nesting level and one copy of the handler while preserving both operator-facing suffixes:

primary_estimate: int | None = None
try:
    primary_estimate = await _estimate_context_tokens(...)
except Exception:  # noqa: BLE001 - estimation must never fail the run
    logger.warning("Compaction gate measurement failed for agent %r; "
                   "using the conservative fallback.", ..., exc_info=True)

conservative_estimate: int | None = None
try:
    conservative_estimate = _estimate_conservative_context_tokens(...)
except Exception:  # noqa: BLE001 - estimation must never fail the run
    outcome = ("skipping compaction for this request." if primary_estimate is None
               else "using the primary estimate.")
    logger.warning("Compaction safety measurement failed for agent %r; %s",
                   self._config.agent_name, outcome, exc_info=True)

if primary_estimate is not None:
    before_estimate = primary_estimate
elif conservative_estimate is not None:
    before_estimate = conservative_estimate
else:
    return request_context

Then fold the window condition directly into the if so the narrowing is real and the not/not dance disappears (see the telemetry finding for the combined form).

[hertznsk]

Restructured in e8490bf along the lines you sketched: both estimates are resolved as int | None with a single handler per arm, before_estimate is picked once, and the window condition is folded directly into the gate so the narrowing is real (the ty point about the load-bearing is not None is moot under the new shape — make typecheck is clean).

One deliberate deviation from the sketch: the two arms no longer call the same function. The else arm uses the shared harness-backed density estimator (accurate anchoring), while the except arm uses the new independent walker, because the fallback's whole purpose is to survive a failure in the shared path — see the thread above about fallback independence. A side effect of the restructure is exactly the one you predicted: the else-arm warning no longer fires from inside an active exception handler, so the spurious "During handling of the above exception" chaining is gone on the common path.

[hertznsk]

Thank you for the careful review — every blocking finding reproduced against the real build_tiered_compaction stack before I changed anything, and the review was right on all four:

1. No-op delegation — reproduced: under the production plan (trigger 121,000 / target 110,000) the motivating scenario returned 31 of 31 messages with both lifecycle events emitted.
2. Miscalibration — reproduced: 200,000 characters of plain prose measured primary 50,000 / conservative 200,000, tripping the guard at 25% of the window.
3. Telemetry corruption — reproduced exactly as tabulated (tokens_before=200,519, still_over_trigger=true on a successful compaction).
4. Unreachable test config — reproduced: the scenario compacted only against the hard-coded target_tokens=100,000, never against the resolver's 110,000.

The rework is in e8490bf, and each finding has its own thread reply. In short: the byte count is gone. The guard now runs on a density-calibrated estimate (_density_text_token_bound) that matches the primary heuristic on ordinary prose, counts substantial non-ASCII text at ~1 token/character, and whitespace-poor ASCII blobs at ~2 chars/token — so it fires only on genuinely dense content. When it fires, the tier chain is driven directly against that measurement until it fits the target, so the inner strategy's heuristic gate can no longer veto the safety compaction. Token telemetry stays on the primary scale; the density value travels as density_tokens + trigger_reason on the start event, with degraded_estimators and still_over_window on the complete event, plus a new agent_compaction_skipped event for the double-estimator-failure case.

The recommended findings are all addressed as well: the fallback is now genuinely independent of the primary (with a test that patches the shared harness layer itself), both error branches are covered, == 21 is replaced by the safety post-condition plus a degraded_tiers assertion, the helpers are renamed with plain unit documentation and uniform async, CJK coverage pins byte-vs-character behavior, the >= boundary is pinned, and the measurement block is restructured to int | None resolution.

The unanchored docstring findings from the review body are also all in e8490bf: the "Gate measurement failure" bullet is split into primary-failure (falls back, may compact, emits events) and both-failed (skipped event, unchanged, no latch); the dangling zone letters are dropped in favour of descriptive text; the "measured once per request / halves the estimator work" rationale is replaced with an accurate description of the two-estimate design; module docstring items 1 and 3 name both trigger conditions and the fallback behaviour; _estimate_after_compaction_tokens now documents its token-scale assumption and the guard-path caveat; and emit_compaction_start's docstring describes the window_guard branch.

Verification: uv run pytest tests/test_providers/test_pydantic_ai_compaction.py tests/test_providers/test_openai.py tests/test_providers/test_claude.py (all pass), full suite green except two pre-existing environment failures in unrelated plugin/skill permission tests, make lint, make typecheck, plus a driver run: dense CJK suffix (primary 106,026 / density 200,609) compacts 31 → 21 with density_after=88,283 < window, ordinary prose at 200,000 characters produces no events, and a base64-shaped suffix compacts 61 → 21.

Also added the missing CHANGELOG entry under Unreleased → Fixed, and documented the guard in the workflow syntax guide and AGENTS.md.

[Jason Robert (jrob5756)]

LGTM, thanks for contributing!

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

❌ Patch coverage is 87.73585% with 13 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@ff4b312). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/conductor/providers/_pydantic_ai/compaction.py	92.22%	7 Missing ⚠️
src/conductor/cli/run.py	66.66%	3 Missing ⚠️
src/conductor/providers/_pydantic_ai/events.py	57.14%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #507   +/-   ##
=======================================
  Coverage        ?   91.88%           
=======================================
  Files           ?      164           
  Lines           ?    26628           
  Branches        ?        0           
=======================================
  Hits            ?    24466           
  Misses          ?     2162           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.