Conversation
🦋 Changeset detectedLatest commit: b2da066 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
TL;DR — Prevents redundant artifact retrieval when the compressor has already summarized an artifact. The Key changes
Summary ｜ 6 files ｜ 11 commits ｜ base: Short-circuit artifact retrieval for compressed artifacts
The sentinel hint instructs the model to reference the artifact by ID using Persist compression key findings to the runtime database
The persistence is fire-and-forget — failures are logged at
Unit tests for compressor lookups and DAL updateTests cover
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Medium urgency — the retrieval guard in default-tools.ts looks correct, but persistArtifactKeyFindings has a silent data-loss bug: upsertLedgerArtifact is insert-only (returns { created: false } on conflict rather than updating), so the mutated parts with key_findings are never actually written back to the database. This means the hint message returned from get_reference_artifact references key findings that may not match what's actually stored.
| await upsertLedgerArtifact(runDbClient)({ | ||
| scopes: { tenantId: this.tenantId, projectId: this.projectId }, | ||
| contextId: this.conversationId, | ||
| taskId: dbArtifact.taskId ?? `task_${this.conversationId}-${this.sessionId}`, | ||
| toolCallId: dbArtifact.toolCallId, | ||
| artifact: { | ||
| ...dbArtifact, | ||
| parts, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Bug: upsertLedgerArtifact is insert-only — this update is silently lost.
upsertLedgerArtifact calls db.insert() and on a unique-constraint violation returns { created: false, existing } without writing anything. Since compression artifacts already exist at this point (they were created in createNewArtifact / saveToolResultsAsArtifacts), the insert will always conflict and the mutated parts with key_findings will never reach the database.
You need either:
- A proper
UPDATEquery (e.g.,db.update(ledgerArtifacts).set({ parts }).where(...)) — likely the right call here since you're patching a single field on an existing row. - An
onConflictDoUpdatein the upsert function.
Without this fix, the key_findings data is only held in the in-memory cumulativeSummary and will be lost on process restart.
| const dbArtifact = existing[0]; | ||
| const parts = (dbArtifact.parts ?? []) as any[]; | ||
| if (parts.length > 0 && parts[0]?.data?.summary) { | ||
| parts[0].data.summary = { | ||
| ...parts[0].data.summary, | ||
| key_findings: artifact.key_findings, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Mutates a shared object in place. parts aliases dbArtifact.parts — the parts[0].data.summary = { ... } assignment mutates the fetched DB row object. This is harmless today since the object is not reused, but if getLedgerArtifacts ever adds caching, this will silently corrupt the cache. Consider cloning:
const parts = structuredClone(dbArtifact.parts ?? []) as any[];| for (const artifact of relatedArtifacts) { | ||
| if (!artifact.id || !artifact.key_findings?.length) continue; |
There was a problem hiding this comment.
Nit: artifact.id is typed as z.string() (non-nullable) from the ConversationSummarySchema, so the !artifact.id guard is dead code — it can never be falsy. Consider removing it for clarity, or if you genuinely need to guard against empty strings, use !artifact.id.length.
| 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.`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The guard correctly prevents re-fetching a summarized artifact and points the model at the sentinel syntax. One concern: hasSummarizedArtifact matches on artifact.id (the compression-generated compress_* ID), but the model calls get_reference_artifact with an artifactId that came from the artifact list. Are these guaranteed to be the same ID? If the distillation produces the compression-generated ID in related_artifacts[].id but the model sees a different ID in available_artifacts, this guard will never trigger. Worth verifying the ID mapping is consistent end-to-end.
| if (summary.related_artifacts?.length) { | ||
| this.persistArtifactKeyFindings(summary.related_artifacts).catch((err) => | ||
| logger.warn( | ||
| { err: err instanceof Error ? err.message : String(err) }, | ||
| 'Failed to persist key_findings to artifacts' | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The .catch() fire-and-forget is fine for a best-effort persist, but since persistArtifactKeyFindings currently has no effect (see the upsert bug above), this entire block is inert. Once the persist path is fixed, consider whether a failure here should propagate — if the key findings aren't stored, the retrieval guard in default-tools.ts will still return them from memory, creating an inconsistency after restart.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
BaseCompressor.ts:653-662upsertLedgerArtifactis insert-only — key_findings will never be persisted to the database
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
BaseCompressor.ts:646-651Silent no-op when artifact structure doesn't match expected format - 🟠 Major:
BaseCompressor.ts:532-538Fire-and-forget async call masks persistence failures
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
BaseCompressor.ts:617-629Missing test coverage forhasSummarizedArtifact()andgetSummarizedArtifact()methods - 🟡 Minor:
default-tools.ts:24-36Missing test coverage for early return path when artifact already summarized
💭 Consider (1) 💭
💭 1) BaseCompressor.ts:632-673 Track persistence results for better observability
Issue: The persistArtifactKeyFindings method silently continues when individual artifacts fail. The method completes "successfully" even if ALL artifacts failed to persist.
Why: A "successful" call could have persisted 0 of N artifacts. Mix of silent skips and warn-logged failures makes tracking impossible.
Fix: Consider returning a result object: { persisted: number; skipped: number; failed: number } to enable monitoring and debugging.
🚫 REQUEST CHANGES
Summary: The core functionality of this PR — persisting key_findings to artifacts — will not work due to upsertLedgerArtifact being insert-only (no update capability). The PR adds the methods and calls them, but the database writes silently fail to update existing records. This is a blocking issue that must be fixed before merge.
Additionally:
- The new code paths lack test coverage (per AGENTS.md requirements)
- Silent failure modes in
persistArtifactKeyFindingscould mask data integrity issues - A changeset is needed since this changes runtime behavior in
agents-api
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
default-tools.ts:34 |
Race condition if hasSummarizedArtifact returns true but getSummarizedArtifact returns null |
Extremely unlikely given current implementation — both methods use the same cumulativeSummary object synchronously. Low confidence finding. |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-errors |
4 | 0 | 1 | 0 | 2 | 0 | 1 |
pr-review-tests |
4 | 0 | 0 | 0 | 2 | 0 | 0 |
| Total | 9 | 0 | 1 | 0 | 5 | 0 | 1 |
| await upsertLedgerArtifact(runDbClient)({ | ||
| scopes: { tenantId: this.tenantId, projectId: this.projectId }, | ||
| contextId: this.conversationId, | ||
| taskId: dbArtifact.taskId ?? `task_${this.conversationId}-${this.sessionId}`, | ||
| toolCallId: dbArtifact.toolCallId, | ||
| artifact: { | ||
| ...dbArtifact, | ||
| parts, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🔴 CRITICAL: upsertLedgerArtifact is insert-only — key_findings will never be persisted
Issue: The persistArtifactKeyFindings method calls upsertLedgerArtifact to update existing artifacts with key_findings, but this function doesn't actually update existing records. When a duplicate key error occurs, it returns { created: false, existing: ... } without applying any changes.
Why: Looking at ledgerArtifacts.ts:166-196, the function uses db.insert() without .onConflictDoUpdate(). The modified parts array with key_findings is never persisted to the database. This means:
- The in-memory
cumulativeSummarywill havekey_findings - The database artifacts will NOT have them
- On service restart/session reload,
key_findingsare lost - The
get_artifact_fullearly-return path will show stale data
Fix: Either:
- Add a dedicated
updateLedgerArtifactfunction that usesdb.update()with a WHERE clause, or - Modify
upsertLedgerArtifactto use.onConflictDoUpdate()similar to other upserts in the codebase (e.g.,upsertWorkAppSlackChannelAgentConfig)
Refs:
| if (parts.length > 0 && parts[0]?.data?.summary) { | ||
| parts[0].data.summary = { | ||
| ...parts[0].data.summary, | ||
| key_findings: artifact.key_findings, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Silent no-op when artifact structure doesn't match expected format
Issue: The code checks if (parts.length > 0 && parts[0]?.data?.summary) but has no else branch. If this condition is false (e.g., artifact has parts: [] or a different structure), the artifact is still upserted with unmodified parts, potentially overwriting existing data without adding key_findings.
Why: This creates a silent failure path where:
- The mutation at lines 647-650 never executes
- The upsert still runs with original (unmodified) parts
- No indication this happened — no log, no error
- Could corrupt artifact data if there's a race condition between read and write
Fix: Add explicit handling for unexpected structures:
const parts = (dbArtifact.parts ?? []) as any[];
if (parts.length === 0) {
logger.debug({ artifactId: artifact.id }, 'Artifact has no parts, skipping key_findings persistence');
continue; // Skip this artifact entirely
}
if (!parts[0]?.data?.summary) {
logger.debug(
{ artifactId: artifact.id, partKind: parts[0]?.kind },
'Artifact first part has no summary structure, skipping key_findings persistence'
);
continue; // Skip rather than risk overwriting with unmodified data
}| if (summary.related_artifacts?.length) { | ||
| this.persistArtifactKeyFindings(summary.related_artifacts).catch((err) => | ||
| logger.warn( | ||
| { err: err instanceof Error ? err.message : String(err) }, | ||
| 'Failed to persist key_findings to artifacts' | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🟠 MAJOR: Fire-and-forget async call masks persistence failures
Issue: The persistArtifactKeyFindings() method is called fire-and-forget with .catch() that only logs a warning. There's no mechanism for the caller to know if persistence failed, and no retry logic.
Why: Silent data loss scenario:
- If persistence fails (DB connection issue, constraint violation, etc.), the
key_findingsdata will never be persisted - The compression summary in memory will contain
key_findings, but database artifacts won't - On service restart or session reload,
key_findingswould be lost - Debugging is difficult — only a WARN log entry with minimal context (no artifact IDs, no correlation ID)
Fix: If persistence is critical, make it synchronous and propagate errors:
if (summary.related_artifacts?.length) {
try {
await this.persistArtifactKeyFindings(summary.related_artifacts);
} catch (err) {
logger.error(
{
err: err instanceof Error ? err.message : String(err),
conversationId: this.conversationId,
artifactIds: summary.related_artifacts.map(a => a.id),
},
'Failed to persist key_findings to artifacts - summary may be inconsistent'
);
// Decide: throw to fail compression, or continue with explicit degraded state
}
}If fire-and-forget is intentional, at minimum add richer logging with artifact IDs and conversation context.
| 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, | ||
| }; |
There was a problem hiding this comment.
🟡 Minor: Missing test coverage for new public methods
Issue: The new hasSummarizedArtifact() and getSummarizedArtifact() methods are called in the critical path of getArtifactTools but have no test coverage.
Why: These methods determine whether to short-circuit artifact retrieval. If broken (e.g., a typo changing a.id to a.artifactId), the agent could:
- Fail to recognize already-summarized artifacts, causing redundant retrievals that bloat context
- Return incorrect key_findings, causing the agent to use stale/wrong information
Fix: Add unit tests in BaseCompressor.test.ts:
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
});
});| 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.`, | ||
| }; |
There was a problem hiding this comment.
🟡 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:
- Returns a different response shape that the agent needs to handle
- Constructs a hint with SENTINEL_KEY values — if constants change, tests should fail
- Uses null-safety fallbacks (
summarized?.key_findings ?? []) that should be validated
Fix: Add tests in the appropriate test file:
describe('getArtifactTools with summarized artifact', () => {
it('should return already_summarized response when artifact is in compression summary', async () => {
const mockCompressor = {
hasSummarizedArtifact: vi.fn().mockReturnValue(true),
getSummarizedArtifact: vi.fn().mockReturnValue({
key_findings: ['finding1', 'finding2'],
tool_call_id: 'original-call-123',
}),
};
const ctx = { ...baseAgentRunContext, currentCompressor: mockCompressor };
const tool = getArtifactTools(ctx);
const result = await tool.execute({ artifactId: 'artifact-xyz', toolCallId: 'call-456' });
expect(result.status).toBe('already_summarized');
expect(result.key_findings).toEqual(['finding1', 'finding2']);
expect(result.hint).toContain('$artifact');
});
});
Ito Test Report ✅10 test cases ran. 10 passed. Across the unified run, all 10 executed test cases passed with 0 failures, and additional verification-only checks found no reportable product defects when reviewing ROUTE-2/ROUTE-3 artifacts and related code. Key findings were that playground token issuance and retry-after-failure flows are robust, summarized-artifact retrieval correctly short-circuits with already_summarized plus key_findings and compressor-preferred tool_call_id hints, and edge-case behavior remained stable across rapid double submit, send-clear-send resets, back/forward interruptions, mobile debug toggling, and multi-tab usage. ✅ Passed (10)
Commit: Tell us how we did: Give Ito Feedback |
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
Re-review scope: This review covers only the delta since the previous automated review. The author has addressed all Critical and Major issues.
✅ Issues Addressed Since Last Review
The following issues from the previous review have been fixed:
| Prior Finding | Resolution |
|---|---|
🔴 Critical: upsertLedgerArtifact is insert-only |
✅ Now uses new updateLedgerArtifactParts function with proper db.update() |
| 🟠 Major: Silent no-op when artifact structure doesn't match | ✅ Now logs at debug level and increments result.skipped |
| 🟠 Major: Fire-and-forget async masks persistence failures | ✅ Now synchronous with try/await/catch and rich error logging |
| 💭 Consider: Track persistence results for observability | ✅ Returns { persisted, skipped, failed } and logs at debug level |
| 🐸 Pullfrog: Mutates shared object in place | ✅ Now uses structuredClone(dbArtifact.parts ?? []) |
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
BaseCompressor.ts:627-640Missing test coverage forhasSummarizedArtifact()andgetSummarizedArtifact()methods - 🟡 Minor:
ledgerArtifacts.ts:373-389Missing test coverage forupdateLedgerArtifactPartsfunction
💭 Consider (1) 💭
💭 1) scope Add a changeset for this bug fix
Issue: The existing changeset (informal-green-porpoise.md) covers the PDF attachments feature from a merged commit, not this compressor bug fix.
Why: Per AGENTS.md, any bug fix or behavior change to a published package needs a changeset. This PR changes runtime behavior in agents-api and agents-core.
Fix:
pnpm bump patch --pkg agents-api --pkg agents-core "Fix key_findings persistence in compressor by using proper update instead of insert-only upsert"💡 APPROVE WITH SUGGESTIONS
Summary: Great work addressing all the critical and major issues from the previous review! The core bug is fixed — updateLedgerArtifactParts now properly updates existing artifacts instead of silently failing. The remaining items are test coverage (recommended but non-blocking for a bug fix) and adding a changeset. This PR is ready to merge once a changeset is added.
Discarded (0)
No findings were discarded in this review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
3 | 0 | 1 | 0 | 2 | 0 | 0 |
| Total | 3 | 0 | 1 | 0 | 2 | 0 | 0 |
Note: This is a delta re-review. Sub-reviewers were not re-dispatched since the prior Critical/Major issues have been addressed and no new significant code was introduced.
| key_findings: artifact.key_findings, | ||
| tool_call_id: artifact.tool_call_id, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 Minor: Missing test coverage for hasSummarizedArtifact() and getSummarizedArtifact() methods
Issue: These new public methods are called in the critical path of getArtifactTools but have no test coverage in BaseCompressor.test.ts.
Why: These methods determine whether to short-circuit artifact retrieval. If broken (e.g., a typo changing a.id to a.artifactId), the agent could fail to recognize already-summarized artifacts, causing redundant retrievals that bloat context, or return incorrect key_findings.
Fix: Add unit tests for edge cases:
hasSummarizedArtifactwhencumulativeSummaryis nullhasSummarizedArtifactwhen artifact exists vs doesn't exist inrelated_artifactsgetSummarizedArtifactreturning correctkey_findingsandtool_call_id
Refs:
| .returning(); | ||
|
|
||
| return result.length > 0; | ||
| }; |
There was a problem hiding this comment.
🟡 Minor: Missing test coverage for updateLedgerArtifactParts
Issue: This new data-access function has no test coverage in ledgerArtifacts.test.ts.
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:
- Successful update returns
true - Non-existent artifact returns
false - Tenant/project scoping is enforced (can't update artifacts from other projects)
Refs:
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
Re-review scope: This review covers only the delta since the previous automated review. The new commits are merge commits bringing in upstream changes — the core bug fix code is unchanged.
✅ Delta Assessment
The commits since last review (780e46ce2f98..HEAD) are:
d22b0f70a— merged mainc0018a66c— Route LLM calls through AI Gateway (#2803)d2b5368b7— return structured conversation history (#2743)
These changes affect BaseCompressor.ts but only introduce upstream modifications:
- Import reorganization (
estimateTokensmoved to@inkeep/agents-core) - Added
agentIdconstructor parameter - Added
usageContexttodistillConversationcall for cost tracking
The core bug fix functionality is intact and unchanged. All Critical and Major issues from prior reviews remain addressed.
🕐 Pending Recommendations (2)
- 🟡
BaseCompressor.ts:638-650Missing test coverage forhasSummarizedArtifact()andgetSummarizedArtifact()methods - 🟡
ledgerArtifacts.ts:373-389Missing test coverage forupdateLedgerArtifactPartsfunction
💭 Consider (1)
💭 1) scope Add a changeset for this bug fix
Issue: No changeset exists for this compressor bug fix. The existing changesets (golden-crimson-emu.md, informal-green-porpoise.md) are from other merged PRs.
Why: Per AGENTS.md, any bug fix or behavior change to a published package needs a changeset. This PR fixes a data persistence bug in agents-api and adds a new DAL function in agents-core.
Fix:
pnpm bump patch --pkg agents-api --pkg agents-core "Fix key_findings persistence in compressor by using proper update instead of insert-only upsert"💡 APPROVE WITH SUGGESTIONS
Summary: The merge commits introduce no new issues — they only bring in upstream changes from main (cost tracking, structured conversation history). The core bug fix remains solid: updateLedgerArtifactParts correctly persists key_findings to existing artifacts, and the get_artifact_full guard properly short-circuits for already-summarized artifacts.
Remaining items before merge:
- Changeset required — Run
pnpm bump patch --pkg agents-api --pkg agents-core "..."to document the bug fix - Test coverage recommended — The new methods lack tests, though this is non-blocking for a bug fix
Discarded (0)
No findings were discarded in this review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
3 | 0 | 1 | 0 | 0 | 2 | 0 |
| Total | 3 | 0 | 1 | 0 | 0 | 2 | 0 |
Note: Sub-reviewers were not re-dispatched — the delta contains only merge commits with no substantive changes to the bug fix code.
Ito Test Report ✅8 test cases ran. 8 passed. Across the unified run, 8 executable test cases were included and all 8 passed (0 failed), while two additional code-first verification checks reported no includable pass/fail outcomes. Key findings were that conversation detail retrieval remained valid after compression-related artifact persistence, the playground stayed resilient and usable through refresh/mobile/rapid-submit/stream-interruption scenarios, and adversarial attempts (sentinel-style payloads, policy-bypass prompt injection, and cross-project artifact guessing) were safely rejected without unauthorized data exposure, aligning with expected local non-production behavior. ✅ Passed (8)
Commit: Tell us how we did: Give Ito Feedback |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: This review covers only the delta since the previous automated review. All pending recommendations have been addressed.
✅ All Prior Recommendations Addressed
The following items from the previous review are now complete:
| Prior Recommendation | Resolution |
|---|---|
🟡 Missing test coverage for hasSummarizedArtifact() and getSummarizedArtifact() |
✅ Added 7 comprehensive tests in BaseCompressor.test.ts covering: null cumulativeSummary, null related_artifacts, artifact not found, artifact found, and correct return values |
🟡 Missing test coverage for updateLedgerArtifactParts |
✅ Added 2 tests in ledgerArtifacts.test.ts covering: successful update returns true, no-match returns false |
| 💭 Add a changeset for this bug fix | ✅ Added unconscious-brown-deer.md with proper packages and a clear message: "Fix key_findings persistence in compressor by using proper update instead of insert-only upsert" |
Implementation Summary
This PR correctly fixes the key_findings persistence bug:
- New DAL function:
updateLedgerArtifactPartsuses properdb.update()to update existing artifacts - Error handling: Synchronous
try/await/catchwith rich error logging (artifact IDs, conversation ID) - Observability: Returns
{ persisted, skipped, failed }results and logs at debug level - Data safety: Uses
structuredClone()to avoid mutating cached DB objects - Artifact retrieval guard:
hasSummarizedArtifact()check indefault-tools.tsshort-circuits redundant fetches
✅ APPROVE
Summary: Excellent work addressing all feedback! The tests thoroughly cover edge cases, the changeset is properly formatted, and the core bug fix implementation is solid. This PR is ready to merge.
Discarded (0)
No findings were discarded in this review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Sub-reviewers were not dispatched — this delta review verifies that all prior pending recommendations have been addressed.
Ito Test Report ❌12 test cases ran. 1 failed, 11 passed. Across 12 executed test cases, 11 passed and 1 failed, showing generally stable API/UI behavior including streaming completions, scope-header auth enforcement, Manage UI playground chat, conversation-history serialization, and race/mobile/navigation resilience. The single confirmed high-severity defect is that after a refresh the summarized-artifact retrieval guard can be bypassed because summarized state lives in request-scoped compressor memory that is cleaned up and not rehydrated, while non-refresh paths (including already_summarized responses and empty key_findings handling) behaved as designed. ❌ Failed (1)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Run bypass bearer without required scope headers was correctly rejected with HTTP 401. |  |
| Adversarial | Same-conversation concurrent requests completed without retrieval-state corruption. |  |
| Edge | Empty key_findings in summarized retrieval returns [] safely without server failure. |
 |
| Edge | Rapid concurrent submits completed without duplicate-output corruption. |  |
| Edge | Back/forward navigation during mid-flow did not break the conversation; a follow-up prompt still produced a valid response. |  |
| Edge | Mobile viewport flow remained usable through artifact, compression, and retrieval prompts, with readable streamed output. |  |
| Edge | Multi-turn compression/retrieval loop stayed stable across the long run. |  |
| Logic | Verified summarized retrieval is blocked with already_summarized, includes key_findings, and provides sentinel guidance. |
 |
| Happy-path | OpenAI-compatible completions endpoint returned HTTP 200 SSE with assistant delta chunks and a completion marker using valid scope headers. |  |
| Happy-path | Manage UI playground chat completed successfully after local non-production remediations, with assistant response visible. |  |
| Happy-path | Conversation detail endpoint returned HTTP 200 with parseable JSON messages after artifact compression and retrieval flow. |  |
Commit: 4fdb505
Tell us how we did: Give Ito Feedback
|
@pullfrog, please review this PR and tell me how helpful it will be and if it critically breaks anything |
There was a problem hiding this comment.
Verdict: Helpful bugfix, no critical breakage.
This PR fixes a real gap: after compression summarizes artifacts, the key_findings were never persisted back to the DB, and agents could wastefully re-retrieve already-summarized artifacts. Both problems are now addressed. The new updateLedgerArtifactParts DAL function is clean and correctly scoped. Tests cover the new public methods well.
Helpfulness: High. It closes two concrete loops — persistence of key_findings and short-circuiting redundant get_artifact_full calls with an actionable hint.
Breaking risk: None. All changes are additive. The early return in get_artifact_full is gated behind compressor?.hasSummarizedArtifact(), which is only truthy when compression has actually run. The new DAL function doesn't touch existing queries. The changeset is correctly scoped to agents-api and agents-core as patches.
Two minor notes inline.
Claude Opus ｜ 𝕏
| } | ||
|
|
||
| const dbArtifact = existing[0]; | ||
| const parts = structuredClone(dbArtifact.parts ?? []) as any[]; |
There was a problem hiding this comment.
Casting to any[] after structuredClone drops all type safety on the parts structure. The subsequent parts[0]?.data?.summary access chain is fragile — if the part shape ever changes (e.g. data gets renamed, or summary moves), this will silently produce malformed artifacts.
Consider adding a minimal type guard or narrowing before mutating, e.g.:
const firstPart = parts[0];
if (firstPart?.kind !== 'data' || !firstPart.data?.summary) { ... }This also makes the kind === 'data' assumption explicit rather than hidden.
| 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' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
The sequential for...of loop issues a SELECT + UPDATE per artifact. For conversations with many artifacts this creates N×2 round-trips. Not a problem today if artifact counts per compression are small, but worth a // TODO: batch if artifact count grows note if you expect this to scale.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: This review covers only the delta since the previous automated review. The delta addresses feedback and merges upstream changes.
✅ All Feedback Addressed
| Prior Feedback | Resolution |
|---|---|
🐸 Pullfrog: Type safety — as any[] drops type safety |
✅ Fixed in 0d27041af — now uses as Part[] with explicit firstPart.kind !== 'data' guard |
| 🐸 Pullfrog: Sequential loop performance (N×2 round-trips) | ℹ️ Noted as future optimization — acceptable for current artifact counts |
Implementation Summary
This PR correctly fixes the key_findings persistence bug:
- New DAL function:
updateLedgerArtifactPartsuses properdb.update()to persist changes to existing artifacts - Short-circuit guard:
hasSummarizedArtifact()indefault-tools.tsprevents redundantget_artifact_fullretrievals - Type safety: Uses
Part[]typing with explicitkind !== 'data'narrowing (addressed in latest commit) - Error handling: Synchronous
try/await/catchwith rich error logging - Observability: Returns
{ persisted, skipped, failed }results - Tests: Comprehensive coverage for new methods
- Changeset: Properly scoped to
agents-apiandagents-coreas patches
Note on Ito Test Failure
The Ito report flagged that summarized-artifact state can be lost after page refresh. This is a known architectural limitation — the compressor is request-scoped and doesn't rehydrate from persisted summaries on session restore. This PR correctly persists key_findings to the database; rehydration on session restore would be a separate enhancement.
✅ APPROVE
Summary: All prior feedback has been addressed. The type safety improvement (as Part[] with explicit kind guard) was the final polish needed. This is a well-implemented bug fix with proper tests, error handling, and observability. Ready to merge.
Discarded (0)
No findings were discarded in this review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Sub-reviewers were not dispatched — the delta contains only a type safety fix and merge commits with no substantive new code requiring specialized review.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Re-review scope: This review covers only the delta since the previous automated review. The delta is a single commit (
b2da06628) that parallelizes DB operations.
✅ Delta Assessment: Performance Optimization
The commit b2da06628 refactors persistArtifactKeyFindings from sequential to parallel execution, directly addressing the prior Pullfrog feedback about N×2 round-trips.
Changes:
| Before | After |
|---|---|
Sequential for...of loop with await per artifact |
Promise.allSettled for parallel fetch |
| Sequential update calls | Promise.allSettled for parallel update |
| N×2 round-trips | 2 batched operations |
Validation:
- ✅ Error handling preserved —
Promise.allSettledcatches individual failures without aborting the batch;result.failed++tracking maintained - ✅ Type safety maintained —
Part[]casting with explicitkind !== 'data'guard preserved from prior commit - ✅ Logging preserved — All warn/debug logs retained for observability
- ✅ Return value unchanged —
{ persisted, skipped, failed }contract intact - ✅ No new bugs introduced — Logic flow is equivalent, just parallelized
Implementation Summary
This PR correctly fixes the key_findings persistence bug and is now also performance-optimized:
- Core fix:
updateLedgerArtifactPartsuses properdb.update()to persist changes - Short-circuit guard:
hasSummarizedArtifact()prevents redundantget_artifact_fullretrievals - Performance: Parallelized fetch/update operations (this delta)
- Error handling: Synchronous
try/await/catchwith rich error logging - Observability: Returns
{ persisted, skipped, failed }results - Tests: Comprehensive coverage for new methods
- Changeset: Properly scoped to
agents-apiandagents-coreas patches
✅ APPROVE
Summary: Clean performance optimization that addresses prior feedback. The parallelization is correct — Promise.allSettled ensures all operations complete even if some fail, and the result tracking is properly maintained. Ready to merge.
Discarded (0)
No findings were discarded in this review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Sub-reviewers were not dispatched — the delta contains only a performance optimization refactor with no substantive logic changes requiring specialized review.
|
@pullfrog, is this safe to merge? |
There was a problem hiding this comment.
Safe to merge. The PR is well-structured: correct error handling (Promise.allSettled + try/catch wrapper), proper tenant/project scoping, consistent DAL patterns, good test coverage for the new public methods, and the get_artifact_full guard correctly prevents token-expensive re-retrieval of already-summarized artifacts while preserving the sentinel resolution path for tool chaining.
One observation (non-blocking): updateLedgerArtifactParts filters by (tenantId, projectId, id) but the composite PK includes taskId. If the same artifact ID exists under multiple tasks within a project, all rows get updated. This appears intentional — key_findings are a property of the artifact content, not task-specific — but worth being aware of.
Claude Opus ｜ 𝕏
|
No documentation updates needed for this bugfix. The changes are internal to the compressor and artifact persistence logic with no impact on public APIs or user-facing behavior. |
Ito Test Report ✅9 test cases ran. 9 passed. The unified run passed all 9/9 test cases with zero failures, indicating stable behavior across core chat routing, conversation continuity, playground/debug UI interactivity, navigation recovery (refresh/back-forward), long-context compression fallback, and resilience to non-fatal persistence warnings. Security and hardening checks also succeeded, with anonymous-session creation correctly enforcing origin rules (403 for disallowed origin, 200 with token for allowed localhost plus valid PoW) and prompt-injection attempts using fake artifact/tool references safely failing without data leakage. ✅ Passed (9)
Commit: Tell us how we did: Give Ito Feedback |
1 participant
No description provided.