refactor(cloud-agent-next): use message-based queued execution#3383
refactor(cloud-agent-next): use message-based queued execution#3383eshurakov wants to merge 5 commits into
Conversation
Code Review SummaryStatus: 2 Issues Remaining | Recommendation: Can merge; carried-forward items are low-risk Executive SummaryThe three new commits ( Overview
Issue Details (click to expand)WARNING (carried forward — unchanged)
SUGGESTION (carried forward — unchanged)
Resolved in this round
New changes reviewed (commits since 1532be0)
Files ReviewedNew in this round:
Carried forward (unchanged):
Fix these issues in Kilo Cloud Reviewed by claude-4.6-sonnet-20260217 · 1,983,115 tokens Review guidance: REVIEW.md from base branch |
9670177 to
ab795d7
Compare
bdeaa03 to
e2dbe6e
Compare
| prompt: rs?.prompt ?? null, | ||
| initialMessageId: rs?.initialMessageId ?? null, | ||
| associatedPr: sessionResult.associatedPr, | ||
| associatedPr: null, |
There was a problem hiding this comment.
Why are we setting associatedPr to always be null?
| label: 'Queued' | 'Failed to deliver'; | ||
| tone: 'info' | 'error'; | ||
| title?: string; | ||
| }; |
There was a problem hiding this comment.
Sounds like this should be:
export type DeliveryBadge =
| {
label: 'Queued';
tone: 'info';
title?: string;
}
| {
label: 'Failed to deliver';
tone: 'error';
title?: string;
};
| if (metadata.githubRepo) { | ||
| return { | ||
| type: 'github', | ||
| repo: metadata.githubRepo, | ||
| ...omitUndefined({ | ||
| platform: metadata.platform === 'github' ? 'github' : undefined, | ||
| githubInstallationId: metadata.githubInstallationId, | ||
| githubAppType: metadata.githubAppType, | ||
| upstreamBranch: metadata.upstreamBranch, | ||
| }), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Should this also still support the legacy githubToken attribute?
| const payload: CallbackJob['payload'] = { | ||
| sessionId, | ||
| cloudAgentSessionId: sessionId, | ||
| status, | ||
| errorMessage: error, | ||
| lastSeenBranch: metadata.repository?.upstreamBranch, | ||
| kiloSessionId: metadata.auth.kiloSessionId, | ||
| gateResult, | ||
| lastAssistantMessageText, | ||
| }; |
There was a problem hiding this comment.
I have zero clue about this one, but I figure that you will:
enqueueCallbackNotificationbuilds the legacy callback payload withoutexecutionId(services/cloud-agent-next/src/persistence/CloudAgentSession.ts:248), but the security-analysis callback still requires it (apps/web/src/app/api/internal/security-analysis-callback/[findingId]/route.ts:42). Old execution-id-only terminal paths can complete but have callbacks rejected. IncludeexecutionId: messageId ??execution.executionId.
Preserve callback and pending-message state through queue failures, keep legacy execution callback compatibility explicit during rollout, and tighten wrapper reconnect/lifecycle cleanup.
Summary
Verification
Reviewer Notes
messageIdbecomes the durable delivery/settlement key, withexecutionIdretained only where legacy response compatibility requires it.getSessionintentionally keeps callback targets redacted; stored callback metadata remains internal for outbox delivery and may contain service-to-service headers.