Repository navigation
feat(agent-core-v2): report fork request prefix parity in the prompt cache probe #4179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
93ee486
f61d7a0
ed46636
8a1d94e
3ba0d97
ebee0e5
1348081
1659910
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ import { | |
| type AgentLLMRequestPartHandler, | ||
| type AgentLLMRequestSource, | ||
| type AgentLLMRequestTask, | ||
| type AgentLLMTurnRequestSnapshot, | ||
| type PreparedTurnRequestConfig, | ||
| } from './llmRequester'; | ||
| import type { LLMRequestTrace } from '#/llm-adapter/contract/request-trace'; | ||
|
|
@@ -105,6 +106,7 @@ interface ResolvedLLMRequest { | |
| readonly systemPrompt: string; | ||
| readonly tools: readonly Tool[]; | ||
| readonly messages: Message[]; | ||
| readonly contextRevision: number; | ||
| readonly source: AgentLLMRequestSource | undefined; | ||
| readonly logFields: AgentLLMRequestLogFields; | ||
| } | ||
|
|
@@ -148,6 +150,10 @@ export const llmRequesterEmittedThinkingEffortWarningsKey = defineState<Set<stri | |
| 'llmRequester.emittedThinkingEffortWarnings', | ||
| () => new Set(), | ||
| ); | ||
| export const llmRequesterLastTurnRequestKey = defineState<AgentLLMTurnRequestSnapshot | undefined>( | ||
| 'llmRequester.lastTurnRequest', | ||
| () => undefined as AgentLLMTurnRequestSnapshot | undefined, | ||
| ); | ||
|
|
||
| export class AgentLLMRequesterService implements IAgentLLMRequesterService { | ||
| declare readonly _serviceBrand: undefined; | ||
|
|
@@ -179,6 +185,7 @@ export class AgentLLMRequesterService implements IAgentLLMRequesterService { | |
| this.states.contributeState(llmRequesterMediaDegradedTurnsKey); | ||
| this.states.contributeState(llmRequesterMediaStrippedTurnsKey); | ||
| this.states.contributeState(llmRequesterEmittedThinkingEffortWarningsKey); | ||
| this.states.contributeState(llmRequesterLastTurnRequestKey); | ||
| } | ||
|
|
||
| private get lastConfigLogSignature(): string | undefined { | ||
|
|
@@ -222,6 +229,10 @@ export class AgentLLMRequesterService implements IAgentLLMRequesterService { | |
| return this.modelCatalog.get(resolved.modelAlias).credentialProvider; | ||
| } | ||
|
|
||
| lastTurnRequest(): AgentLLMTurnRequestSnapshot | undefined { | ||
| return this.states.get(llmRequesterLastTurnRequestKey); | ||
| } | ||
|
|
||
| async request( | ||
| overrides: AgentLLMRequestOverrides = {}, | ||
| onPart: AgentLLMRequestPartHandler = noopOnPart, | ||
|
|
@@ -413,7 +424,8 @@ export class AgentLLMRequesterService implements IAgentLLMRequesterService { | |
| fields, | ||
| }; | ||
| this.logRequest(logInput); | ||
| this.recordRequest(logInput); | ||
| const recorded = this.recordRequest(logInput); | ||
| const startedAt = Date.now(); | ||
|
|
||
| let message: Message | undefined; | ||
| let usage: TokenUsage | undefined; | ||
|
|
@@ -474,6 +486,17 @@ export class AgentLLMRequesterService implements IAgentLLMRequesterService { | |
| throw error; | ||
| } | ||
|
|
||
| if (request.source?.type === 'turn') { | ||
| this.states.set(llmRequesterLastTurnRequestKey, { | ||
| modelAlias: request.modelAlias, | ||
| contextRevision: request.contextRevision, | ||
| toolsHash: recorded.toolsHash, | ||
| systemPromptHash: recorded.systemPromptHash, | ||
|
Comment on lines
+491
to
+494
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence beyond the prior effort-switch issue is the reloadable AGENTS.md reference: AGENTS.md:L118-L120 Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accepted as a known blind spot for now. Divergence through this dimension needs a [thinking].keep / effort change between the parent's request and the fork plus earlier thinking parts in history — narrow enough that the probe documents it instead of comparing it (noted in the PR description). If more serialization dimensions show up, the snapshot carries one more field per dimension; happy to revisit then. |
||
| inputTokens: usage === undefined ? undefined : inputTotal(usage), | ||
| startedAt, | ||
| finishedAt: Date.now(), | ||
| }); | ||
| } | ||
| void this.usage.record( | ||
| this.scopeContext.agentContext, | ||
| request.modelAlias, | ||
|
|
@@ -723,6 +746,7 @@ export class AgentLLMRequesterService implements IAgentLLMRequesterService { | |
| systemPrompt: overrides.systemPrompt ?? turnConfig?.systemPrompt ?? this.profile.getSystemPrompt(), | ||
| tools: [...(overrides.tools ?? this.defaultTools())], | ||
| messages: [...messages], | ||
| contextRevision: this.context.prefixRevision(), | ||
| source: overrides.source, | ||
| logFields: logFieldsForSource(overrides.source), | ||
| }; | ||
|
|
@@ -776,7 +800,7 @@ export class AgentLLMRequesterService implements IAgentLLMRequesterService { | |
| this.log.info('llm request', requestFields); | ||
| } | ||
|
|
||
| private recordRequest(input: LLMRequestLogInput): void { | ||
| private recordRequest(input: LLMRequestLogInput): LlmRequestPayload { | ||
| const fields = input.fields ?? {}; | ||
| const wireTools = providerVisibleTools(input.tools); | ||
| const tools = toolSignature(wireTools); | ||
|
|
@@ -822,6 +846,7 @@ export class AgentLLMRequesterService implements IAgentLLMRequesterService { | |
| droppedCount: numberField(fields, 'droppedCount'), | ||
| }; | ||
| void this.dispatcher.dispatch(new LlmRequest(payload)); | ||
| return payload; | ||
| } | ||
|
|
||
| private logResponse( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,17 @@ | ||
| import { IInstantiationService } from '#/_base/di/instantiation'; | ||
| import { Service } from '#/_base/di/service'; | ||
| import { ScopeActivation, registerScopedService } from '#/_base/di/scope'; | ||
| import { LifecycleScope } from '#/app/scopes'; | ||
| import { IAgentContextMemoryService } from '#/agent/contextMemory/contextMemory'; | ||
| import { | ||
| IAgentLLMRequesterService, | ||
| type AgentLLMTurnRequestSnapshot, | ||
| } from '#/agent/llmRequester/llmRequester'; | ||
| import { IAgentScopeContext } from '#/agent/scopeContext/scopeContext'; | ||
| import { ITelemetryService } from '#/app/telemetry/telemetry'; | ||
| import { inputTotal } from '#human/llm/usage'; | ||
| import { IModelCatalog } from '#/llm-adapter/model/catalog'; | ||
| import { IAgentLifecycleService } from '#/session/agentLifecycle/agentLifecycle'; | ||
| import { ISessionUsageService } from '#/session/usage/sessionUsage'; | ||
|
|
||
| import { IAgentCacheProbeService } from './cacheProbe'; | ||
|
|
@@ -13,14 +20,25 @@ import { type UsageRecordedContext } from './usage'; | |
| export class AgentCacheProbeService extends Service implements IAgentCacheProbeService { | ||
| declare readonly _serviceBrand: undefined; | ||
|
|
||
| private parentId: string | undefined; | ||
|
|
||
| private parentRequest: AgentLLMTurnRequestSnapshot | undefined; | ||
|
|
||
| constructor( | ||
| @ISessionUsageService usage: ISessionUsageService, | ||
| @IAgentScopeContext scopeContext: IAgentScopeContext, | ||
| @ITelemetryService private readonly telemetry: ITelemetryService, | ||
| @IModelCatalog private readonly models: IModelCatalog, | ||
| @IInstantiationService private readonly instantiation: IInstantiationService, | ||
| @IAgentLifecycleService private readonly agentLifecycle: IAgentLifecycleService, | ||
| ) { | ||
| super(); | ||
| if (scopeContext.forkedFrom === undefined) return; | ||
| this.parentId = scopeContext.forkedFrom; | ||
| this.parentRequest = agentLifecycle | ||
| .handleOf(this.parentId) | ||
| ?.accessor.get(IAgentLLMRequesterService) | ||
| .lastTurnRequest(); | ||
| this._register( | ||
| usage.onDidRecord((e) => { | ||
| if (e.agent.agentId === scopeContext.agentId) this.probe(e); | ||
|
|
@@ -30,13 +48,29 @@ export class AgentCacheProbeService extends Service implements IAgentCacheProbeS | |
|
|
||
| private probe(e: UsageRecordedContext): void { | ||
| if (!e.firstRecord || e.source?.type !== 'turn') return; | ||
| const parentRevision = | ||
| this.parentId === undefined | ||
| ? undefined | ||
| : this.agentLifecycle | ||
| .handleOf(this.parentId) | ||
| ?.accessor.get(IAgentContextMemoryService) | ||
| .prefixRevision(); | ||
| const parent = | ||
| this.parentRequest !== undefined && this.parentRequest.contextRevision === parentRevision | ||
| ? this.parentRequest | ||
| : undefined; | ||
| let providerType: string | undefined; | ||
| let protocol: string | undefined; | ||
| try { | ||
| const model = this.models.get(e.model); | ||
| providerType = model.providerType ?? model.protocol; | ||
| protocol = model.protocol; | ||
| } catch { } | ||
| const own = this.instantiation | ||
| .invokeFunction((accessor) => accessor.get(IAgentLLMRequesterService)) | ||
| .lastTurnRequest(); | ||
| const sameModel = parent !== undefined && parent.modelAlias === e.model; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a provider's model or base URL is reconfigured in place under the same alias after the parent turn, Useful? React with 👍 / 👎. |
||
| const compared = sameModel && own !== undefined; | ||
|
Comment on lines
+72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| this.telemetry.track2('prompt_cache_probe', { | ||
| source: 'fork', | ||
| turn_id: e.source.turnId, | ||
|
|
@@ -46,6 +80,10 @@ export class AgentCacheProbeService extends Service implements IAgentCacheProbeS | |
| input_cache_read: e.usage.inputCacheRead, | ||
| input_cache_creation: e.usage.inputCacheCreation, | ||
| output_tokens: e.usage.output, | ||
| tools_match: compared ? own.toolsHash === parent.toolsHash : undefined, | ||
| system_prompt_match: compared ? own.systemPromptHash === parent.systemPromptHash : undefined, | ||
|
Comment on lines
+83
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When progressive tool disclosure is enabled and a loaded deferred tool is disabled between the parent request and the fork (for example through the supported session Useful? React with 👍 / 👎. |
||
| parent_input_tokens: sameModel ? parent?.inputTokens : undefined, | ||
| parent_request_gap_ms: compared ? own.startedAt - parent.finishedAt : undefined, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the parent response streams or is back-pressured for appreciable time, subtracting Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accepted. The stream-duration bias only misclassifies samples whose parent finished with a long single-step stream, and gap_ms is the weakest of the four fields (a heuristic for write-race vs expiry while the hashes and token baseline stay exact). Documented as a known limitation in the PR description rather than changing the field semantics. |
||
| }); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds
llmRequester.lastTurnRequestto the generatedAgentStateSnapshotcontract, but the change description only discusses probe internals and does not name the desktop/web/VS Code or other state-manifest consumers, nor explain how previous-release clients or snapshot payloads tolerate the new required key. Record the consumer and compatibility behavior before merging this contract change.AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.