fix(proxy): apply exclusions and real provider identity to summarizer calls - #884
fix(proxy): apply exclusions and real provider identity to summarizer calls#884devin-ai-integration[bot] wants to merge 2 commits into
Conversation
… calls The handover and compaction summarizers send the entire prior conversation upstream, and did so without consulting the exclusion lists that are authoritative for routed traffic: an installation that excluded a provider still had full conversations shipped to it. ProviderSummarizer also reported anthropic regardless of ROUTER_HANDOVER_PROVIDER, so credential resolution looked up the tenant's Anthropic key and handed it to whatever client was configured - a Bearer gateway would have received it as Authorization: Bearer. Both summarizer paths now go through gateSummarizerCall, which refuses an excluded provider or model before resolving credentials and keeps the existing tenant-boundary rule. A refusal takes the same fallback as a summarizer error: full history on switch, trim on compaction.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
PR author is not in the allowed authors list. |
|
Claude finished @devin-ai-integration[bot]'s task —— View job Comment-length lint
Posted 3 comment-length nits as committable suggestions on the PR review. No logic/correctness comments. |
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
| // gateSummarizerCall decides whether the summary of a conversation may be sent | ||
| // to provider/model for this request. | ||
| // | ||
| // A summary call ships the entire prior conversation upstream, so it is subject | ||
| // to the same exclusions as routed traffic: a provider or model the operator | ||
| // excluded must not receive the content it was excluded from seeing. Session | ||
| // strike-outs are deliberately not consulted — transient 529 evidence is not a | ||
| // policy statement, matching policyExcludedProviders. | ||
| // | ||
| // The tenant boundary is unchanged: prefer the caller's own forwarded | ||
| // credentials, and rather than spend the deployment key on a BYOK/client-keyed | ||
| // request, skip. |
There was a problem hiding this comment.
| // gateSummarizerCall decides whether the summary of a conversation may be sent | |
| // to provider/model for this request. | |
| // | |
| // A summary call ships the entire prior conversation upstream, so it is subject | |
| // to the same exclusions as routed traffic: a provider or model the operator | |
| // excluded must not receive the content it was excluded from seeing. Session | |
| // strike-outs are deliberately not consulted — transient 529 evidence is not a | |
| // policy statement, matching policyExcludedProviders. | |
| // | |
| // The tenant boundary is unchanged: prefer the caller's own forwarded | |
| // credentials, and rather than spend the deployment key on a BYOK/client-keyed | |
| // request, skip. | |
| // gateSummarizerCall decides whether a summary call may be dispatched to provider/model. | |
| // Summaries ship the full prior conversation, so policy exclusions apply — use | |
| // policyExcludedProviders (not session strike-outs, which are transient evidence). | |
| // On BYOK/client-keyed requests without matching forwarded creds, skip to avoid | |
| // spending the deployment key across the tenant boundary. |
Was 9 lines across 3 paragraphs; the three WHYs fit in 5.
| // summarizerContext returns the context the summary call runs under: the | ||
| // caller's own credentials when the gate resolved some, otherwise a context | ||
| // stripped of any request credential (e.g. a subscription OAuth token) so the | ||
| // synthetic call runs on the deployment key instead of inheriting one that | ||
| // would 401 or cross a tenant boundary. |
There was a problem hiding this comment.
| // summarizerContext returns the context the summary call runs under: the | |
| // caller's own credentials when the gate resolved some, otherwise a context | |
| // stripped of any request credential (e.g. a subscription OAuth token) so the | |
| // synthetic call runs on the deployment key instead of inheriting one that | |
| // would 401 or cross a tenant boundary. | |
| // summarizerContext returns the context for the summary call: caller creds when | |
| // available, otherwise stripped so a subscription OAuth token can't 401 or cross a | |
| // tenant boundary. |
Was 5 lines; 3 carry the same information.
| // A summary carries the entire prior conversation, so an installation that | ||
| // excluded the summarizer's provider must not have it sent there — even though | ||
| // the request itself routes to a permitted provider and the deployment holds a | ||
| // key for the excluded one. |
There was a problem hiding this comment.
| // A summary carries the entire prior conversation, so an installation that | |
| // excluded the summarizer's provider must not have it sent there — even though | |
| // the request itself routes to a permitted provider and the deployment holds a | |
| // key for the excluded one. | |
| // An excluded summarizer provider must not receive the conversation even when | |
| // the routed model is permitted and the deployment holds a key for it. |
Was 4 lines; the "even though" qualifiers reduce to one clause.
|
Claude finished @devin-ai-integration[bot]'s task —— View job Comment-length lint
No comment blocks warrant flagging after the previous nits were applied. |
Runtime verification — summarizer egress policy ✅Tested Harness note — why this is adversarial rather than tautological: The fix: exclusion refuses the summary, and the turn still succeeds
Same call site, other refusal reasons: Baseline unchanged (the control that makes the above meaningful)Compaction paths — both call sites, each with a negative controlReaching Tier 3 needs care: Gated: Negative control — identical request, gate condition removed: 968 messages / 3.8 MB is what the permitted path ships upstream — precisely the exposure this PR gates. The third call site ( Boot behaviorA switch turn under that config still returns 200 and logs
One non-defect note for reviewers: |
2 participants
Summary
The handover and compaction summarizers send the entire prior conversation upstream — on every model switch and every compaction turn — and neither call consulted the exclusion lists that #883 made authoritative for routed traffic. An installation that excluded a provider still had full conversations shipped to it, through the one call that carries the most content.
Worse,
ProviderSummarizer.Provider()returnedanthropicregardless ofROUTER_HANDOVER_PROVIDER, and that string is what credential resolution keys off. Point handover at a Bearer gateway and the tenant's Anthropic key gets resolved and handed to the gateway client, which sends it asAuthorization: Bearerto the gateway's URL:Both summarizer paths now go through one gate:
A refusal takes the fallback that already exists for a summarizer error — full history on switch, trim on compaction — so no request fails because of this.
Three things worth calling out:
policyExcludedProviders, notexcludedProvidersForRequest. The latter folds in transient 529 session strike-outs; a strike is evidence, not policy, and shouldn't silently disable summarization for the rest of a session.selectCompactionSummarizernow takes actx), rather than picking a banned model and having the gate refuse it.handover.SummarizergrowsModel()alongsideProvider()so the orchestrator can ask what will actually be called.main.goadditionally refuses to wire a handover provider that isn't inFamilyAnthropic— the summarizer emits Anthropic Messages bodies with no translator in front of it, so a non-Anthropic-format client was never going to work; it now logs and leaves summarization off instead.Tests: the end-to-end one (
TestTurnLoop_HandoverSkippedWhenSummarizerProviderExcluded) was verified to fail against the pre-fix code — with the provider check removed it invokes the summarizer and reportshandover_fallback_to_full_history: false.Link to Devin session: https://app.devin.ai/sessions/4f53046daf8d4dbd8ac1c5695709f00b
Requested by: @steventohme