fix(scratchlist): persist manual ordering and improve draft actions - #1611
fix(scratchlist): persist manual ordering and improve draft actions#1611techotaku39 wants to merge 33 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- 3 Major and 2 Minor findings; see inline comments.
Summary
Review mode: initial
Scheduled attachment delivery and cleanup have paths that can leave messages undeliverable or retain orphaned files. Attachment-only editing and thumbnail caching also have edge-case regressions.
Testing
Not run (automation; PR code was not executed). Static review of the full diff and surrounding source only.
HAPI Bot
| let deliveryContent: unknown = contentForDeferredDelivery(msg.content) | ||
| if (hasAttachments) { | ||
| try { | ||
| deliveryContent = await this.getScheduledDeliveryContent(msg) |
There was a problem hiding this comment.
[MAJOR] Route every scheduled attachment delivery through materialization
This is the only path that replaces a durable hapi-hub:scratchlist/... path with a CLI-local upload path. Other delivery paths still emit the stored content directly: an already-due scheduledAt goes through sendMessage at hub/src/sync/messageService.ts:738, reconnect backfill goes through getDeliverableMessagesAfter at hub/src/sync/messageService.ts:454, and OpenCode clear releases through releaseDeliverableQueuedMessages at hub/src/sync/messageService.ts:834. In each case the CLI receives a Hub-only path and cannot read the attachment.
Suggested fix:
private async contentForCliDelivery(msg: StoredMessageForDelivery): Promise<unknown | null> {
if (msg.scheduledAt !== null && getUserMessageAttachments(msg.content).length > 0) {
return await this.getScheduledDeliveryContent(msg)
}
return contentForDeferredDelivery(msg.content)
}
// Use/await this helper from sendMessage, reconnect backfill,
// releaseDeliverableQueuedMessages, and the maturity tick.Alternatively, exclude every scheduled row from CLI backfill/direct release and make the maturity path the sole delivery channel, as the route comment already states. Evidence: hub/src/sync/messageService.ts:929.
| ): Promise<AttachmentMetadata[]> { | ||
| const session = this.store.sessions.getSession(sessionId) | ||
| if (!session) throw new Error('Session not found') | ||
| const checked = await this.resolveScratchlistAttachmentsForSession( |
There was a problem hiding this comment.
[MAJOR] Re-home scheduled attachment paths when messages change sessions
Materialization validates each path against the message's current sessionId. Session merge/dedup changes message ownership via mergeSessionMessages (hub/src/sync/sessionCache.ts:1120), and OpenCode clear moves uninvoked messages at hub/src/sync/syncEngine.ts:2347, but neither rewrites the attachment path embedded in message content. Because scheduling removes the scratchlist row, scratchlist transfer often cannot discover that file; a merge before maturity therefore makes validation fail forever, and source-session cleanup may delete the blob.
Suggested fix:
const scheduled = this.store.messages.getUninvokedHubAttachments(oldSessionId)
const pathMap = await moveScheduledAttachmentFilesForSession(
hapiHome,
namespace,
oldSessionId,
newSessionId,
scheduled,
)
this.store.messages.rewriteUninvokedAttachmentPaths(
oldSessionId,
newSessionId,
pathMap,
)Run this as part of every message move before deleting/archiving attachment storage. Evidence: hub/src/sync/syncEngine.ts:968.
| } | ||
| } | ||
|
|
||
| async releaseConsumedScheduledAttachments(sessionId: string, localIds: string[]): Promise<void> { |
There was a problem hiding this comment.
[MAJOR] Release durable blobs when a scheduled message is cancelled
Cleanup is wired only to messages-consumed. The future-schedule cancellation branch deletes the DB row and emits message-cancelled at hub/src/sync/messageService.ts:515, but never deletes its Hub attachment. The web removes the scratchlist entry immediately after scheduling, so the cancelled file has no remaining owner and continues counting toward the 100 MB per-session disk quota until the entire session is deleted. Repeated schedule/cancel operations can block future uploads.
Suggested fix:
const cancelledMessage = lookup.message
if (this.store.messages.deleteQueuedMessageById(sessionId, resolvedId)) {
await this.releaseScheduledAttachmentsForMessages(
sessionId,
[cancelledMessage],
)
}The shared helper should delete only when neither another uninvoked message nor a remaining scratchlist entry references the path, and it should be called from every cancellation-delete branch. Evidence: hub/src/sync/messageService.ts:177.
| const nextText = editingText.trim() | ||
| setEditingEntryId(null) | ||
| setEditingText('') | ||
| if (nextText.length > 0 && nextText !== entry.text) { |
There was a problem hiding this comment.
[MINOR] Allow clearing text while attachments remain
The new editor only calls onUpdate when the trimmed text is non-empty. An entry containing text plus an image therefore cannot be edited into an attachment-only entry, even though the updated shared helper and Hub route explicitly allow empty text when attachments remain.
Suggested fix:
const hasAttachments = (entry.attachments?.length ?? 0) > 0
if (
nextText !== entry.text
&& (nextText.length > 0 || hasAttachments)
) {
void onUpdate(entry, nextText)
}Add a component test that clears the textarea on a text+attachment entry and expects onUpdate(entry, '', undefined). Evidence: web/src/components/AssistantChat/ScratchlistPanel.tsx:946.
| return current.src | ||
| } | ||
| revokeIfOwned(current) | ||
| previews.set(attachment.id, next) |
There was a problem hiding this comment.
[MINOR] Bound and revoke the thumbnail object-URL cache
The new module-level map never evicts entries, and the panel no longer revokes object URLs on unmount. Each session may hold 100 MB of scratchlist attachments, so opening attachment-heavy sessions can pin every downloaded blob for the lifetime of the PWA, including files whose rows were removed.
Suggested fix:
const MAX_CACHED_PREVIEWS = 64
function evictOldPreviews(): void {
while (previews.size > MAX_CACHED_PREVIEWS) {
const oldest = previews.entries().next().value
if (!oldest) return
const [id, preview] = oldest
revokeIfOwned(preview)
previews.delete(id)
}
}
// after previews.set(...)
evictOldPreviews()Also expose a per-attachment release function and call it after delete/remove so stale blobs are reclaimed promptly. Evidence: web/src/lib/scratchlistAttachmentPreview.ts:51.
There was a problem hiding this comment.
Findings
- [Major] Scheduled Hub attachments are not re-homed when OpenCode redirects the message — see inline comment at
hub/src/sync/messageService.ts:719. - [Major] A scheduled row can still emit after its queued state changes during attachment materialization — see inline comment at
hub/src/sync/messageService.ts:953. - [Major] One slow attachment upload blocks scheduled delivery for every session — see inline comment at
hub/src/sync/messageService.ts:926.
Questions
- None.
Summary
Review mode: follow-up after new commits
The prior HAPI Bot findings were rechecked against the latest full diff. Three correctness/liveness issues remain in scheduled attachment delivery.
Testing
Not run (automation; PR code was not executed). Static review of the full latest diff and surrounding source only. Missing coverage: redirected OpenCode schedules with attachments, cancellation during deferred materialization, and cross-session isolation from a stalled upload.
HAPI Bot
| throw new Error('sendMessage: scheduled messages with attachments must use scratchlist attachments') | ||
| } | ||
| if (payload.scheduledAt != null && attachments.length > 0) { | ||
| await this.options.validateScheduledAttachments?.(sessionId, attachments) |
There was a problem hiding this comment.
[MAJOR] Re-home attachments when insertion redirects the session
Validation here uses the requested source session, but addMessageForCurrentSession can redirect an active OpenCode clear source to its reserved replacement. The message is then stored under the replacement while its Hub path still embeds the source id. When sendScratchlistEntry removes the source scratchlist row, deleteScratchlistEntry checks message references only in the source session and deletes the blob; the replacement's maturity scan can never validate or read it.
Suggested fix:
const inserted = this.store.addMessageForCurrentSession(
sessionId,
content,
payload.localId ?? undefined,
payload.scheduledAt ?? null,
)
let msg = inserted.message
if (msg.scheduledAt !== null && inserted.sessionId !== sessionId && attachments.length > 0) {
await this.options.rehomeScheduledMessageAttachments?.(
sessionId,
inserted.sessionId,
[msg],
)
msg = this.store.messages.getAllMessages(inserted.sessionId)
.find((row) => row.id === msg.id) ?? msg
}The callback should move the file and rewrite the stored content before the HTTP send returns. Add a regression test based on the existing active-clear redirect case with a Hub attachment.
| let deliveryContent: unknown = contentForDeferredDelivery(msg.content) | ||
| if (hasAttachments) { | ||
| try { | ||
| deliveryContent = await this.getScheduledDeliveryContent(msg) |
There was a problem hiding this comment.
[MAJOR] Re-check queued state after asynchronous materialization
This await introduces a long interleaving point, but the code emits the original mature snapshot unconditionally afterward. While the RPC is pending, cancellation or a consumption transition can delete or invoke the row; once upload resolves, the stale snapshot is still delivered to the CLI even though Hub/Web no longer considers it queued.
Suggested fix:
deliveryContent = await this.getScheduledDeliveryContent(msg)
const current = this.store.messages.lookupQueuedMessage(msg.sessionId, msg.id)
if (current.status !== 'queued') {
this.scheduledAttachmentDeliveryCache.delete(`${msg.sessionId}:${msg.id}`)
continue
}Also track materializing message ids so cancelQueuedMessage can delete an item that has not yet been emitted instead of interpreting the CLI's not-found response as already consumed. Test with a deferred materializer, cancel before resolving it, then assert no CLI update is emitted.
| localId: msg.localId, | ||
| content: contentForDeferredDelivery(msg.content) | ||
| async releaseMatureScheduledMessages(now: number, skipSessionIds?: ReadonlySet<string>): Promise<void> { | ||
| if (this.matureReleaseInFlight) return |
There was a problem hiding this comment.
[MAJOR] Avoid a Hub-wide lock for scheduled attachment uploads
matureReleaseInFlight serializes every session behind the slowest materialization. materializeScheduledAttachments performs sequential RPC uploads, each with the normal RPC timeout, so one connected but stalled CLI can delay unrelated text-only scheduled messages across all namespaces for tens of seconds per attachment; later 5-second ticks return immediately while the flag is set.
Suggested fix:
private readonly matureReleaseInFlightSessions = new Set<string>()
const bySession = new Map<string, StoredMessageForDelivery[]>()
for (const msg of mature) {
const rows = bySession.get(msg.sessionId) ?? []
rows.push(msg)
bySession.set(msg.sessionId, rows)
}
await Promise.all([...bySession].map(async ([sessionId, messages]) => {
if (this.matureReleaseInFlightSessions.has(sessionId)) return
this.matureReleaseInFlightSessions.add(sessionId)
try {
await this.releaseMatureForSession(messages, skipSessionIds)
} finally {
this.matureReleaseInFlightSessions.delete(sessionId)
}
}))Keep FIFO within each session, but let independent sessions progress concurrently. Add a test where session A's materializer is deferred and session B's mature text message still emits.
There was a problem hiding this comment.
Findings
- [Major] Clear-abort can report success before scheduled attachments are re-homed — see inline comment at
hub/src/sync/syncEngine.ts:2177. - [Major] Scheduled attachment blobs are still deletable through non-entry deletion paths — see inline comment at
hub/src/sync/syncEngine.ts:820. - [Major] Partial multi-attachment materialization leaks duplicate CLI uploads on every retry — see inline comment at
hub/src/sync/syncEngine.ts:996.
Questions
- None.
Summary
Review mode: follow-up after new commits
The previous bot findings were rechecked against the complete latest diff. The new commits address cross-session blocking and cancellation during materialization, but three scheduled-attachment durability/resource issues remain.
Testing
Not run (automation; PR code was not executed). Static review of the complete latest diff and surrounding source; git diff --check passed. Missing coverage: clear-abort re-home failure/retry, explicit deletion while a schedule references the blob, and partial failure during multi-attachment staging.
HAPI Bot
| }, latest.metadataVersion, namespace, required) | ||
| if (result.result === 'success') { | ||
| this.sessionCache.refreshSession(sessionId) | ||
| void rehomeMessageAttachments( |
There was a problem hiding this comment.
[MAJOR] Do not acknowledge clear-abort before attachment re-home succeeds
abortOpenCodeClearOperation has already moved the uninvoked rows back to sessionId, but this fire-and-forget re-home runs after the operation is persisted as aborted. If the filesystem move or message rewrite fails, this method still returns success, and later calls take the early state === 'aborted' success branch without retrying. The scheduled row then permanently contains a Hub path owned by the replacement session, so maturity validation keeps failing and the prompt never delivers.
Suggested fix:
async abortOpenCodeClearSession(/* ... */): Promise<ClearOpencodeSessionResult> {
// Also run this reconciliation in the existing `state === 'aborted'` branch
// so a previous partial failure is retryable.
if (result.result === 'success') {
await rehomeMessageAttachments(
this.store,
namespace,
current.replacementSessionId,
sessionId,
sourceMessages,
)
this.sessionCache.refreshSession(sessionId)
return { type: 'success', sessionId }
}
}Update callers to await the result, and add a failure-then-retry regression test.
| ) | ||
| const orphaned = existing.attachments.filter((att) => !remainingIds.has(att.id)) | ||
| if (orphaned.length > 0) { | ||
| const deletable = orphaned.filter( |
There was a problem hiding this comment.
[MAJOR] Apply scheduled-message reference protection to every blob deletion path
This guard protects attachments when an entire scratchlist entry is deleted, but the other deletion paths do not use it: the PUT orphan cleanup at hub/src/web/routes/sessions.ts:1219 and DELETE /scratchlist/attachments/:attachmentId at hub/src/web/routes/sessions.ts:1243 only inspect scratchlist rows. Once a scheduled send owns the Hub path, either path can delete the blob and make the mature message retry forever.
Suggested fix:
const canDeleteScratchlistAttachment = (
sessionId: string,
attachment: ScratchlistAttachmentMetadata,
) => !store.messages.hasUninvokedAttachmentReference(sessionId, attachment.path)
&& !store.scratchlist.list(sessionId).some((entry) =>
entry.attachments.some((item) => item.path === attachment.path)
)Centralize this check in SyncEngine and use it for entry deletion, PUT orphan cleanup, and the explicit attachment DELETE route (return 409 while a pending schedule references the path).
| for (const attachment of checked.attachments) { | ||
| const read = await readScratchlistAttachmentFile(getHapiHomeDir(), attachment.path) | ||
| if (!read) throw new Error(`Scheduled attachment file missing: ${attachment.filename}`) | ||
| const uploaded = await this.rpcGateway.uploadFile( |
There was a problem hiding this comment.
[MAJOR] Clean up successful uploads when a later attachment fails
Materialization uploads attachments sequentially. If attachment 1 succeeds and attachment 2 fails, the successful CLI path is discarded because the method throws before the delivery cache is populated. The five-second maturity scan retries the whole batch, uploading attachment 1 again each time; a persistent later failure can grow the CLI upload directory without bound until session cleanup.
Suggested fix:
const stagedPaths: string[] = []
try {
for (const attachment of checked.attachments) {
const uploaded = await this.rpcGateway.uploadFile(/* ... */)
if (!uploaded.success || !uploaded.path) throw new Error(/* ... */)
stagedPaths.push(uploaded.path)
materialized.push({ ...attachment, path: uploaded.path })
}
return materialized
} catch (error) {
await Promise.allSettled(
stagedPaths.map((path) => this.rpcGateway.deleteUploadFile(sessionId, path))
)
throw error
}Add a two-attachment test where the second upload fails and assert the first staged path is deleted before retry.
There was a problem hiding this comment.
Findings
- [Major] Attachment deletion checks are not serialized with scheduled-message acceptance; a queued message can retain a deleted Hub blob. Evidence:
hub/src/sync/syncEngine.ts:973.
Suggested fix:return this.withScratchlistAttachmentLock(namespace, sessionId, async () => { if (!this.canDeleteScratchlistAttachment(sessionId, { id: attachmentId })) return false return deleteById(getHapiHomeDir(), namespace, sessionId, attachmentId) }) // Use the same lock around scheduled validation + insertion.
- [Major] Cancelling during materialization leaks the completed CLI upload. Evidence:
hub/src/sync/messageService.ts:1034.
Suggested fix:const staged = this.scheduledAttachmentDeliveryCache.get(materializingKey) ?? [] if (current.status !== 'queued') { await this.options.deleteMaterializedScheduledAttachments?.(sessionId, staged) this.scheduledAttachmentDeliveryCache.delete(materializingKey) continue }
- [Major] Re-home persists a destination path even when the file move fails. Evidence:
hub/src/sync/messageAttachmentTransfer.ts:56.
Suggested fix:const moved = await moveScratchlistAttachmentFilesForSession( hapiHome, namespace, oldSessionId, newSessionId, attachments, { throwOnFailure: true }, )
- [Minor] Concurrent complete-order mutations can commit out of order. Evidence:
web/src/lib/use-hub-scratchlist.ts:575.
Suggested fix:const reorderMutation = useMutation({ scope: { id: `scratchlist-reorder:${sessionId}` }, // existing mutationFn and optimistic handlers })
Questions
- None.
Summary
Review mode: follow-up after new commits
The previous clear-abort retry, static deletion guards, and partial-upload cleanup were rechecked against the complete latest diff. Four lifecycle/concurrency issues remain: three can orphan or invalidate scheduled attachments, and one can lose the latest manual order.
Testing
Not run (automation; PR code was not executed). Static review of the complete latest diff, prior review/compare context, and surrounding source; git diff --check passed. Missing coverage: schedule-vs-delete interleaving, cancel-after-upload cleanup, caught rename failure with no destination, and reverse-resolution reorder requests.
HAPI Bot
| namespace: string, | ||
| attachmentId: string | ||
| ): Promise<boolean> { | ||
| if (!this.canDeleteScratchlistAttachment(sessionId, { id: attachmentId })) return false |
There was a problem hiding this comment.
[MAJOR] Serialize deletion with scheduled-message acceptance
This check is only a snapshot. The following import, directory scan, and unlink all await. During that window another request can validate the same Hub path and insert a scheduled row at messageService.ts:750-779; this method then deletes the blob, so the mature scan retries forever. PUT orphan cleanup has the same check-then-fire-and-forget race.
Suggested fix:
return this.withScratchlistAttachmentLock(namespace, sessionId, async () => {
if (!this.canDeleteScratchlistAttachment(sessionId, { id: attachmentId })) return false
const { deleteScratchlistAttachmentById, getHapiHomeDir } =
await import('../scratchlistAttachments/storage')
return deleteScratchlistAttachmentById(
getHapiHomeDir(), namespace, sessionId, attachmentId,
)
})Use the same per-session lock around scheduled validation plus addMessageForCurrentSession, and route orphan cleanup through the locked method. Add a test that pauses deletion after the check, schedules the attachment, then resumes and asserts the file is retained.
| // snapshot after the row leaves the queued state. | ||
| const current = this.store.messages.lookupQueuedMessage(sessionId, msg.id) | ||
| if (current.status !== 'queued') { | ||
| this.scheduledAttachmentDeliveryCache.delete(materializingKey) |
There was a problem hiding this comment.
[MAJOR] Delete the staged CLI upload when cancellation wins
Cancellation deletes the DB row while uploadFile is pending. After the RPC succeeds, this branch suppresses the stale emit but only drops the cache entry. The uploaded CLI file remains; repeated upload-then-cancel cycles grow the session upload directory until cleanup.
Suggested fix:
const staged = this.scheduledAttachmentDeliveryCache.get(materializingKey) ?? []
if (current.status !== 'queued') {
await this.options.deleteMaterializedScheduledAttachments?.(sessionId, staged)
this.scheduledAttachmentDeliveryCache.delete(materializingKey)
continue
}Wire the callback to rpcGateway.deleteUploadFile. Extend the existing cancellation test to assert cleanup of the completed upload.
| continue | ||
| } | ||
|
|
||
| const moved = await moveScratchlistAttachmentFilesForSession( |
There was a problem hiding this comment.
[MAJOR] Do not rewrite message paths after a failed file move
moveScratchlistAttachmentFilesForSession catches rename failures at storage.ts:153-160 and still returns the destination path. This caller persists it. If the destination is absent, retries no longer recognize the old-session path and the scheduled message permanently points at a nonexistent file. The retry test only fails mkdir, before that catch.
Suggested fix:
const moved = await moveScratchlistAttachmentFilesForSession(
hapiHome,
namespace,
oldSessionId,
newSessionId,
attachments,
{ throwOnFailure: true },
)
// rewrite only after each destination is confirmed to existA strict message-rehome helper is also fine if legacy scratchlist transfers need best-effort behavior. Add a rename-failure-then-retry test with no destination file.
| .filter((entry): entry is HubEntry => Boolean(entry)) | ||
| if (next.length !== current.entries.length) return | ||
| if (next.every((entry, index) => entry.entryId === current.entries[index]?.entryId)) return | ||
| reorderMutation.mutate(next) |
There was a problem hiding this comment.
[MINOR] Serialize complete-order mutations
Each drag sends a full ID order, but multiple mutate calls can run concurrently. If drag B follows A and request A finishes last, A's server write and onSuccess overwrite the newer B order.
Suggested fix:
const reorderMutation = useMutation({
scope: { id: `scratchlist-reorder:${sessionId}` },
mutationFn: async (entries: HubEntry[]) => api.reorderScratchlistEntries(
sessionId,
entries.map((entry) => entry.entryId),
),
// keep existing optimistic handlers
})TanStack Query v5 serializes mutations sharing a scope. Add a test with two rapid reorders whose responses are released in reverse order.
There was a problem hiding this comment.
Findings
- [Major] Scheduled blob cleanup still uses a stale reference snapshot — a new scheduled row can be inserted after the checks but before the locked delete, leaving that row with a missing Hub file. Evidence:
hub/src/sync/messageService.ts:219.
Suggested fix:await this.withScheduledAttachmentLock(sessionId, async () => { const scratchlistPaths = new Set(/* re-read current scratchlist refs */) const deletable = [...unique.values()].filter((attachment) => !this.store.messages.hasUninvokedAttachmentReference(sessionId, attachment.path) && !scratchlistPaths.has(attachment.path) ) await this.options.deleteScheduledAttachments?.(sessionId, deletable) })
- [Major] Attachment validation runs before local-id deduplication — after the first accepted request re-homes or cleans the source blob, retrying a lost response fails validation instead of returning the existing scheduled row. Evidence:
hub/src/sync/messageService.ts:784.
Suggested fix:const existing = payload.localId ? this.store.getMessageForCurrentSession(sessionId, payload.localId) : null if (!existing && payload.scheduledAt != null && attachments.length > 0) { await this.options.validateScheduledAttachments?.(sessionId, attachments) } const inserted = this.store.addMessageForCurrentSession(/* existing args */)
- [Major] Attachment-only local entries are silently skipped during migration — the loop still continues on empty text before the newly added attachment payload is sent, then marks migration complete and allows the local cache to be overwritten. Evidence:
web/src/lib/use-hub-scratchlist.ts:294.
Suggested fix:const attachments = stripPreviewUrls(entry.attachments ?? []) if (text.trim().length === 0 && attachments.length === 0) continue await api.createScratchlistEntry(sessionId, { text, entryId: entry.id, createdAt: entry.createdAt, position: entry.position ?? position, attachments, })
- [Major] Strict re-home can partially delete source files and then abort metadata rewriting — if one target-wins cleanup fails after another succeeds, the catch path cannot restore deleted sources, so messages retain old paths that no longer exist. Evidence:
hub/src/scratchlistAttachments/storage.ts:178.
Suggested fix:// Every destination already exists here; source cleanup must not abort the rewrite. await Promise.allSettled(completedMoves .filter((move) => move.destinationExisted) .map((move) => rm(move.oldPath, { force: true }))) return moved
Questions
- None.
Summary
Review mode: follow-up after new commits
The latest commit addresses the previously reported direct deletion, cancelled-upload, rename, and reorder paths, but four related lifecycle cases remain. Two can leave scheduled messages pointing at missing files, one breaks lost-response idempotency, and one drops attachment-only drafts during migration.
Testing
Not run (automation; PR code was not executed). Static review of the complete latest diff, prior review/compare context, and surrounding source; git diff --check passed. Missing coverage: cleanup-vs-new-schedule interleaving, lost-response retry after re-home, attachment-only local migration, and partial target-wins cleanup failure.
HAPI Bot
| (attachment) => !this.store.messages.hasUninvokedAttachmentReference(sessionId, attachment.path) | ||
| && !scratchlistPaths.has(attachment.path) | ||
| ) | ||
| if (deletable.length === 0) return |
There was a problem hiding this comment.
[MAJOR] Keep the reference re-check inside the shared lock
scratchlistPaths and hasUninvokedAttachmentReference are evaluated before this callback acquires the lock in SyncEngine. A new scheduled send can therefore acquire the same lock, validate the file, and insert its row between this snapshot and the delete; cleanup then removes a blob referenced by the new row.
Suggested fix:
await this.withScheduledAttachmentLock(sessionId, async () => {
const scratchlistPaths = new Set(
this.store.scratchlist.list(sessionId)
.flatMap((entry) => entry.attachments.map((attachment) => attachment.path))
)
const deletable = [...unique.values()].filter((attachment) =>
!this.store.messages.hasUninvokedAttachmentReference(sessionId, attachment.path)
&& !scratchlistPaths.has(attachment.path)
)
await this.options.deleteScheduledAttachments?.(sessionId, deletable)
})Remove the nested lock from the SyncEngine callback, and add an interleaving test that inserts a second scheduled reference after cleanup starts but before deletion.
| ) | ||
| const insertMessage = async () => { | ||
| if (payload.scheduledAt != null && attachments.length > 0) { | ||
| await this.options.validateScheduledAttachments?.(sessionId, attachments) |
There was a problem hiding this comment.
[MAJOR] Deduplicate before validating a retry's old Hub path
The local-id lookup currently happens inside addMessageForCurrentSession, after this validation. On the first accepted request, re-home may move the attachment from the source session to an OpenCode replacement; normal consumed cleanup can also remove it later. If the HTTP response was lost, retrying the same localId and original payload now fails here even though the durable message already exists.
Suggested fix:
const existing = payload.localId
? this.store.getMessageForCurrentSession(sessionId, payload.localId)
: null
if (!existing && payload.scheduledAt != null && attachments.length > 0) {
await this.options.validateScheduledAttachments?.(sessionId, attachments)
}
const inserted = this.store.addMessageForCurrentSession(
sessionId, content, payload.localId ?? undefined, payload.scheduledAt ?? null,
)The helper should resolve the same redirect target as insertion. Add a lost-response retry test where the first request re-homes the blob and the second returns the existing row.
| createdAt: entry.createdAt | ||
| createdAt: entry.createdAt, | ||
| position: entry.position ?? position, | ||
| attachments: entry.attachments ?? [] |
There was a problem hiding this comment.
[MAJOR] Preserve attachment-only entries during local migration
The loop still executes if (text.trim().length === 0) continue above this new attachment field. An attachment-only cached draft is therefore neither POSTed nor recorded as failed; the migration flag is set, the Hub query is invalidated, and the later cache mirror removes the only local copy.
Suggested fix:
const attachments = stripPreviewUrls(entry.attachments ?? [])
if (text.trim().length === 0 && attachments.length === 0) continue
await api.createScratchlistEntry(sessionId, {
text,
entryId: entry.id,
createdAt: entry.createdAt,
position: entry.position ?? position,
attachments,
})Add a migration test with text: '' and one attachment, asserting the POST occurs and local storage is not discarded before reconciliation.
| if (options.throwOnFailure) { | ||
| // Defer target-wins source cleanup until every rename succeeds so a | ||
| // later failure can leave all metadata pointing at valid old paths. | ||
| await Promise.all(completedMoves |
There was a problem hiding this comment.
[MAJOR] Do not abort the rewrite after destinations are already valid
This Promise.all can partially succeed. If removing source A succeeds and removing source B fails, control enters the catch path; renamed files are rolled back, but source A cannot be restored because its destination pre-existed. The caller then skips metadata rewriting, leaving attachment A pointed at a deleted old path. A persistent failure on B keeps retries from repairing A.
Suggested fix:
// All destination files are confirmed at this point. Cleanup is best-effort;
// metadata must advance to the valid destination paths.
await Promise.allSettled(completedMoves
.filter((move) => move.destinationExisted)
.map((move) => rm(move.oldPath, { force: true })))
return movedAlternatively return cleanup failures separately and persist the rewrites before retrying orphan deletion. Add a two-file test where one target-wins source removal succeeds and the other fails.
There was a problem hiding this comment.
Findings
-
[Major] Editing a queued scheduled message drops its attachments — the new schema accepts scheduled scratchlist attachments, but Edit still cancels the row and restores only text plus schedule. For an attachment-only row it restores filenames as text, while cancellation can release the Hub file. Evidence
shared/src/apiTypes.ts:554, related contextweb/src/components/AssistantChat/QueuedMessagesBar.tsx:394andweb/src/components/AssistantChat/QueuedMessagesBar.tsx:470.
Suggested fix:const canEdit = canCancel && !(msg.scheduledAt != null && hasAttachments)
-
[Major] Cached materialized CLI paths survive a lost session-end event — the cache stores paths inside a CLI upload directory, but is cleared only by explicit session-end. The CLI deletes that directory on process exit, so a resumed session can receive a cached path that no longer exists. Evidence
hub/src/sync/messageService.ts:976, related contexthub/src/sync/syncEngine.ts:617andcli/src/modules/common/handlers/uploads.ts:157.
Suggested fix:handleCliSessionConnected(sessionId: string): void { this.messageService.clearScheduledAttachmentDeliveryCache(sessionId) }
-
[Major] Scratchlist creation is outside the attachment lifecycle lock — scheduled cleanup now snapshots scratchlist references under the lock, but the create route validates the file before an unlocked insert. Cleanup can delete the file between validation and insertion, leaving a new entry with a dangling path. Evidence
hub/src/sync/messageService.ts:210, related contexthub/src/sync/syncEngine.ts:752andhub/src/web/routes/sessions.ts:1075.
Suggested fix:return this.withScratchlistAttachmentLock(namespace, sessionId, async () => { const checked = await this.resolveScratchlistAttachmentsForSession(sessionId, namespace, attachments) if (!checked.ok) return checked return this.createScratchlistEntryLocked(sessionId, text, { ...options, attachments: checked.attachments }) })
Questions
- None.
Summary
Review mode: follow-up after new commits
Three Major correctness/data-loss risks remain in the latest full diff: queued attachment editing, stale materialized-path reuse across CLI connection generations, and a create-versus-cleanup attachment race.
Testing
- Not run (automation; PR code was not executed). Static review of the complete latest diff, prior review/compare context, and surrounding source; git diff --check passed.
- Missing coverage: scheduled attachment Edit behavior; CLI exit/reconnect after materialization but before acknowledgement; scratchlist create interleaved with scheduled attachment cleanup.
HAPI Bot
| { message: 'scheduled messages with attachments are not supported', path: ['attachments'] } | ||
| (data) => data.scheduledAt == null | ||
| || !data.attachments?.length | ||
| || data.attachments.every((attachment) => isHubScratchlistAttachmentPath(attachment.path)), |
There was a problem hiding this comment.
[MAJOR] Editing a queued scheduled message drops its attachments
This refinement newly accepts scheduled scratchlist attachments, but QueuedMessagesBar.handleEdit still implements Edit as cancel plus restoration of only editText and the schedule (web/src/components/AssistantChat/QueuedMessagesBar.tsx:394-462). getQueuedMessageEditText substitutes filenames when there is no text, and cancellation may then release the Hub file. An attachment-only scheduled message therefore becomes filename text and loses the actual attachment.
Suggested fix:
const canEdit = canCancel && !(msg.scheduledAt != null && hasAttachments)Alternatively, rehydrate the attachments before cancellation and restore text, schedule, and attachments atomically. Add a test for an attachment-only scheduled row.
| if (!deliveryAttachments) { | ||
| if (!this.options.materializeScheduledAttachments) return null | ||
| deliveryAttachments = await this.options.materializeScheduledAttachments(msg.sessionId, attachments) | ||
| this.scheduledAttachmentDeliveryCache.set(cacheKey, deliveryAttachments) |
There was a problem hiding this comment.
[MAJOR] Scope cached materialized paths to a live CLI connection generation
The cached values are paths inside the current CLI upload directory. They are cleared on explicit session-end, but cleanupUploadDirsSync removes those directories when the CLI process exits. If the process dies before its end event reaches the Hub, a later CLI connection for the same session reuses this cache entry and receives a path that was deleted on the old host process. The scheduled row can then be consumed without a usable attachment.
Suggested fix:
handleCliSessionConnected(sessionId: string): void {
this.messageService.clearScheduledAttachmentDeliveryCache(sessionId)
}Call this once when a new CLI socket joins the session room, or otherwise key the cache by connection generation. Add a test that materializes, loses the CLI before acknowledgement, reconnects, and verifies a fresh upload path.
| .filter((message) => message.scheduledAt !== null) | ||
| .flatMap((message) => getUserMessageAttachments(message.content)) | ||
| .filter((attachment) => isHubScratchlistAttachmentPath(attachment.path)) | ||
| await this.withScheduledAttachmentLock(sessionId, async () => { |
There was a problem hiding this comment.
[MAJOR] Include scratchlist creation in the attachment lifecycle lock
The cleanup now re-reads scratchlist references while holding the lock, but POST /scratchlist resolves and validates attachment files before calling the synchronous, unlocked createScratchlistEntry. A cleanup can acquire this lock after validation, snapshot no new entry, delete the blob, and then let the route insert a row that references the deleted path.
Suggested fix:
return this.withScratchlistAttachmentLock(namespace, sessionId, async () => {
const checked = await this.resolveScratchlistAttachmentsForSession(sessionId, namespace, attachments)
if (!checked.ok) return checked
return this.createScratchlistEntryLocked(sessionId, text, { ...options, attachments: checked.attachments })
})The route must await this operation so revalidation and insertion remain inside the same critical section. Add an interleaving test where cleanup holds the lock while creation targets the same path.
There was a problem hiding this comment.
Findings
-
[Major] Reconnect cache invalidation still misses ordinary agents - the new invalidation runs only from
session-ready, but ordinary socket reconnects emitsession-alive;emitSessionReady()is limited to AGY, Cursor ACP, and Pi callers. Claude/Codex-style sessions that losesession-endcan therefore retain materialized paths from the previous CLI process. Evidencehub/src/sync/syncEngine.ts:603, related contextcli/src/api/apiSession.ts:327.
Suggested fix:// Run once when an authenticated session-scoped socket joins its room. onSessionConnected?.(sessionId) handleSessionConnected(sessionId: string): void { this.messageService.clearScheduledAttachmentDeliveryCache(sessionId) }
-
[Major] Scheduled scratchlist send can delete the Hub blob before the send is persisted -
onSendreturns an attempt id immediately after starting the mutation, not after the HTTP request succeeds. The followingscratchlist.removeraces the scheduled-message validation/insert; if deletion gets the attachment lock first, it sees no message reference and removes the file, so the send fails with an unretryable attachment path. Evidenceweb/src/components/SessionChat.tsx:852, related contextweb/src/hooks/mutations/useSendMessage.ts:350.
Suggested fix:const acceptance = await props.onSend(...) if (!acceptance) return false pendingScratchlistSendsRef.current.set(acceptance.attemptId, entry.id) return true // Remove only after props.sendSettlement reports success for this attempt.
-
[Major] Attachment materialization failure breaks scheduled FIFO - mature rows are ordered oldest-first, but a failed/null materialization uses
continue, allowing later scheduled prompts in the same session to be emitted ahead of the blocked row. Evidencehub/src/sync/messageService.ts:1057, related contexthub/src/store/messages.ts:620.
Suggested fix:try { deliveryContent = await this.getScheduledDeliveryContent(msg) } catch { break } if (deliveryContent === null) break
-
[Major] Multi-message attachment re-home can leave earlier rows pointing at moved-away files - each message's files are moved independently, while every database rewrite is deferred until the entire loop finishes. If a later move throws, earlier moves remain on disk but none of their message paths are updated. Evidence
hub/src/sync/messageAttachmentTransfer.ts:69.
Suggested fix:const moved = await moveScratchlistAttachmentFilesForSession(...) if (moved.some((attachment, index) => attachment.path !== attachments[index]?.path)) { store.messages.rewriteMessageAttachments(newSessionId, [ { messageId, attachments: moved }, ]) }
Questions
- None.
Summary
Review mode: follow-up after new commits
Four Major correctness/data-loss risks remain in the latest full diff. One prior reconnect-cache finding is only partially addressed; the other findings concern scheduled scratchlist delivery ordering and attachment ownership during asynchronous send/session-transfer flows.
Testing
- Not run (automation; PR code was not executed). Static review of the complete latest diff, previous HAPI Bot review, follow-up compare, and surrounding source;
git diff --checkpassed. - Missing coverage: generic Claude/Codex socket reconnect after a lost
session-end; scheduled scratchlist send where draft deletion wins the server lock; two mature rows where the first attachment materialization fails; multi-message re-home where a later filesystem move fails.
HAPI Bot
| // Materialized attachment paths belong to the previous CLI process. | ||
| // A reconnect may not deliver the old session-end event, so never | ||
| // reuse those host-local paths after the new CLI becomes ready. | ||
| this.messageService.clearScheduledAttachmentDeliveryCache(payload.sid) |
There was a problem hiding this comment.
[MAJOR] Invalidate materialized paths on every session-scoped socket connection
This only runs for session-ready. Generic ApiSession reconnects emit session-alive from the socket connect callback, while emitSessionReady() has only AGY, Cursor ACP, and Pi callers. A Claude/Codex process that exits without delivering session-end can therefore reconnect under the same session id and reuse cached paths from the deleted previous-process upload directory.
Suggested fix:
// In registerCliHandlers, immediately after the authenticated room join:
onSessionConnected?.(sessionId)
// Wire this once-per-socket callback to:
handleSessionConnected(sessionId: string): void {
this.messageService.clearScheduledAttachmentDeliveryCache(sessionId)
}Do not clear on every session-alive heartbeat; the callback should represent a new socket connection generation. Add a generic ApiSession reconnect test, not only a direct cache-clear unit test.
| hubItems.map((attachment) => props.api.deleteScratchlistAttachment(props.session.id, attachment.id)) | ||
| ) | ||
| } | ||
| await scratchlist.remove(entry.id) |
There was a problem hiding this comment.
[MAJOR] Wait for send settlement before removing a scheduled attachment draft
props.onSend returns { attemptId } immediately after mutation.mutate(); it does not await api.sendMessage. This removal therefore races the Hub's scheduled attachment validation/insert. If the DELETE acquires the attachment lock first, it removes the scratchlist row and blob before any uninvoked message references it, then the POST fails validation. The failed optimistic message retains a Hub path that no longer exists.
Suggested fix:
const acceptance = await props.onSend(...)
if (!acceptance) return false
pendingScratchlistSendsRef.current.set(acceptance.attemptId, entry.id)
return true
useEffect(() => {
const settlement = props.sendSettlement
if (!settlement) return
const entryId = pendingScratchlistSendsRef.current.get(settlement.attemptId)
if (!entryId) return
pendingScratchlistSendsRef.current.delete(settlement.attemptId)
if (settlement.status === 'success') void scratchlist.remove(entryId)
}, [props.sendSettlement, scratchlist])Keep the entry and Hub blob on error. Add an interleaving test where draft deletion would otherwise win the server-side lock.
| } catch { | ||
| // Keep the row uninvoked. The next tick retries after | ||
| // the file or CLI connection becomes available. | ||
| continue |
There was a problem hiding this comment.
[MAJOR] Stop the session batch when the head scheduled row cannot materialize
getMatureScheduledMessages supplies oldest-first rows, but this continue skips a failed attachment row and emits later prompts from the same session. That violates the queue's FIFO contract and can deliver dependent instructions out of order.
Suggested fix:
try {
deliveryContent = await this.getScheduledDeliveryContent(msg)
} catch {
break
}
if (deliveryContent === null) breakAlternatively, explicitly fail/cancel the head row before proceeding. Add a test with an older attachment row whose materializer rejects and a newer text-only row; neither later row should emit while the head remains queued.
| } | ||
| } | ||
|
|
||
| store.messages.rewriteMessageAttachments(newSessionId, rewrites) |
There was a problem hiding this comment.
[MAJOR] Do not defer all path rewrites until after independent file moves
The loop moves files one message at a time, but this single rewrite happens only after every move succeeds. If message 1 moves successfully and message 2 throws, message 1's target row still contains the old-session path even though its file now exists only under the new session directory.
Suggested fix:
const moved = await moveScratchlistAttachmentFilesForSession(...)
if (moved.some((attachment, index) => attachment.path !== attachments[index]?.path)) {
store.messages.rewriteMessageAttachments(newSessionId, [
{ messageId, attachments: moved },
])
}This keeps each completed move consistent even if a later message fails. An all-files single rollback domain is also valid. Add a two-message test where the second move fails and verify the first row still resolves to an existing file.
There was a problem hiding this comment.
Findings
- [Major]
session-readyinvalidates scheduled attachment uploads a second time within the same socket generation — the session-scoped socket joins its room before agent readiness, and the 5-second mature scan can materialize/cache an attachment during that interval. CallinghandleSessionConnectedagain at readiness drops that live mapping, so the next scan uploads the same blob again and may enqueue a duplicate path while the first upload is left orphaned. Evidencehub/src/sync/syncEngine.ts:607, related contexthub/src/sync/messageService.ts:971,hub/src/sync/syncEngine.ts:1173,cli/src/api/apiSession.ts:327.
Suggested fix:handleSessionReady(payload: { sid: string; time: number }): void { // The connection-generation cache reset already ran from onSessionConnected. this.sessionReadyIds.add(payload.sid) // existing ready handling... }
Questions
- None.
Summary
Review mode: follow-up after new commits
One Major correctness/resource-lifecycle issue remains in the latest full diff. The prior findings were rechecked against the fresh head; the remaining issue is the duplicate cache reset between socket connection and agent readiness.
Testing
- Not run (automation; PR code was not executed). GitHub
testandintegrationchecks passed. - Add a lifecycle regression test: connect a session-scoped socket, materialize one mature scheduled attachment, emit
session-ready, run the mature scan again, and assert the upload RPC is called once and the cached delivery path is reused.
HAPI Bot
| } | ||
|
|
||
| handleSessionReady(payload: { sid: string; time: number }): void { | ||
| this.handleSessionConnected(payload.sid) |
There was a problem hiding this comment.
[MAJOR] Do not reset the delivery cache again at session-ready
onSessionConnected now clears the cache when the authenticated socket joins the session room. That room is already visible to the 5-second mature scan, while Cursor/Pi/AGY may emit session-ready later. If a scheduled attachment is materialized in that interval, this second reset discards the valid CLI-upload path; the next scan uploads the same blob again and can enqueue a duplicate path, leaving the first upload orphaned.
Suggested fix:
handleSessionReady(payload: { sid: string; time: number }): void {
// Cache generation was reset by onSessionConnected.
this.sessionReadyIds.add(payload.sid)
// existing ready handling...
}Add a socket-lifecycle test covering connect → materialize → session-ready → next scan and assert one upload RPC.
There was a problem hiding this comment.
Findings
-
[Major] Attachment validation still occurs outside the lifecycle lock — the PUT route resolves the requested files and computes quota before
updateScratchlistEntryacquires the session lock. Two devices can therefore both validate the same attachment state; the first request can remove and delete a blob while the second is waiting, after which the second commits metadata pointing to the deleted file. Evidencehub/src/web/routes/sessions.ts:1169,hub/src/web/routes/sessions.ts:1204, related lock/cleanup contexthub/src/sync/syncEngine.ts:815.
Suggested fix:// One engine operation; acquire once, then re-read/resolve/quota-check/update/cleanup. return this.withScratchlistAttachmentLock(namespace, sessionId, async () => { const existing = this.store.scratchlist.get(sessionId, entryId) const checked = await this.resolveScratchlistAttachmentsForSession( sessionId, namespace, claimedAttachments, ) // Recompute disk quota here, then update and delete orphans before releasing. return this.updateScratchlistEntryLocked(existing, checked.attachments, text) })
-
[Minor] Mature cancel-as-invoked skips Hub attachment cleanup — the new cleanup runs for true deletes and normal
messages-consumedevents, but the existingnot-found/timeout branch stampsinvoked_atand returns without releasing the scheduled Hub blob. Because this method emits directly throughpublisher,SyncEngine.handleRealtimeEventis not called to perform the cleanup; once the draft is gone, the orphan remains counted against the session attachment quota. Evidencehub/src/sync/messageService.ts:544, related branchhub/src/sync/messageService.ts:645.
Suggested fix:this.store.messages.markMessagesInvoked(sessionId, [localId], invokedAt) await this.releaseCancelledScheduledAttachment(sessionId, message)
Questions
- None.
Summary
Review mode: follow-up after new commits
The new commit fixes the prior duplicate cache reset and adds a focused regression test. The latest full diff still has one Major attachment-consistency race and one Minor permanent-quota leak.
Testing
- Not run (automation; PR code was not executed). GitHub
testandintegrationchecks passed. - Add a concurrent PUT regression: pause one attachment resolve, remove the same attachment from another request, then resume and assert no row can commit a missing path.
- Add mature scheduled-attachment cancel tests for both CLI
not-foundand timeout outcomes; assert the unreferenced Hub blob is released.
HAPI Bot
| } | ||
|
|
||
| const updated = engine.updateScratchlistEntry( | ||
| const updated = await engine.updateScratchlistEntry( |
There was a problem hiding this comment.
[MAJOR] Keep attachment validation inside the same lifecycle lock
The route resolves nextAttachments and computes disk quota before this call acquires withScratchlistAttachmentLock. With two clients, request A can validate the current blob, request B can remove it and delete the file under the lock, and then A can enter the lock and save metadata for the now-missing path. The new inline attachment editing makes that a reachable multi-device data-loss race.
Suggested fix:
// Expose one engine operation that acquires once, then performs:
// re-read -> resolve -> quota check -> update -> orphan cleanup.
const updated = await engine.updateScratchlistEntryAtomic(
sessionResult.sessionId,
entryId,
parsed.data,
namespace,
)Do not wrap the current updateScratchlistEntry from the route, because it already acquires the same lock; move the preflight work into that locked engine operation. Add a delayed-resolve/concurrent-remove regression test.
| // Phase 2: row is still queued. Ask the CLI whether it already shifted the item | ||
| // (race window between collectBatch() shift and messages-consumed ack). | ||
| const { localId, resolvedId, scheduledAt } = lookup | ||
| const { localId, resolvedId, scheduledAt, message } = lookup |
There was a problem hiding this comment.
[MINOR] Release the Hub blob when cancel resolves as invoked
The new message value lets the true-delete branches clean scheduled attachments, but the later not-found/timeout path marks this row invoked and returns without calling the cleanup helper. Its locally emitted messages-consumed event does not pass through SyncEngine.handleRealtimeEvent, so no other path releases the source blob. After the scratchlist draft is removed, that orphan permanently consumes session quota.
Suggested fix:
this.store.messages.markMessagesInvoked(sessionId, [localId], invokedAt)
await this.releaseCancelledScheduledAttachment(sessionId, message)Add mature attachment tests for both removed: false and timeout acknowledgements, with no remaining scratchlist reference.
There was a problem hiding this comment.
Findings
- [Major] Skip consumed messages when re-homing scheduled attachments — consumption intentionally deletes the Hub source blob once no uninvoked message or scratchlist row references it. The new merge helper nevertheless scans every moved message, including invoked rows, and calls the strict filesystem move for any old-session Hub path. After a normal scheduled attachment is delivered and cleaned up, a later dedup/resume merge therefore tries to rename a missing source file and aborts the merge. Evidence
hub/src/sync/messageAttachmentTransfer.ts:44, related cleanuphub/src/sync/messageService.ts:221.
Suggested fix:for (const [messageId, candidate] of candidates) { if (candidate.invokedAt !== null || candidate.scheduledAt === null) continue if (!targetById.has(messageId)) continue // Re-home only still-pending scheduled deliveries. }
Questions
- None.
Summary
Review mode: follow-up after new commits
The new commits resolve the two prior bot findings. The latest full diff still contains one Major regression: a successfully consumed scheduled attachment can make a later session merge/resume fail.
Testing
- Not run (automation; PR code was not executed). GitHub
testandintegrationchecks passed. - Add a regression that schedules an attachment, marks it invoked, releases the Hub blob, then merges sessions and asserts the merge succeeds without re-homing the invoked row. Retain coverage that an uninvoked scheduled row is re-homed.
HAPI Bot
| } | ||
|
|
||
| const hapiHome = getHapiHomeDir() | ||
| for (const [messageId, candidate] of candidates) { |
There was a problem hiding this comment.
[MAJOR] Skip already-consumed scheduled attachments
releaseConsumedScheduledAttachments deletes the Hub source blob after invocation when no draft/uninvoked row still references it. This loop processes all moved messages, so a later session merge sees the invoked message's stale Hub path and calls moveScratchlistAttachmentFilesForSession(..., { throwOnFailure: true }); the expected missing source then aborts dedup/resume.
Suggested fix:
for (const [messageId, candidate] of candidates) {
if (candidate.invokedAt !== null || candidate.scheduledAt === null) continue
if (!targetById.has(messageId)) continue
// move only pending scheduled deliveries
}Add a consumed-then-merge regression alongside the existing partial-move test.
There was a problem hiding this comment.
Findings
-
[Minor] Use persisted merge state before re-homing scheduled attachments —
sourceMessagesis captured beforemergeSessionMessagesforce-invokes duplicatelocal_idrows. The check athub/src/sync/messageAttachmentTransfer.ts:49can therefore treat a just-consumed collision loser as pending and move its blob into the target session. Because the merge also clears that row'slocal_id, no later consumed-message acknowledgement can release the moved blob; source-session deletion can no longer remove it, so it remains counted against the target attachment quota.Suggested fix:
const persisted = targetById.get(messageId) if (!persisted || persisted.invokedAt !== null || persisted.scheduledAt === null) continue const attachments = getUserMessageAttachments(persisted.content)
Questions
- None.
Summary
Review mode: follow-up after new commits
One minor attachment-lifecycle issue remains at the current head. The prior scheduled-attachment merge failure is addressed by the new commits.
Testing
- Not run (automation; PR code was not executed).
- Add a merge regression where source and target contain the same
localId, the source copy has a scheduled Hub attachment, and verify the force-invoked loser is not re-homed or charged to the target session quota.
HAPI Bot
| // their history, but releaseConsumedScheduledAttachments deliberately | ||
| // removes that source file once no pending row references it. Only | ||
| // re-home attachments that can still be delivered or cancelled. | ||
| if (candidate.invokedAt !== null || candidate.scheduledAt === null) continue |
There was a problem hiding this comment.
[Minor] Use the post-merge row state here.
sourceMessages was read before mergeSessionMessages, which force-invokes source rows whose local_id collides with the target and clears that acknowledgement key. This stale candidate.invokedAt === null then re-homes the collision loser's Hub blob into the target. No later messages-consumed event can release it, and deleting the source session cannot remove a file already moved under the target, leaving permanent quota usage.
Suggested fix:
const persisted = targetById.get(messageId)
if (!persisted || persisted.invokedAt !== null || persisted.scheduledAt === null) continue
const attachments = getUserMessageAttachments(persisted.content)
There was a problem hiding this comment.
Findings
-
[Major] Reject materializations completed for a stale CLI connection generation - reconnect only clears paths already present in the cache. If the previous CLI's upload RPC is still in flight, it can resolve afterward, repopulate the cache, and be emitted to the newly connected CLI even though the path belongs to the old process. This can deliver a scheduled prompt with a missing attachment. Evidence:
hub/src/sync/messageService.ts:984.Suggested fix:
const generation = this.scheduledAttachmentGeneration.get(msg.sessionId) ?? 0 const staged = await this.options.materializeScheduledAttachments(msg.sessionId, attachments) if ((this.scheduledAttachmentGeneration.get(msg.sessionId) ?? 0) !== generation) { await this.options.deleteMaterializedScheduledAttachments?.(msg.sessionId, staged) return null } this.scheduledAttachmentDeliveryCache.set(cacheKey, staged)
-
[Minor] Reconcile consumed scheduled attachments after cleanup interruption - the invocation is committed before cleanup is launched fire-and-forget. A hub exit or filesystem failure after that commit leaves the Hub blob on disk indefinitely;
sumScratchlistAttachmentBytesOnDiskcontinues charging it to the session quota, and no startup/periodic path retries the cleanup. Evidence:hub/src/sync/syncEngine.ts:482.Suggested fix:
for (const session of this.store.sessions.getSessions()) { const consumedLocalIds = this.store.messages.getAllMessages(session.id) .filter((message) => message.scheduledAt !== null && message.invokedAt !== null && message.localId) .map((message) => message.localId!) await this.messageService.releaseConsumedScheduledAttachments(session.id, consumedLocalIds) }
Questions
- None.
Summary
Review mode: follow-up after new commits
The prior collision-loser finding is addressed at the new head. Two attachment-lifecycle issues remain: stale in-flight materialization across reconnects and non-recoverable cleanup interruption.
Testing
- Not run (automation; PR code was not executed).
- Add a test that reconnects while
materializeScheduledAttachmentsis unresolved, then resolves the old generation and verifies its upload is deleted and never emitted. - Add a restart-reconciliation test where an invoked scheduled attachment remains on disk and verify the next hub startup removes it and restores quota.
HAPI Bot
| if (!deliveryAttachments) { | ||
| if (!this.options.materializeScheduledAttachments) return null | ||
| deliveryAttachments = await this.options.materializeScheduledAttachments(msg.sessionId, attachments) | ||
| this.scheduledAttachmentDeliveryCache.set(cacheKey, deliveryAttachments) |
There was a problem hiding this comment.
[MAJOR] Reject results from the previous CLI generation.
clearScheduledAttachmentDeliveryCache() only removes entries that already exist. If a reconnect occurs while this upload RPC is unresolved, the old CLI can finish afterward and this line repopulates the cache with its process-scoped path. The release loop then sees the new socket connected and emits that stale path, so the scheduled prompt can be acknowledged without a usable attachment.
Suggested fix:
const generation = this.scheduledAttachmentGeneration.get(msg.sessionId) ?? 0
const staged = await this.options.materializeScheduledAttachments(msg.sessionId, attachments)
if ((this.scheduledAttachmentGeneration.get(msg.sessionId) ?? 0) !== generation) {
await this.options.deleteMaterializedScheduledAttachments?.(msg.sessionId, staged)
return null
}
this.scheduledAttachmentDeliveryCache.set(cacheKey, staged)Increment the generation in clearScheduledAttachmentDeliveryCache().
|
|
||
| handleRealtimeEvent(event: SyncEvent): void { | ||
| if (event.type === 'messages-consumed') { | ||
| void this.messageService |
There was a problem hiding this comment.
[MINOR] Make consumed-attachment cleanup recoverable.
The messages-consumed handler has already committed invoked_at before this fire-and-forget cleanup starts. If the hub exits or file deletion fails, the blob remains under the session directory and keeps counting toward sumScratchlistAttachmentBytesOnDisk, but nothing retries cleanup after restart.
Suggested fix:
for (const session of this.store.sessions.getSessions()) {
const consumedLocalIds = this.store.messages.getAllMessages(session.id)
.filter((message) => message.scheduledAt !== null && message.invokedAt !== null && message.localId)
.map((message) => message.localId!)
await this.messageService.releaseConsumedScheduledAttachments(session.id, consumedLocalIds)
}Run this reconciliation on startup (and optionally on a low-frequency maintenance tick).
There was a problem hiding this comment.
Findings
-
[Major] Avoid rescanning every stored transcript on the five-second tick —
hub/src/sync/syncEngine.ts:1287invokes reconciliation for every session, andhub/src/sync/messageService.ts:255loads and decodes the complete message history before filtering. Already-cleaned consumed rows are revisited forever, so storage growth turns into continuous O(total messages) SQLite/decompression work even when no cleanup is pending.Suggested fix:
// Run the complete recovery scan once during startup. this.reconcileConsumedScheduledAttachments() // On the regular tick, retry only sessions whose live cleanup failed. for (const sessionId of this.consumedAttachmentCleanupRetries) { void this.messageService.reconcileConsumedScheduledAttachments(sessionId) .then(() => this.consumedAttachmentCleanupRetries.delete(sessionId)) }
Add the session to
consumedAttachmentCleanupRetrieswhen themessages-consumedcleanup fails, and make filesystem deletion failures propagate so they can be queued.
Questions
- None.
Summary
Review mode: follow-up after new commits
The two prior attachment-lifecycle findings are addressed at this head. One new performance regression remains in the periodic recovery path.
Testing
- Not run (automation); GitHub
testandintegrationchecks pass at the reviewed head. - Add a regression test proving repeated inactivity ticks do not call
getAllMessagesafter successful startup reconciliation, while a failed cleanup retries only the affected session.
HAPI Bot
| // but shares its 5s cadence (avoids a second timer). | ||
| this.messageService.releaseMatureScheduledMessages(Date.now(), this.historyActionsInFlight) | ||
| void this.messageService.releaseMatureScheduledMessages(Date.now(), this.historyActionsInFlight) | ||
| this.reconcileConsumedScheduledAttachments() |
There was a problem hiding this comment.
[MAJOR] Do not rescan all stored message history every five seconds.
This call runs on every inactivity tick. It iterates every session, and MessageService.reconcileConsumedScheduledAttachments() calls getAllMessages(), decoding the full transcript before filtering. It also repeats already-successful cleanup forever. Large hubs therefore spend continuous CPU/SQLite/decompression work proportional to all stored history, even when no attachment cleanup is pending.
Suggested fix:
// Startup: one full recovery scan.
this.reconcileConsumedScheduledAttachments()
// Tick: retry only sessions whose live cleanup failed.
for (const sessionId of this.consumedAttachmentCleanupRetries) {
void this.messageService.reconcileConsumedScheduledAttachments(sessionId)
.then(() => this.consumedAttachmentCleanupRetries.delete(sessionId))
}Add a session to the retry set from the messages-consumed cleanup catch, and propagate filesystem deletion failures so only real failures are retried.
There was a problem hiding this comment.
Findings
-
[Minor] A later successful cleanup can cancel an earlier pending retry —
hub/src/sync/syncEngine.ts:495removes the session fromconsumedAttachmentCleanupRetriesafter any local-ID cleanup succeeds. If cleanup A fails and sets the retry marker, then cleanup B succeeds before the next five-second tick, B clears the marker without revisiting A. The blob from A remains on disk and continues counting against the session quota until a hub restart.Suggested fix:
void this.messageService .releaseConsumedScheduledAttachments(event.sessionId, event.localIds) .catch((error) => { this.consumedAttachmentCleanupRetries.add(event.sessionId) console.error('[Scratchlist] failed to release scheduled attachments', error) }) // Only a successful full reconcile should delete the session from the retry set.
Questions
- None.
Summary
Review mode: follow-up after new commits
The previous periodic full-history rescan is removed. One retry-state race remains: a later successful live cleanup can suppress recovery of an earlier failed cleanup in the same session.
Testing
- Not run (automation; PR code was not executed). GitHub
testandintegrationchecks pass at the reviewed head. - Add a regression where one consumed-message cleanup fails, a later cleanup for the same session succeeds, then the inactivity tick still reconciles that session.
HAPI Bot
| void this.messageService | ||
| .releaseConsumedScheduledAttachments(event.sessionId, event.localIds) | ||
| .then(() => { | ||
| this.consumedAttachmentCleanupRetries.delete(event.sessionId) |
There was a problem hiding this comment.
[MINOR] Do not clear a session-level retry after a partial live cleanup succeeds.
This success covers only the current event's localIds. If an earlier cleanup for the same session failed and added the retry marker, a later successful event reaches this line before the next tick and removes the marker without revisiting the failed row. That Hub blob then remains charged to quota until restart.
Suggested fix:
void this.messageService
.releaseConsumedScheduledAttachments(event.sessionId, event.localIds)
.catch((error) => {
this.consumedAttachmentCleanupRetries.add(event.sessionId)
console.error('[Scratchlist] failed to release scheduled attachments', error)
})
// Keep deletion of the retry marker only in the successful full-reconcile path.
There was a problem hiding this comment.
Findings
-
[Minor] Cancelling an already-materialized scheduled attachment leaves the CLI-side upload behind - after the mature scan caches the staged
/cli/...path, the normal cancellation path deletes the queued row and callsreleaseCancelledScheduledAttachment.releaseScheduledAttachmentsremoves that cache entry and the Hub source blob, but never callsdeleteMaterializedScheduledAttachments, so repeated schedule -> mature -> cancel cycles accumulate unused files until broader session cleanup. Evidencehub/src/sync/messageService.ts:222.Suggested fix:
private async releaseCancelledScheduledAttachment( sessionId: string, message: StoredMessageForDelivery, ): Promise<void> { if (message.scheduledAt === null) return const cacheKey = `${sessionId}:${message.id}` const staged = this.scheduledAttachmentDeliveryCache.get(cacheKey) try { await this.releaseScheduledAttachments(sessionId, [message]) } finally { this.scheduledAttachmentDeliveryCache.delete(cacheKey) if (staged) { await this.cleanupMaterializedScheduledAttachments(sessionId, staged) } } }
Questions
- None.
Summary
- Review mode: follow-up after new commits
- The previous keyed-chat navigation cleanup issue is resolved at the current head. One resource-cleanup gap remains in mature scheduled-attachment cancellation.
Testing
- Not run (automation;
bunis unavailable in this runner, and PR code was not executed). GitHubintegrationpassed;testwas pending at review time. - Add a regression that materializes and emits a mature scheduled attachment, cancels it with
removed: true, and asserts both the Hub source and cached CLI upload are deleted.
HAPI Bot
| const deleteScheduledAttachments = this.options.deleteScheduledAttachments | ||
| if (!deleteScheduledAttachments || messages.length === 0) return | ||
| for (const message of messages) { | ||
| this.scheduledAttachmentDeliveryCache.delete(`${sessionId}:${message.id}`) |
There was a problem hiding this comment.
[MINOR] Clean the materialized CLI upload when cancellation wins.
At this point a mature scheduled attachment may already have a cached CLI upload path. The cancellation flow deletes the queued row, then this method drops the cache entry and deletes only the Hub-resident source. Because the cached value is discarded without calling cleanupMaterializedScheduledAttachments, the staged CLI file is orphaned until broader session cleanup.
Suggested fix:
const cacheKey = `${sessionId}:${message.id}`
const staged = this.scheduledAttachmentDeliveryCache.get(cacheKey)
try {
await this.releaseScheduledAttachments(sessionId, [message])
} finally {
this.scheduledAttachmentDeliveryCache.delete(cacheKey)
if (staged) {
await this.cleanupMaterializedScheduledAttachments(sessionId, staged)
}
}Please add a test for cancellation after materialization/emission; the existing race test covers cancellation only while materialization is still pending.
There was a problem hiding this comment.
Findings
-
[Major] Wait for send settlement before deleting the scratchlist row —
props.onSendreturns as soon as the mutation starts (web/src/hooks/mutations/useSendMessage.ts:350), but every path except a scheduled send with Hub attachments immediately deletes the durable draft and, for immediate attachments, its Hub blobs. A network or HTTP failure can therefore remove the scratchlist item before the send succeeds. Evidenceweb/src/components/SessionChat.tsx:880.Suggested fix:
if (!accepted) return false props.onScheduledScratchlistSend?.( accepted.attemptId, props.session.id, entry.id, ) return true
Questions
- None.
Summary
Review mode: follow-up after new commits
The prior staged-upload cancellation cleanup finding is fixed. One send-settlement regression remains.
Testing
- Not run (automation; PR code was not executed). GitHub
integrationandtestchecks passed. - Add immediate and scheduled scratchlist-send failure tests that assert the entry and its attachments remain until a
successsettlement.
HAPI Bot
| // release the scratchlist file right away. Scheduled sends retain the | ||
| // hub path so validation can run against the persisted message; wait | ||
| // for the matching mutation settlement before removing the draft. | ||
| if (scheduledAt != null && hubItems.length > 0) { |
There was a problem hiding this comment.
[MAJOR] Preserve the draft until the send settles.
props.onSend returns { attemptId } immediately after mutation.mutate(...), not after the POST succeeds. This branch deletes the scratchlist row (and immediate-send Hub blobs) on acceptance, so a later network/HTTP failure loses the durable draft before a successful send.
Use the existing settlement tracker for every scratchlist send; deleting the entry on the success settlement will also let the Hub clean attachments safely:
if (!accepted) return false
props.onScheduledScratchlistSend?.(
accepted.attemptId,
props.session.id,
entry.id,
)
return truePlease cover both immediate and scheduled failure settlements, including an attachment entry.
There was a problem hiding this comment.
Findings
-
[Major] Preserve blobs that are still referenced by the source scratchlist when re-homing scheduled messages — this helper always renames the old-session file. OpenCode redirect/clear moves the queued message but leaves the source scratchlist in place; the web also intentionally keeps that draft until send settlement. If the response is lost, cleanup fails, or another draft shares the attachment, the durable source entry remains with a path whose file has been moved. Evidence
hub/src/sync/messageAttachmentTransfer.ts:60; related contexthub/src/sync/syncEngine.ts:2662,web/src/components/SessionChat.tsx:882.Suggested fix:
const sourceDraftPaths = new Set( store.scratchlist.list(oldSessionId) .flatMap((entry) => entry.attachments.map((attachment) => attachment.path)) ) const moved = await moveScratchlistAttachmentFilesForSession( hapiHome, namespace, oldSessionId, newSessionId, attachments, { throwOnFailure: true, preserveSource: attachments.some(({ path }) => sourceDraftPaths.has(path)), }, )
-
[Major] Delete previously materialized CLI uploads when invalidating the reconnect cache — the cache currently forgets paths that were already copied into the CLI upload directory. Every socket reconnect before acknowledgement then materializes new uniquely named copies, while the CLI only removes the whole directory on session death; repeated network churn can therefore grow disk usage by the scheduled attachment set on every reconnect. Evidence
hub/src/sync/messageService.ts:196; related contexthub/src/sync/syncEngine.ts:610,cli/src/modules/common/handlers/uploads.ts:124.Suggested fix:
for (const [cacheKey, attachments] of this.scheduledAttachmentDeliveryCache) { if (!cacheKey.startsWith(`${sessionId}:`)) continue this.scheduledAttachmentDeliveryCache.delete(cacheKey) void this.cleanupMaterializedScheduledAttachments(sessionId, attachments) }
Run this cleanup after the reconnecting socket has registered its RPC handlers so the delete RPC has a live target.
Questions
- None.
Summary
Review mode: follow-up after new commits
Two Major scheduled-attachment lifecycle regressions remain: re-home can break a retained source draft, and reconnect invalidation can leak staged CLI files.
Testing
- Not run (automation; PR code was not executed). GitHub
integrationandtestchecks were pending at review time. - Add coverage for an OpenCode redirect/clear where the same Hub blob remains referenced by a source scratchlist entry.
- Add a reconnect-before-ack test asserting the prior materialized upload is deleted before a replacement is staged.
HAPI Bot
| continue | ||
| } | ||
|
|
||
| const moved = await moveScratchlistAttachmentFilesForSession( |
There was a problem hiding this comment.
[MAJOR] Preserve source-draft references when re-homing.
This always renames the old-session blob. OpenCode redirect/clear moves the queued row but does not transfer the source scratchlist, and the web intentionally retains the draft until send settlement. If that cleanup does not complete—or another entry shares the blob—the source draft keeps its old path after the file has moved.
Detect source scratchlist references and copy instead of rename when one remains:
const sourceDraftPaths = new Set(
store.scratchlist.list(oldSessionId)
.flatMap((entry) => entry.attachments.map((attachment) => attachment.path))
)
const preserveSource = attachments.some(({ path }) => sourceDraftPaths.has(path))Pass preserveSource into the storage transfer helper and cover the redirected-send/clear case with a retained source entry.
| sessionId, | ||
| (this.scheduledAttachmentDeliveryGenerations.get(sessionId) ?? 0) + 1, | ||
| ) | ||
| for (const messageId of this.scheduledAttachmentDeliveryCache.keys()) { |
There was a problem hiding this comment.
[MAJOR] Clean the staged files before dropping reconnect cache entries.
These cached paths already exist in the CLI upload directory. Forgetting them on each socket reconnect causes the next mature scan to upload another uniquely named copy; the CLI only clears the directory on session death, so reconnect churn can repeatedly amplify disk usage.
Retain the cached attachment list and call cleanupMaterializedScheduledAttachments for every removed entry:
for (const [cacheKey, attachments] of this.scheduledAttachmentDeliveryCache) {
if (!cacheKey.startsWith(`${sessionId}:`)) continue
this.scheduledAttachmentDeliveryCache.delete(cacheKey)
void this.cleanupMaterializedScheduledAttachments(sessionId, attachments)
}Invoke the cleanup after the new socket registers its RPC handlers so the delete RPC has a live target.
There was a problem hiding this comment.
Findings
-
[Major] Make scratchlist send cleanup retry-aware and revision-safe — the tracker removes the attempt on both success and error, but attachment retries reuse the same localId; a failed send followed by a successful failed-bubble retry therefore leaves the draft behind. On the success path, deleting only by entry id can also erase text or attachments edited on another device while the send was settling. Evidence web/src/components/SessionChat.tsx:515.
Suggested fix:
type PendingScratchlistSend = { sessionId: string entryId: string expectedUpdatedAt: number } if (settlement.status === 'error') return await api.deleteScratchlistEntryIfUnchanged( pending.sessionId, pending.entryId, pending.expectedUpdatedAt, ) pendingSendsRef.current.delete(attemptId)
Back this with an atomic Hub delete:
DELETE FROM session_scratchlist WHERE session_id = ? AND entry_id = ? AND updated_at = ?
-
[Minor] Render non-image attachments in attachment-only rows — thumbnails intentionally filter to image MIME types, while the row now renders only entry.text. A PDF/file-only draft is therefore a blank row with no filename and no per-attachment remove affordance. Evidence web/src/components/AssistantChat/ScratchlistPanel.tsx:1051.
Suggested fix:
{props.attachments .filter((attachment) => !isImageMimeType(attachment.mimeType)) .map((attachment) => ( <button key={attachment.id} type="button" data-scratchlist-action="remove-attachment" onClick={() => props.onRemove(attachment.id)} > 📎 {attachment.filename} </button> ))}
Questions
- None.
Summary
Review mode: follow-up after new commits
One Major cleanup race can retain a successfully retried draft or delete a newer edit; one Minor rendering regression hides non-image attachment-only drafts.
Testing
- Not run locally (automation; PR code was not executed).
- GitHub test and integration checks passed.
- Add coverage for failure → failed-bubble retry → success cleanup, edit-during-send conditional cleanup, and non-image attachment-only rendering/removal.
HAPI Bot
| if (settlement?.attemptId !== attemptId) return | ||
| const pending = pendingSendsRef.current.get(attemptId) | ||
| if (!pending) return | ||
| pendingSendsRef.current.delete(attemptId) |
There was a problem hiding this comment.
[MAJOR] Make post-send cleanup retry-aware and revision-safe.
useSendMessage keeps attachment failures as retryable bubbles, and retryMessage reuses the same localId/attempt id. Deleting this mapping on an error settlement means a later successful retry has no source draft to clean up. The success branch also deletes by entry id only, so an edit made while the POST is pending—including from another device—can be deleted even though that edited revision was never sent.
Keep the association across errors, record the sent revision, and remove it only after an atomic conditional delete succeeds:
type PendingScratchlistSend = {
sessionId: string
entryId: string
expectedUpdatedAt: number
}
if (settlement.status === 'error') return
await api.deleteScratchlistEntryIfUnchanged(
pending.sessionId,
pending.entryId,
pending.expectedUpdatedAt,
)
pendingSendsRef.current.delete(attemptId)Hub-side:
DELETE FROM session_scratchlist
WHERE session_id = ? AND entry_id = ? AND updated_at = ?Add tests for failure → retry success and editing the draft before the original send settles.
| disabled={disabled} | ||
| className="min-h-6 min-w-0 w-full cursor-text rounded text-left text-sm leading-6 text-[var(--app-fg)] outline-none line-clamp-4 whitespace-pre-wrap break-words focus-visible:ring-1 focus-visible:ring-[var(--app-link)] disabled:cursor-not-allowed disabled:opacity-60" | ||
| > | ||
| {entry.text} |
There was a problem hiding this comment.
[MINOR] Render non-image attachments instead of an empty row.
The thumbnail component filters to images, and this button now renders only entry.text. For an attachment-only PDF or other file, the row has no visible filename and no per-attachment remove control.
Render a generic file chip alongside image thumbnails:
{props.attachments
.filter((attachment) => !isImageMimeType(attachment.mimeType))
.map((attachment) => (
<button
key={attachment.id}
type="button"
data-scratchlist-action="remove-attachment"
onClick={() => props.onRemove(attachment.id)}
>
📎 {attachment.filename}
</button>
))}
There was a problem hiding this comment.
Findings
-
[Major] Bound attachment previews by bytes, not only entry count — the new cache permits 64 previews regardless of payload size. With the allowed 5 MiB attachments, it can retain roughly 320 MiB of image payload and over 400 MiB as base64 data URLs in a long-lived PWA tab, risking severe memory pressure or tab termination. Evidence:
web/src/lib/scratchlistAttachmentPreview.ts:15.Suggested fix:
type CachedPreview = { signature: string src: string kind: 'data' | 'object-url' bytes: number } const MAX_CACHED_PREVIEW_BYTES = 20 * 1024 * 1024 let cachedPreviewBytes = 0 // In putPreview/release/clear, add/subtract attachment.size and evict // oldest entries while cachedPreviewBytes exceeds the byte budget.
-
[Minor] Reconcile the optimistic timestamp before allowing send/schedule — inline updates put client
Date.now()into the cache, but the Hub returns the canonical monotonicupdatedAt. Sending before the mutation is reconciled registers the client timestamp asexpectedUpdatedAt, so the post-send conditional delete can returnrevision-mismatchand leave the successfully sent draft in the list. Evidence:web/src/components/SessionChat.tsx:916; related optimistic update:web/src/lib/use-hub-scratchlist.ts:415.Suggested fix:
onSuccess: ({ entry }) => { queryClient.setQueryData<ScratchlistResponse>(queryKey, (prev) => prev && ({ entries: prev.entries.map((item) => item.entryId === entry.entryId ? entry : item ), })) } // Expose updateMutation.isPending and disable Send/Schedule while true.
-
[Minor] Retry cleanup requests that fail after a successful send — the catch keeps the attempt mapping, but the successful settlement object does not change again, so nothing calls
settlePendingSenda second time. A transient network error therefore leaves the draft and its Hub attachments indefinitely. Evidence:web/src/components/SessionChat.tsx:539.Suggested fix:
const cleanupRetriesRef = useRef(new Map<string, number>()) // In the cleanup catch: const retries = cleanupRetriesRef.current.get(attemptId) ?? 0 if (retries < 3) { cleanupRetriesRef.current.set(attemptId, retries + 1) window.setTimeout( () => settlePendingSend(attemptId), 1_000 * (2 ** retries), ) } // Clear the retry counter when cleanup settles.
-
[Minor] Render non-image attachment-only entries — the thumbnail component filters to image MIME types, while the row otherwise renders only
entry.text. Since PDFs and text files are accepted, an attachment-only entry for those types is visually blank and provides no filename or removal affordance. Evidence:web/src/components/AssistantChat/ScratchlistPanel.tsx:1051.Suggested fix:
{entry.attachments ?.filter((attachment) => !isImageMimeType(attachment.mimeType)) .map((attachment) => ( <button key={attachment.id} type="button" onClick={() => removeAttachment(entry, attachment.id)} > {attachment.filename} </button> ))}
Questions
- None.
Summary
Review mode: follow-up after new commits
Four issues found in the latest full diff. The non-image attachment issue remains unresolved from the prior HAPI Bot review; the other findings concern the new preview cache and send-cleanup lifecycle.
Testing
- Not run locally; PR code was not executed.
- GitHub
testandintegrationchecks passed. - Missing focused coverage for byte-budget preview eviction, edit-then-immediate-send cleanup, transient cleanup retry, and non-image attachment-only rendering.
HAPI Bot
| } | ||
|
|
||
| const previews = new Map<string, CachedPreview>() | ||
| const MAX_CACHED_PREVIEWS = 64 |
There was a problem hiding this comment.
[MAJOR] Cache size needs a byte budget. This limits only the number of entries. At the allowed 5 MiB attachment size, 64 previews can retain about 320 MiB of payload, and data URLs expand that beyond 400 MiB in JS strings. In a long-lived PWA tab this can cause severe memory pressure or termination.
Suggested fix:
type CachedPreview = {
signature: string
src: string
kind: 'data' | 'object-url'
bytes: number
}
const MAX_CACHED_PREVIEW_BYTES = 20 * 1024 * 1024
let cachedPreviewBytes = 0
// Add/subtract attachment.size in putPreview/release/clear,
// evicting oldest entries while over MAX_CACHED_PREVIEW_BYTES.| accepted.attemptId, | ||
| props.session.id, | ||
| entry.id, | ||
| entry.updatedAt ?? entry.createdAt, |
There was a problem hiding this comment.
[MINOR] This revision may still be the optimistic client timestamp. use-hub-scratchlist.ts:415 writes Date.now() during an update but never replaces it with the Hub-returned canonical entry. If the user edits and immediately sends, this conditional delete can use a revision the Hub never stored, return revision-mismatch, and leave the sent draft behind.
Suggested fix:
onSuccess: ({ entry }) => {
queryClient.setQueryData<ScratchlistResponse>(queryKey, (prev) => prev && ({
entries: prev.entries.map((item) =>
item.entryId === entry.entryId ? entry : item
),
}))
}
// Also disable Send/Schedule while updateMutation.isPending.| // A false result is an intentional no-op when another client has | ||
| // edited or deleted the draft; in either case this send is settled. | ||
| pendingSendsRef.current.delete(attemptId) | ||
| }).catch(() => { |
There was a problem hiding this comment.
[MINOR] A failed cleanup is never retried. Keeping the mapping is not enough: after a successful send, the settlement object will not change again, so settlePendingSend is never invoked a second time. One transient request failure leaves the draft and its Hub attachments indefinitely.
Suggested fix:
const retries = cleanupRetriesRef.current.get(attemptId) ?? 0
if (retries < 3) {
cleanupRetriesRef.current.set(attemptId, retries + 1)
window.setTimeout(
() => settlePendingSend(attemptId),
1_000 * (2 ** retries),
)
}
// Clear the counter after cleanup settles.| disabled={disabled} | ||
| className="min-h-6 min-w-0 w-full cursor-text rounded text-left text-sm leading-6 text-[var(--app-fg)] outline-none line-clamp-4 whitespace-pre-wrap break-words focus-visible:ring-1 focus-visible:ring-[var(--app-link)] disabled:cursor-not-allowed disabled:opacity-60" | ||
| > | ||
| {entry.text} |
There was a problem hiding this comment.
[MINOR] Non-image attachment-only entries render blank. ScratchlistAttachmentThumbnails filters to image MIME types, and this is the only remaining row content. PDFs/text files are accepted, so those entries expose neither a filename nor a removal affordance.
Suggested fix:
{entry.attachments
?.filter((attachment) => !isImageMimeType(attachment.mimeType))
.map((attachment) => (
<button
key={attachment.id}
type="button"
onClick={() => removeAttachment(entry, attachment.id)}
>
{attachment.filename}
</button>
))}
There was a problem hiding this comment.
Findings
- [Major] Socket reconnect invalidates attachment paths still referenced by the live CLI queue —
hub/src/sync/syncEngine.ts:618 - [Minor] Cleanup retry loses an earlier successful settlement when another send settles —
web/src/components/SessionChat.tsx:536 - [Minor] Chat-route unmount drops pending scratchlist cleanup tracking —
web/src/router.tsx:656
Summary
Review mode: follow-up after new commits. Reviewed the full latest diff; the prior review and commit comparison were used only as follow-up context. Three correctness issues remain: one can make a scheduled attachment path disappear before the agent consumes it, and two can leave successfully sent scratchlist drafts visible and resendable.
Testing
Not run (automation); PR code was not executed. Static inspection only; git diff --check passed. Existing tests do not cover a same-process socket reconnect with a retained CLI queue, interleaved cleanup settlements, or route navigation before settlement.
HAPI Bot
| // Materialized attachment paths belong to the previous CLI process. | ||
| // Socket reconnection is the reliable generation boundary for every | ||
| // agent, including ordinary sessions that do not emit session-ready. | ||
| this.messageService.clearScheduledAttachmentDeliveryCache(sessionId) |
There was a problem hiding this comment.
[MAJOR] Preserve materialized paths across transport reconnects
This runs for every Socket.IO connection, including a reconnect by the same ApiSessionClient. That client retains both IncomingMessageFilter (cli/src/api/apiSession.ts:119-164) and its already-formatted agent queue across reconnects. If a mature scheduled message was queued with path P1, this call schedules P1 for deletion; the Hub rematerializes P2 and re-emits the same message ID, but the retained filter rejects that emit as a duplicate (cli/src/api/apiSession.ts:736-750). The queue therefore still references deleted P1 when the agent consumes it. The reconnect test at hub/src/sync/messageService.test.ts:1227 checks P2 emission, but does not model the retained CLI filter/queue.
Suggested fix:
// Persist this for the ApiSessionClient/process lifetime, not the socket lifetime.
const clientInstanceId = randomUUID()
// Invalidate only when a different CLI process takes ownership of the session.
if (previousClientInstanceId !== payload.clientInstanceId) {
this.messageService.clearScheduledAttachmentDeliveryCache(payload.sid)
}Carry the stable instance ID in the CLI auth or session-alive payload; an ordinary reconnect with the same ID must keep the cached paths alive.
| }, []) | ||
| const settlePendingSend = useCallback((attemptId: string) => { | ||
| const settlement = sendSettlementRef.current | ||
| if (settlement?.attemptId !== attemptId) return |
There was a problem hiding this comment.
[MINOR] Retain success state for cleanup retries
A retry validates against only the latest global sendSettlement. After cleanup for attempt A fails, attempt B can settle during A's one-second backoff. A's timer then reaches this line, sees B, returns permanently, and never schedules another retry; the successfully sent draft A remains visible and can be sent again. The new retry test advances the timer without introducing another settlement, so it misses this case.
Suggested fix:
const successfulAttemptsRef = useRef(new Set<string>())
useEffect(() => {
if (sendSettlement?.status !== 'success') return
successfulAttemptsRef.current.add(sendSettlement.attemptId)
settlePendingSend(sendSettlement.attemptId)
}, [sendSettlement, settlePendingSend])
// In settlePendingSend:
if (!successfulAttemptsRef.current.has(attemptId)) returnRemove the ID from the set when cleanup succeeds or the retry budget is exhausted, instead of consulting the latest unrelated settlement.
| // 'no-session' and 'pending' don't need toast - either invalid state or expected behavior | ||
| } | ||
| }) | ||
| const trackScratchlistSend = usePendingScratchlistSendCleanup(api, sendSettlement) |
There was a problem hiding this comment.
[MINOR] Keep cleanup tracking alive outside the chat-only route
The tracker is owned by SessionPage, but SessionDetailRoute replaces that component with <Outlet /> when the user opens files or terminal (web/src/router.tsx:845-900). If the user navigates after the scratchlist send is accepted but before useSendMessage publishes its settlement, this hook unmounts and loses the attempt mapping; the request can still succeed, but the source draft is never deleted. The component test covers a keyed SessionChatInner remount, not actual route navigation.
Suggested fix:
function SessionsPage() {
const cleanup = useScratchlistSendCleanupRegistry(api)
return (
<ScratchlistSendCleanupContext.Provider value={cleanup}>
{/* existing sessions layout and <Outlet /> */}
</ScratchlistSendCleanupContext.Provider>
)
}
// SessionPage registers accepted attempts and mutation settlements with
// this persistent registry instead of owning the maps/timers itself.The registry/provider must outlive SessionPage so route navigation cannot discard an accepted send before settlement.
There was a problem hiding this comment.
Findings
- [Major] Ambiguous cancel acknowledgement deletes an attachment path the agent may already be using —
hub/src/sync/messageService.ts:715
Suggested fix:// The row is invoked: release only the durable Hub source. await this.releaseScheduledAttachments(sessionId, [message]) // Keep the staged CLI upload; the agent may already be processing it.
Summary
Review mode: follow-up after new commits. Reviewed the full latest diff; the prior review and commit comparison were used only as follow-up context. The three prior findings are addressed in the new commits, but one Major cancellation race remains: a scheduled attachment can be removed from the CLI host after the CLI has already dequeued the message.
Testing
Not run (automation); PR code was not executed. Static inspection only; git diff --check passed. Existing coverage checks confirmed removal and cancellation during materialization, but not not-found/timeout after materialization; add that regression case and assert the staged upload is retained.
HAPI Bot
| // The messages-consumed event below is local to this Hub process and | ||
| // does not pass through SyncEngine's normal consumed-message cleanup. | ||
| // Release any now-unreferenced Hub attachment explicitly. | ||
| await this.releaseCancelledScheduledAttachment(sessionId, message) |
There was a problem hiding this comment.
[MAJOR] Keep the staged attachment alive when cancellation loses the dequeue race
This branch defines not-found/timeout as meaning the CLI may already have shift()-ed the message, then marks the row invoked. releaseCancelledScheduledAttachment() also calls deleteMaterializedScheduledAttachments, so it can delete the staged CLI path before the agent opens it. The same unsafe cleanup occurs in the recheck.status === 'invoked' branches above. A cancellation attempt can therefore turn an already-running scheduled prompt into a broken attachment. The current post-materialization test only uses { removed: true }, so it does not cover this race.
Suggested fix:
// Invoked / ambiguous-ack paths: release only the durable Hub copy.
await this.releaseScheduledAttachments(sessionId, [message])
// Do not delete the staged CLI upload here; the CLI may already be processing it.Reserve releaseCancelledScheduledAttachment() (including staged-upload deletion) for cases where the DB row was actually deleted and the CLI confirmed removal, and add not-found plus timeout tests asserting deleteMaterializedScheduledAttachments is not called.
There was a problem hiding this comment.
Findings
-
[Major] Serialize attachment ownership changes with scratchlist cleanup — message rows move to the destination session before filesystem re-homing begins, while draft/attachment deletion uses a separate per-session lock and only checks references under the old session id. A concurrent delete can therefore unlink the source blob during the await and leave the moved scheduled row pointing at a missing old path. Evidence:
hub/src/sync/sessionCache.ts:1123; the same ordering appears in the OpenCode transfer path athub/src/sync/syncEngine.ts:2712.Suggested fix:
await attachmentLocks.withSessions(namespace, [oldSessionId, newSessionId], async () => { const sourceMessages = store.messages.getAllMessages(oldSessionId) const movedMessages = store.messages.mergeSessionMessages(oldSessionId, newSessionId) await rehomeMessageAttachments( store, namespace, oldSessionId, newSessionId, sourceMessages, ) // Keep scratchlist transfer and attachment cleanup in this lock scope. })
Summary
Review mode: follow-up after new commits. Reviewed the full latest diff; the previous HAPI Bot review and commit comparison were used as follow-up context. The prior ambiguous-cancellation finding is addressed, but one Major attachment ownership race remains across session merge/OpenCode transfer.
Testing
Not run (automation); PR content was not executed. Static inspection only. Existing tests cover sequential re-home failures and shared-draft preservation, but not concurrent draft/attachment deletion while ownership is moving.
HAPI Bot
|
|
||
| const sourceMessages = this.store.messages.getAllMessages(oldSessionId) | ||
| const movedMessages = this.store.messages.mergeSessionMessages(oldSessionId, newSessionId) | ||
| await rehomeMessageAttachments( |
There was a problem hiding this comment.
[MAJOR] Serialize the message move with attachment lifecycle cleanup
mergeSessionMessages() has already moved the scheduled row off oldSessionId when this awaited filesystem re-home begins. Scratchlist update/delete routes use withScratchlistAttachmentLock and decide whether a blob is orphaned by scanning messages under that same session id. Because this merge path does not take that lock, a concurrent delete on the old session can now see no message reference and remove the source file while rehomeMessageAttachments() is copying it. The merge then fails after the database move, leaving the queued row on the destination with a missing old-session path. The OpenCode move/abort paths repeat the same owner-first ordering.
Suggested fix:
await attachmentLocks.withSessions(namespace, [oldSessionId, newSessionId], async () => {
const sourceMessages = store.messages.getAllMessages(oldSessionId)
const movedMessages = store.messages.mergeSessionMessages(oldSessionId, newSessionId)
await rehomeMessageAttachments(
store,
namespace,
oldSessionId,
newSessionId,
sourceMessages,
)
// Transfer scratchlist rows/files before releasing either session lock.
})Use deterministic lock ordering for the two session ids, apply the same helper to OpenCode forward/abort moves, and add a regression test that pauses the file copy while deleting the source draft.
There was a problem hiding this comment.
Findings
-
[Major] Lock the redirect destination before inserting and re-homing a scheduled attachment — the attachment path is owned by the source session, but
addMessageForCurrentSession()can insert the row directly into a replacement/superseding session. This branch holds only the source-session lock, so a concurrent merge of the destination can move the row again whilerehomeScheduledMessageAttachments()awaits filesystem work. The rewrite then targets the now-empty first destination, leaving the moved queued row with an old-session path whose file was already re-keyed. Evidence:hub/src/sync/messageService.ts:887.Suggested fix:
const targetSessionId = this.store.resolveMessageTargetSessionId(sessionId) const insertedWithMessage = await this.withScheduledAttachmentLocks( [sessionId, targetSessionId], insertMessage, )
Summary
Review mode: follow-up after new commits. Reviewed the full latest diff and used the prior HAPI Bot review plus the inter-commit comparison as context. The previous merge/OpenCode attachment-lock finding is addressed, but the scheduled-send redirect path still has the same ownership race.
Testing
Not run (automation); static inspection only. Existing tests cover sequential re-home behavior and merge/OpenCode pair locking, but not a redirected scheduled attachment send racing a destination-session merge.
HAPI Bot
| } | ||
| return { inserted, msg } | ||
| } | ||
| const insertedWithMessage = payload.scheduledAt != null && attachments.length > 0 |
There was a problem hiding this comment.
[MAJOR] Lock both owners for redirected scheduled attachments
This mutation is protected only by withScheduledAttachmentLock(sessionId), even though addMessageForCurrentSession() may insert directly into actualSessionId. While the awaited re-home is copying/renaming the Hub file, a merge that locks and moves actualSessionId can transfer the queued row again. rehomeMessageAttachments() then rewrites the now-empty first destination, so the final row retains the old Hub path after the file has moved and cannot be materialized when due.
Resolve the redirect target before insertion and book the source + target locks together (using the same deterministic multi-session helper added for merge/OpenCode paths):
const targetSessionId = this.store.resolveMessageTargetSessionId(sessionId)
const insertedWithMessage = await this.withScheduledAttachmentLocks(
[sessionId, targetSessionId],
insertMessage,
)Add a regression test that pauses file re-home, merges the redirect destination, then verifies the final queued row and file path share the final session owner.
There was a problem hiding this comment.
Findings
-
[Major] Scratchlist attachment actions do not survive inactive-session resume — the new action passes source-session attachment paths into the generic send flow. After resume,
onSessionResolveddefers every attachment send, but this path transferred no composer attachment drafts, so the mutation never starts; the click only resumes/navigates and leaves the entry unsent. Evidenceweb/src/components/SessionChat.tsx:992, related contextweb/src/router.tsx:631.
Suggested fix:// Safe minimum until the action can refetch/re-stage against the resolved session. onSend={props.session.active ? handleSendScratchlistEntry : undefined} onSchedule={props.session.active ? handleScheduleScratchlistEntry : undefined}
-
[Major] Successful resumed sends clean up the deleted source session — text-only actions can resume to a different session and send successfully, but the cleanup tracker records
props.session.id. The Hub transfers the scratchlist row and deletes that source during merge, so cleanup retries the obsolete ID and the sent draft remains available for accidental duplicate sends. Evidenceweb/src/components/SessionChat.tsx:1012, related contexthub/src/sync/sessionCache.ts:1095.
Suggested fix:export type SendMessageAcceptance = { attemptId: string sessionId: string } // useSendMessage return { attemptId: localId, sessionId: targetSessionId } // SessionChat props.onScratchlistSendAccepted( accepted.attemptId, accepted.sessionId, entry.id, entry.updatedAt ?? entry.createdAt, )
Summary
- Review mode: follow-up after new commits. Two Major inactive-session regressions remain in the new scratchlist Send now / Schedule send flow. The prior scheduled-attachment locking finding is addressed at the current head.
Testing
- Not run (automation); static inspection only. Add coverage where an inactive source resumes to a different target: attachment scheduling must create the target scheduled row, and successful text-only send must delete the transferred target scratchlist entry.
HAPI Bot
| sendAttachments = [...normalItems, ...staged] | ||
| } | ||
|
|
||
| const accepted = await props.onSend( |
There was a problem hiding this comment.
[MAJOR] Scratchlist attachment actions stop at auto-resume
This calls the generic send flow with attachment paths owned by the inactive source session. On a resume to another ID, web/src/router.tsx:631-648 transfers an empty composer attachment list and returns deferUntilDraftHydrated merely because context.attachments is non-empty. These scratchlist attachments are not composer drafts, so nothing hydrates/retries the action: Send now / Schedule send only navigates and no mutation starts.
Suggested fix:
// Safe minimum until this action resolves the target and refetches/re-stages there.
onSend={props.session.active ? handleSendScratchlistEntry : undefined}
onSchedule={props.session.active ? handleScheduleScratchlistEntry : undefined}| // failure therefore leaves the draft available for retry. | ||
| props.onScratchlistSendAccepted( | ||
| accepted.attemptId, | ||
| props.session.id, |
There was a problem hiding this comment.
[MAJOR] Track cleanup against the resolved session
For text-only entries, props.onSend can resume the inactive source to a different target and successfully start the mutation. mergeSessions transfers the scratchlist row and deletes the source, but this records the old props.session.id; settlement cleanup therefore retries a deleted session and leaves the already-sent draft on the target, enabling an accidental duplicate send.
Suggested fix:
export type SendMessageAcceptance = {
attemptId: string
sessionId: string
}
// useSendMessage
return { attemptId: localId, sessionId: targetSessionId }
// here
props.onScratchlistSendAccepted(
accepted.attemptId,
accepted.sessionId,
entry.id,
entry.updatedAt ?? entry.createdAt,
)
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits. No reportable issues found in the latest full diff. The two prior Major findings are addressed at this head: inactive-session scratchlist actions are gated, and successful-send cleanup tracks the resolved session. Residual risk remains in browser drag/long-press behavior and Hub↔CLI scheduled-attachment timing under real network/process churn.
Testing
- Not run (automation); static inspection only. PR code was not executed per the review security constraints.
HAPI Bot
…r-sync # Conflicts: # hub/src/store/migration-v24.test.ts
There was a problem hiding this comment.
Findings
-
[Blocker] Allocate a new schema version for scratchlist ordering - current
mainalready ships database v24 forfcm_devices.push_key, but this diff keepsSCHEMA_VERSIONat 24 and addspositioninside the v23-to-v24 step. A hub upgrading from current main skips this migration, leavingsession_scratchlist.positionabsent; the new list/create paths then fail at runtime. Evidencehub/src/store/index.ts:846.
Suggested fix:const SCHEMA_VERSION = 25 // migration map 24: () => this.migrateFromV24ToV25() private migrateFromV24ToV25(): void { // add/backfill session_scratchlist.position and create its index }
-
[Major] Do not retain Hub blobs for immediate staged copies that only share an attachment id - immediate scratchlist sends preserve the attachment id while replacing the Hub path with a transient CLI upload path. The added ID fallback therefore blocks draft cleanup even though no message references the Hub path; normal consume cleanup only releases scheduled Hub attachments, so these files remain orphaned and continue counting against the per-session quota. Evidence
hub/src/sync/syncEngine.ts:1021.
Suggested fix:if (attachment.path !== undefined) { return !this.store.messages.hasUninvokedAttachmentReference(sessionId, attachment.path) } return !this.store.messages.getAllMessages(sessionId).some((message) => message.invokedAt === null && messageReferencesScratchlistAttachment(message.content, { id: attachment.id }) )
Questions
- None.
Summary
Review mode: follow-up after new commits. Two reportable issues remain at the fresh head: deployed-v24 databases do not receive the ordering column, and immediate attachment sends can strand Hub blobs until manual storage cleanup.
Testing
- Not run (automation); static inspection only. PR code was not executed per the review security constraints.
HAPI Bot
| * Existing rows retain the previous newest-first order while receiving a | ||
| * dense per-session position that subsequent reorder mutations can update. | ||
| */ | ||
| private migrateFromV23ToV24(): void { |
There was a problem hiding this comment.
[BLOCKER] Existing v24 databases never run this migration.
The live base already sets PRAGMA user_version = 24 for fcm_devices.push_key. This head keeps SCHEMA_VERSION at 24 and folds scratchlist position into v23-to-v24. Any hub upgraded from current main therefore skips this function; session_scratchlist still lacks position, and the new SELECT/INSERT paths fail with no such column.
Suggested fix:
const SCHEMA_VERSION = 25
// migration map
24: () => this.migrateFromV24ToV25()
private migrateFromV24ToV25(): void {
// add/backfill session_scratchlist.position and create its index
}Add a regression fixture shaped exactly like current-main v24: push_key present and position absent.
| return false | ||
| } | ||
| return removed | ||
| return !this.store.messages.getAllMessages(sessionId).some((message) => ( |
There was a problem hiding this comment.
[MAJOR] The ID fallback retains Hub blobs used only by immediate staged copies.
Immediate scratchlist sends keep the same attachment id when copying the file to a transient CLI path (web/src/lib/scratchlistAttachmentFlow.ts:222). Draft cleanup passes the original Hub attachment here: the exact-path check correctly finds no reference, but this second ID-only scan sees the uninvoked immediate message and suppresses deletion. Normal messages-consumed cleanup only releases scheduled Hub attachments, so the source blob becomes a permanent orphan and keeps consuming the session quota.
Suggested fix:
if (attachment.path !== undefined) {
return !this.store.messages.hasUninvokedAttachmentReference(sessionId, attachment.path)
}
return !this.store.messages.getAllMessages(sessionId).some((message) =>
message.invokedAt === null
&& messageReferencesScratchlistAttachment(message.content, { id: attachment.id })
)Add a test where an immediate staged attachment preserves the id but changes the path, then delete the draft before the CLI consume acknowledgement and assert that the Hub file is removed.
There was a problem hiding this comment.
Findings
-
[Major] Bound and deduplicate scheduled attachment references before materialization — scheduled sends currently validate only that every item resolves to a Hub path. The request array remains unbounded and may repeat the same path; maturity then performs one Hub-to-CLI upload per element. A client can therefore schedule the same large blob thousands of times and trigger unbounded RPC traffic and CLI disk growth, bypassing the scratchlist entry limits. Evidence
hub/src/sync/syncEngine.ts:1269, materialization loophub/src/sync/syncEngine.ts:1304.
Suggested fix:const uniquePaths = new Set(checked.attachments.map((attachment) => attachment.path)) if (uniquePaths.size !== checked.attachments.length) { throw new ScheduledAttachmentValidationError('Duplicate scheduled attachment') } const validation = validateScratchlistAttachmentsForWrite( checked.attachments, loadScratchlistAttachmentLimitsFromEnv(), 0, ) if (!validation.ok) { throw new ScheduledAttachmentValidationError(validation.error) }
-
[Minor] Return a client error for stale or invalid scheduled attachments — the new validation callback throws a generic error, while the messages route awaits
engine.sendMessagewithout mapping it. A syntactically valid Hub path that is missing or belongs to another session therefore becomes HTTP 500 instead of a 4xx validation response. Evidencehub/src/sync/messageService.ts:903, route contexthub/src/web/routes/messages.ts:128.
Suggested fix:try { await engine.sendMessage(sessionId, payload) } catch (error) { if (error instanceof ScheduledAttachmentValidationError) { return c.json({ error: error.message, code: 'scheduled_attachment_invalid' }, 400) } throw error }
Questions
- None.
Summary
Review mode: follow-up after new commits. The prior schema-version and immediate Hub-blob cleanup findings are resolved. Two fresh issues remain in scheduled attachment validation: request amplification through duplicate/unbounded references, and invalid client input surfacing as HTTP 500.
Testing
- Not run (automation); static inspection only. PR code was not executed per the review security constraints.
HAPI Bot
| ): Promise<void> { | ||
| const session = this.store.sessions.getSession(sessionId) | ||
| if (!session) throw new Error('Session not found') | ||
| const checked = await this.resolveScratchlistAttachmentsForSession( |
There was a problem hiding this comment.
[MAJOR] Bound and deduplicate scheduled attachment references before materialization.
This only proves that each element resolves. The request array has no scheduled-attachment count or uniqueness bound, and materializeScheduledAttachments later uploads once per element. Repeating one valid 10 MB Hub path thousands of times therefore causes thousands of RPC uploads and CLI files, bypassing the scratchlist write limits.
Suggested fix:
const uniquePaths = new Set(checked.attachments.map((attachment) => attachment.path))
if (uniquePaths.size !== checked.attachments.length) {
throw new ScheduledAttachmentValidationError('Duplicate scheduled attachment')
}
const validation = validateScratchlistAttachmentsForWrite(
checked.attachments,
loadScratchlistAttachmentLimitsFromEnv(),
0,
)
if (!validation.ok) {
throw new ScheduledAttachmentValidationError(validation.error)
}Add a service test with repeated copies of one valid Hub attachment and assert rejection before any materialization RPC.
| ? this.store.getMessageForCurrentSession(sessionId, payload.localId) | ||
| : null | ||
| if (!duplicate && payload.scheduledAt != null && attachments.length > 0) { | ||
| await this.options.validateScheduledAttachments?.(sessionId, attachments) |
There was a problem hiding this comment.
[MINOR] Map scheduled attachment validation failures to HTTP 4xx.
The callback throws Error("Invalid scheduled attachment: ..."), but hub/src/web/routes/messages.ts:128 awaits engine.sendMessage without a catch. A stale/missing Hub file or a path owned by another session passes the prefix-only schema and is reported as HTTP 500, even though this is client input.
Suggested fix:
try {
await engine.sendMessage(sessionId, payload)
} catch (error) {
if (error instanceof ScheduledAttachmentValidationError) {
return c.json({ error: error.message, code: 'scheduled_attachment_invalid' }, 400)
}
throw error
}Use a typed validation error rather than matching the message string, and cover the route with a valid-format but missing Hub path.
There was a problem hiding this comment.
Findings
- [Major] Filter consumed scheduled rows in SQL before startup reconciliation —
reconcileConsumedScheduledAttachmentscurrently callsgetAllMessages, so every Hub restart reads and decodes the complete message history of every session before filtering to the small subset of invoked scheduled rows. HAPI message histories can contain large/compressed agent outputs; this makes restart work grow with all historical chat data and can block the event loop while reconciliation runs. Evidencehub/src/sync/messageService.ts:313, full-history decoder contexthub/src/store/messages.ts:334.
Suggested fix:// store/messages.ts export function getConsumedScheduledMessages(db: Database, sessionId: string): StoredMessage[] { const rows = db.prepare(` SELECT * FROM messages WHERE session_id = ? AND scheduled_at IS NOT NULL AND invoked_at IS NOT NULL `).all(sessionId) as DbMessageRow[] return rows.map(toStoredMessage) } // messageService.ts const messages = this.store.messages.getConsumedScheduledMessages(sessionId)
Questions
- None.
Summary
Review mode: follow-up after new commits. The previous duplicate/unbounded attachment and HTTP 500 findings are resolved. One additional startup performance regression remains: reconciliation scans and decodes all message history instead of querying only consumed scheduled rows.
Testing
- Not run (automation); static inspection only. PR code was not executed per the review security constraints.
HAPI Bot
| } | ||
|
|
||
| async reconcileConsumedScheduledAttachments(sessionId: string): Promise<void> { | ||
| const messages = this.store.messages.getAllMessages(sessionId) |
There was a problem hiding this comment.
[MAJOR] Filter consumed scheduled rows before decoding message content.
getAllMessages(sessionId) reads and decodes every historical message, then this line filters in memory. The constructor runs this reconciliation for every session on each Hub start, so restart cost grows with all chat history—including large/compressed agent outputs—even though only invoked scheduled rows are relevant.
Suggested fix:
// MessageStore
getConsumedScheduledMessages(sessionId: string): StoredMessage[] {
return getConsumedScheduledMessages(this.db, sessionId)
}
// SQL helper: WHERE session_id = ?
// AND scheduled_at IS NOT NULL AND invoked_at IS NOT NULL
const messages = this.store.messages.getConsumedScheduledMessages(sessionId)Add a store/service test with many ordinary messages plus one consumed scheduled attachment and assert reconciliation receives only the scheduled row.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits. No reportable issues found in the latest full diff. The prior startup-reconciliation finding is addressed at this head: consumed scheduled rows are filtered in SQL before message decoding. Residual risk remains in scheduled-attachment lifecycle behavior under real process and network churn.
Testing
- Not run (automation); static inspection only. PR code was not executed per the review security constraints.
HAPI Bot
1 participant
Summary
Problem / Motivation
This change builds on the existing scratchlist v2 implementation from #893 / PR #896, which already provides Hub-backed persistence, basic CRUD, local migration/cache behavior, attachment storage, validation, and session-scoped synchronization.
The remaining issues were that manual ordering was not persisted, row actions occupied too much space, editing and multiline behavior were inconsistent, image previews could reload unnecessarily, scheduled messages rejected durable scratchlist attachments, and scrollbars could change the visible row layout.
Implementation
Validation
Related Issues
Refs #893 (follow-up to PR #896).
AI Assistance
OpenAI Codex (GPT-5.6) was used to implement, test, and review the changes.