-
Notifications
You must be signed in to change notification settings - Fork 113
Bugfix/compressor bug #2833
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
Bugfix/compressor bug #2833
Changes from 5 commits
25a23b1
9db3ef6
c36cf12
780e46c
d22b0f7
2acf7d9
02652bc
4fdb505
0d27041
84dac97
b2da066
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 |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { type Tool, type ToolSet, tool } from 'ai'; | |
| import { getLogger } from '../../../../logger'; | ||
| import { formatOversizedRetrievalReason } from '../../artifacts/artifact-utils'; | ||
| import { getModelAwareCompressionConfig } from '../../compression/BaseCompressor'; | ||
| import { SENTINEL_KEY } from '../../constants/artifact-syntax'; | ||
| import { agentSessionManager } from '../../session/AgentSession'; | ||
| import type { AgentRunContext } from '../agent-types'; | ||
| import { wrapToolWithStreaming } from './tool-wrapper'; | ||
|
|
@@ -20,6 +21,21 @@ export function getArtifactTools(ctx: AgentRunContext): Tool<any, any> { | |
| execute: async ({ artifactId, toolCallId }) => { | ||
| logger.info({ artifactId, toolCallId }, 'get_artifact_full executed'); | ||
|
|
||
| const compressor = ctx.currentCompressor; | ||
| if (compressor?.hasSummarizedArtifact(artifactId)) { | ||
| const summarized = compressor.getSummarizedArtifact(artifactId); | ||
| logger.info( | ||
| { artifactId, toolCallId }, | ||
| 'Blocked retrieval of artifact already summarized in compression' | ||
| ); | ||
| return { | ||
| artifactId, | ||
| status: 'already_summarized', | ||
| key_findings: summarized?.key_findings ?? [], | ||
| hint: `This artifact's key findings are already in your compressed context. Use them directly to answer. To pass this artifact to a tool, use { "${SENTINEL_KEY.ARTIFACT}": "${artifactId}", "${SENTINEL_KEY.TOOL}": "${summarized?.tool_call_id ?? toolCallId}" } sentinel instead of retrieving it.`, | ||
| }; | ||
| } | ||
|
Comment on lines
+24
to
+37
Contributor
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. The guard correctly prevents re-fetching a summarized artifact and points the model at the sentinel syntax. One concern: |
||
|
|
||
| const streamRequestId = ctx.streamRequestId ?? ''; | ||
| const artifactService = agentSessionManager.getArtifactService(streamRequestId); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,9 @@ import { randomUUID } from 'node:crypto'; | |
| import type { ModelSettings } from '@inkeep/agents-core'; | ||
| import { | ||
| estimateTokens as estimateTokensUtil, | ||
| GENERATION_TYPES, | ||
| getLedgerArtifacts, | ||
| SPAN_KEYS, | ||
| updateLedgerArtifactParts, | ||
| } from '@inkeep/agents-core'; | ||
| import { type Span, SpanStatusCode } from '@opentelemetry/api'; | ||
| import runDbClient from '../../../data/db/runDbClient'; | ||
|
|
@@ -539,6 +539,26 @@ export abstract class BaseCompressor { | |
| ); | ||
|
|
||
| this.cumulativeSummary = summary; | ||
|
|
||
| if (summary.related_artifacts?.length) { | ||
| try { | ||
| const persistResult = await this.persistArtifactKeyFindings(summary.related_artifacts); | ||
| logger.debug( | ||
| { ...persistResult, conversationId: this.conversationId }, | ||
| 'Artifact key_findings persistence completed' | ||
| ); | ||
| } catch (err) { | ||
| logger.warn( | ||
| { | ||
| conversationId: this.conversationId, | ||
| artifactIds: summary.related_artifacts.map((a) => a.id), | ||
| err: err instanceof Error ? err.message : String(err), | ||
| }, | ||
| 'Failed to persist key_findings to artifacts' | ||
| ); | ||
| } | ||
| } | ||
|
Comment on lines
+544
to
+561
Contributor
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. The |
||
|
|
||
| return summary; | ||
| } | ||
|
|
||
|
|
@@ -615,6 +635,89 @@ export abstract class BaseCompressor { | |
| return this.cumulativeSummary; | ||
| } | ||
|
|
||
| hasSummarizedArtifact(artifactId: string): boolean { | ||
| return this.cumulativeSummary?.related_artifacts?.some((a) => a.id === artifactId) ?? false; | ||
| } | ||
|
|
||
| getSummarizedArtifact( | ||
| artifactId: string | ||
| ): { key_findings: string[]; tool_call_id: string } | null { | ||
| const artifact = this.cumulativeSummary?.related_artifacts?.find((a) => a.id === artifactId); | ||
| if (!artifact) return null; | ||
| return { | ||
| key_findings: artifact.key_findings, | ||
| tool_call_id: artifact.tool_call_id, | ||
| }; | ||
|
Comment on lines
+639
to
+651
Contributor
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. 🟡 Minor: Missing test coverage for new public methods Issue: The new Why: These methods determine whether to short-circuit artifact retrieval. If broken (e.g., a typo changing
Fix: Add unit tests in describe('hasSummarizedArtifact', () => {
it('should return true when artifact exists in related_artifacts', () => {
compressor['cumulativeSummary'] = {
...baseSummary,
related_artifacts: [
{ id: 'artifact-123', name: 'Test', tool_name: 'search', tool_call_id: 'call-1', content_type: 'results', key_findings: ['finding1'] }
],
};
expect(compressor.hasSummarizedArtifact('artifact-123')).toBe(true);
});
it('should return false when cumulativeSummary is null', () => {
compressor['cumulativeSummary'] = null;
expect(compressor.hasSummarizedArtifact('any-id')).toBe(false);
});
});
describe('getSummarizedArtifact', () => {
it('should return key_findings and tool_call_id when artifact exists', () => {
// ... test implementation
});
}); |
||
| } | ||
|
Contributor
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. 🟡 Minor: Missing test coverage for Issue: These new public methods are called in the critical path of Why: These methods determine whether to short-circuit artifact retrieval. If broken (e.g., a typo changing Fix: Add unit tests for edge cases:
Refs: |
||
|
|
||
| protected async persistArtifactKeyFindings( | ||
| relatedArtifacts: NonNullable<ConversationSummary['related_artifacts']> | ||
| ): Promise<{ persisted: number; skipped: number; failed: number }> { | ||
| const result = { persisted: 0, skipped: 0, failed: 0 }; | ||
|
|
||
| for (const artifact of relatedArtifacts) { | ||
| if (!artifact.key_findings?.length) { | ||
| result.skipped++; | ||
| continue; | ||
| } | ||
| try { | ||
| const existing = await getLedgerArtifacts(runDbClient)({ | ||
| scopes: { tenantId: this.tenantId, projectId: this.projectId }, | ||
| artifactId: artifact.id, | ||
| }); | ||
| if (existing.length === 0) { | ||
| logger.debug( | ||
| { artifactId: artifact.id }, | ||
| 'Artifact not found in DB, skipping key_findings persistence' | ||
| ); | ||
| result.skipped++; | ||
| continue; | ||
| } | ||
|
|
||
| const dbArtifact = existing[0]; | ||
| const parts = structuredClone(dbArtifact.parts ?? []) as any[]; | ||
|
||
| if (parts.length === 0 || !parts[0]?.data?.summary) { | ||
| logger.debug( | ||
| { artifactId: artifact.id, hasParts: parts.length > 0 }, | ||
| 'Artifact has no summary structure, skipping key_findings persistence' | ||
| ); | ||
| result.skipped++; | ||
| continue; | ||
| } | ||
|
|
||
| parts[0].data.summary = { | ||
| ...parts[0].data.summary, | ||
| key_findings: artifact.key_findings, | ||
| }; | ||
|
|
||
| const updated = await updateLedgerArtifactParts(runDbClient)({ | ||
| scopes: { tenantId: this.tenantId, projectId: this.projectId }, | ||
| artifactId: artifact.id, | ||
| parts, | ||
| }); | ||
|
|
||
| if (updated) { | ||
| result.persisted++; | ||
| } else { | ||
| logger.warn({ artifactId: artifact.id }, 'updateLedgerArtifactParts matched no rows'); | ||
| result.failed++; | ||
| } | ||
| } catch (error) { | ||
| result.failed++; | ||
| logger.warn( | ||
| { | ||
| artifactId: artifact.id, | ||
| conversationId: this.conversationId, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }, | ||
| 'Failed to persist key_findings to artifact' | ||
| ); | ||
| } | ||
| } | ||
|
||
|
|
||
| return result; | ||
| } | ||
|
|
||
| cleanup(options: { resetSummary?: boolean; keepRecentToolCalls?: number } = {}): void { | ||
| const { resetSummary = false, keepRecentToolCalls = 0 } = options; | ||
| if (keepRecentToolCalls > 0) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -366,6 +366,28 @@ export const getLedgerArtifactsByContext = | |
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * Update the parts column of an existing ledger artifact by ID. | ||
| * Used to persist modifications (e.g. key_findings) to artifacts that already exist. | ||
| */ | ||
| export const updateLedgerArtifactParts = | ||
| (db: AgentsRunDatabaseClient) => | ||
| async (params: { | ||
| scopes: ProjectScopeConfig; | ||
| artifactId: string; | ||
| parts: Part[]; | ||
| }): Promise<boolean> => { | ||
| const { scopes, artifactId, parts } = params; | ||
|
|
||
| const result = await db | ||
| .update(ledgerArtifacts) | ||
| .set({ parts, updatedAt: new Date().toISOString() }) | ||
| .where(and(projectScopedWhere(ledgerArtifacts, scopes), eq(ledgerArtifacts.id, artifactId))) | ||
| .returning(); | ||
|
|
||
| return result.length > 0; | ||
| }; | ||
|
Contributor
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. 🟡 Minor: Missing test coverage for Issue: This new data-access function has no test coverage in Why: This function is critical to the key_findings persistence feature. Without tests, regressions (e.g., incorrect WHERE clause, missing scope filtering) could go undetected. Fix: Add tests covering:
Refs: |
||
|
|
||
| /** | ||
| * Delete ledger artifacts by task ID | ||
| */ | ||
|
|
||
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.
🟡 Minor: Missing test coverage for early return path
Issue: This new code path that returns
status: 'already_summarized'when an artifact is in the compression summary has no test coverage.Why: This code path:
summarized?.key_findings ?? []) that should be validatedFix: Add tests in the appropriate test file: