Repository navigation
Conversation
The Cloud Agent treats a message id as the durable user-message identity and replays a repeated id instead of admitting a second turn. The cloud API binding made the id optional, so a caller could leave it out and lose replay on a retry. CloudAgentApi.send now requires the id, and the cloud transport refuses a send without one before the request leaves the client.
…d rows linkPendingUploads now reports the requested keys whose row is already 'reaped', and the new linkAttachmentsForDispatch helper links one message's attachment rows before a dispatch and refuses the dispatch when a row was already released, with an actionable error instead of a turn whose attachment object is gone. The abandoned-upload reaper re-checks the lease in the claim statement that moves a row to 'reaped', so a row refreshed by a retry presign after the batch was picked is skipped. The claim comments now state the invariant they hold.
…sage id The personal and organization prepareSession and sendMessage handlers linked a message's attachment rows after the Worker had answered, inside the same try whose catch rethrows the Worker error. A ledger write failure therefore reported an accepted send as failed, and a retry of the reported failure had no stable id to replay against. Both handlers now link the rows before the dispatch, so no bookkeeping is left after the Worker accepts the turn, and they forward the client's message id instead of minting one. baseSendMessageNextSchema requires messageId, so a client that omits it is rejected before any Worker call. Comments in the composer hook describe the dispatch-time link they rely on.
iscekic
added
drain
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change replaces the single-level status read with a bounded cause-chain walk (max four links) that keeps the existing 4xx-only release gate, handles non-object and cyclic causes, and adds direct coverage of a status carried only on the cause; no new defect was found in the changed lines. Assumptions: Files Reviewed (2 files)
Previous Review Summaries (8 snapshots, latest commit a04fa79)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a04fa79)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental fix routes the credits and billing errors that Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 incremental files + 2 targeted checks)
Previous review (commit 2ce4703)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe new 4xx-based release gate is sound for the errors it recognizes, but Assumptions: Overview
Issue Details (click to expand)WARNING
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit f4d376e)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe new post-failure release is keyed by the page-level upload path rather than the dispatch, and its two lookups are not serialized with the Worker's admission queue, so either path can return rows of an accepted message to the reaper. Assumptions: Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous review (commit 75a53ca)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryNo new defects at HEAD ( Assumptions: Overview
Issue Details (click to expand)WARNING
Re-verified at HEAD. Files Reviewed (17 files)
Fix these issues in Kilo Cloud Previous review (commit 01616de)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryNo new defects were found at HEAD ( Assumptions: Overview
Issue Details (click to expand)WARNING
Re-verified at HEAD. Files Reviewed (17 files)
Fix these issues in Kilo Cloud Previous reviewStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental commit adds the required Assumptions: Overview
Issue Details (click to expand)WARNING
Re-verified at HEAD. Files Reviewed (3 files)
Previous review (commit 55fa9cf)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental commit fixes both previously reported findings by keeping the link transaction legitimate and updating the send-schema fixtures; the remaining issue is the unbounded retention of Assumptions: Overview
Issue Details (click to expand)WARNING
Re-verified at HEAD and still present, unchanged by this commit. Files Reviewed (5 files)
Targeted re-checks outside the incremental diff: Previous review (commit 22de599)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe incremental transaction refactor fixes the reaped-row race and the partial-link side effect, but the required Assumptions: Overview
Issue Details (click to expand)WARNING
For the retained rows, the failure is not bounded in-repo: Files Reviewed (6 files)
Reviewed by deepseek-flash-latest · Input: 58.4K · Output: 15.1K · Cached: 601.5K Review guidance: REVIEW.md from base branch |
… fixture linkAttachmentsForDispatch now links and refuses in one transaction, so a message that cannot be dispatched leaves every row as it was instead of pinning the rows that could be linked. The link reports every requested key the message does not hold, which also covers a row a failed object delete returned to pending and therefore made reapable again. The schema test's send fixture carries the now-required message id, and the organization ordering assertion also requires the Worker call to have happened.
The dispatch schema now requires the client message id, so the send fixtures in cloud-agent-next-send-schema.test.ts carry one.
|
bot: On the remaining warning, retention of A dispatch failure can arrive after the Worker accepted the turn, so releasing the rows would delete an object an accepted message references, which is the failure this change removes. The rows stay The account-deletion detail is real but pre-existing and independent of this change: |
iscekic
added
merge:blocked:kilobot-not-ready
iscekic
added
the
merge:blocked:insufficient-e2e
…ends-and-attachments # Conflicts: # apps/web/src/routers/cloud-agent-next-send-schema.test.ts
iscekic
removed
merge:human
|
bot: Merged main and preserved both the required client message id and the shared payload-length boundary. Added message ids to the three incoming boundary fixtures. The affected ledger, schema, personal-router, and organization-router suites pass: 537 tests across five suites. Web type checking also passes. The head review repeats the already accepted retention trade-off: linked objects are retained after ambiguous dispatch failure, with no verified age bound. Automatic release would violate the approved accepted-send safety contract; that warning remains documented rather than being described as fixed. |
iscekic
added
merge:blocked:kilobot-not-ready
iscekic
added
merge:human
…admit it After a send fails, the router asks the worker whether that message exists. Only a message the worker reports as not found hands the attachment rows back to the abandoned-upload reaper; a message the worker holds, and an unavailable lookup, keep them linked.
iscekic
removed
merge:blocked:kilobot-not-ready
|
bot: At On the local stack: a send the worker accepted queued with the row |
iscekic
added
merge:blocked:kilobot-not-ready
The link now reports the rows it moved to linked, and the failure path hands back exactly those keys and only when the worker itself refused the turn. A composer path reused by a later send therefore cannot free an accepted message's attachments, and a transport or server-side failure keeps them.
iscekic
removed
merge:human
iscekic
added
merge:blocked:kilobot-not-ready
The send path rewrites some worker answers into its own error classes, so the release now reads the status from any error that carries one and keeps the rows for a transport failure and a server-side status.
iscekic
removed
merge:blocked:kilobot-not-ready
iscekic
added
merge:blocked:kilobot-not-ready
A non-JSON answer is wrapped with the original error as its cause, so the release reads the status along that chain too and still keeps the rows when no status is present.
iscekic
added
merge:blocked:kilobot-not-ready
Evidence
Evidence: demonstrated — a local end-to-end run on this VM drove the changed path through the real web tRPC endpoint, the real Worker runtime, and real PostgreSQL.
Run at
f0cca6b36onpnpm dev:start cloud-agent --no-attach: Next.js, PostgreSQL, thecloud-agent-nextWorker on 8794 (workerd with its Durable Objects), a fake-account user signed in through the real NextAuth fake-login provider, and one Cloud Agent session prepared through the publicprepareSessionprocedure. Attachment rows were admitted directly in the local ledger, so every scenario called only the publicsendMessageprocedure against the real ledger rows.delivery: queued; the row the dispatch linked stayedlinkedSelected model is not available for this cloud agent session; the router loggedReturned the attachments of a refused send { released: 1 }and the row returned topendingInsufficient credits: $1 minimum required;released: 1and the row returned topendinglinkedfetch failed; nothing released and the row stayedlinkedThe ledger, personal-router, organization-router, and helper suites pass: 345 tests across four suites, covering the release transition, the reused composer path, the reaper after a release, a 4xx refusal, a 5xx answer, a transport failure, and a failing release.
pnpm --filter webtype checking passes; oxlint and oxfmt are clean on the changed files.Limits: the accepted scenario queued the turn without a container start, so no upstream model was involved, and no object store was exercised because the ledger rows were admitted directly. Releasing hands rows back to the abandoned-upload reaper rather than deleting objects from the request path, so object deletion remains the reaper's behaviour, covered by the ledger suite.
Change
apps/web/src/lib/r2/cloud-agent-pending-uploads.ts:linkAttachmentsForDispatchnow returns the object keys it moved frompendingtolinked, andreleaseUnadmittedUploadsmoves exactly a supplied key set back topending.apps/web/src/lib/cloud-agent-next/release-refused-send.ts: after a failed send, the rows are handed back only when the failure is the Worker's own error response with a 4xx status, which the Worker sends before it admits anything. A transport failure, a timeout, or a server-side status keeps the rows linked, and the helper never throws, so the send's own error is what the caller sees.sendMessagehandlers keep the keys their link returned and pass them to that helper on the failure path.prepareSessionis unchanged: a failed prepare does not tell the caller the session id, so a refusal cannot be attributed to a specific turn there.Retention after a failed send
A send the Worker holds, and any send whose outcome the web cannot attribute, keeps its attachment rows: releasing them could remove an object an accepted message references. A send the Worker answered with a refusal links its own rows and hands exactly those back to the reaper, so an abandoned attachment no longer waits for account deletion. Because the composer reuses one attachment path across sends, the release is bound to the keys this dispatch linked rather than to the path.
Production prerequisites
None.
Proxy answers
The owner's proxy gave these answers in the owner's place.
kiloprovider to the already configured control-bot credential for this session, then restore it after publication? This affects new localkiloprovider lookups; gate code, rules, production configuration, and account provisioning stay unchanged. Answer: Temporary credential routing Confidence: 0.60