Skip to content

fix(desktop): refuse a pasted or dropped folder as an attachment - #5293

Open
Totoro-qaq wants to merge 3 commits into
apache:mainfrom
Totoro-qaq:fix/pasted-folder-attachment
Open

Totoro-qaq wants to merge 3 commits into
apache:mainfrom
Totoro-qaq:fix/pasted-folder-attachment

Conversation

@Totoro-qaq

@Totoro-qaq Totoro-qaq commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

A folder copied in Finder and pasted into the composer, or dropped on it, was staged like any other attachment. It only failed on send, with a generic error, because the File behind a folder can never be read.

  • Before staging, the composer checks each pasted or dropped file. The preload reads its path with webUtils.getPathForFile, and main stats the path and returns only one boolean per path. Folders are left out of the batch and a toast explains why. When a local folder reference is available, the toast points to Reference folder. The folder is not turned into a reference automatically, because a reference and an attachment mean different things to the model, so that choice stays with the user.
  • If a surface can't detect folders, or detection fails, files stage as they did before.
  • Backstop: a File that can't be read at send time now reports item_unreadable instead of a generic send failure.

Fixes #5279

Verification

  • New tests:

    • main detectAttachmentDirectories: directory, file, missing, relative (.), empty and non-string paths; request bounds; the IPC channel.
    • encodeIngestItems: an unreadable File maps to item_unreadable.
    • the composer hook: a folder is refused while its sibling stages; the two toast variants; no detector, or a detector that throws.
  • Each guard was mutated out of the built output and a test failed each time (6 mutants).

  • @maka/ui test:dist: 458/458.

  • @maka/desktop test:dist: 2608 passed. 8 were cancelled in mcp-oauth-controller.test, and the same 8 are cancelled on a build without this change (Node 22.23.1).

  • Desktop typecheck, biome lint and format, ASF headers, Windows test inventory, and Windows package source closure all pass.

  • Manual, macOS, dev build (npm run dev), local Host ready. Each of these shows the toast below:

    • Cmd+V of a folder copied in Finder
    • dragging a folder onto the composer
    • dragging a folder together with a file (here the file still stages)
    maka-5293-folder-toast
  • Not tested on Windows (Explorer drag or copy-paste of a folder).

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Claude Code (Claude Opus 5) helped investigate, implement and write tests. I reviewed and tested the result.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

A folder copied in Finder and pasted into the composer, or dropped on it,
arrives as a File whose bytes can never be read. It staged like any other
attachment and only failed on send, where the DOMException from
arrayBuffer() surfaced as a generic send failure (apache#5279).

The composer now asks, for each pasted or dropped file, whether it is a
directory: the preload reads the file's path with webUtils.getPathForFile
and main answers only a boolean per path. Folders are left out of the
batch with a toast; when a local folder reference is available the toast
points at Reference folder instead. Surfaces without detection, or a
detection that fails, stage files as before.

As a backstop, a File that cannot be read at send time is now reported
as item_unreadable rather than a generic failure.

Generated-by: Claude Code
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 14, 2026
@Totoro-qaq
Totoro-qaq marked this pull request as ready for review September 14, 2026 09:53

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change adds a main-process stat boundary for pasted/dropped File objects, filters directories before staging, and classifies unreadable staged items so the UI can explain the failure. I traced the shared composer hook through AppShell, WorkHub, and Quote Companion and found one non-blocking propagation gap.

Findings: 1 × P3 (inline). No P0-P2 issue found.

Validation completed on 6ac41074a87ce9fe85ce1f2c683c6027ea984f4f: build:test; full typecheck/lint/format; ASF headers; changed-file Biome; diff check; UI 458/458; focused Desktop attachment/Workbar 27/27; renderer architecture 112/112; E2E budget 38; renderer production build; hosted test and package; clean merge with current main f32cf2b482a17f235b92e2775ed2672c2775e219. Full Desktop reached 2,474 passes with 8 cancellations in the unchanged MCP OAuth deadline suite; the same 8 cancellations reproduce when that suite runs alone.

Required conclusion:

  1. The directory-detection design is appropriate and keeps filesystem paths out of renderer-visible results, but the unreadable-item fallback is not yet propagated consistently across every composer surface.
  2. Production code to delete: none identified.
  3. Tests to delete/replace: none identified; add a production-adapter regression for the inline case.
  4. A deeper refactor is not required. Preserve the classified attachment error through the Workbar adapter and localize it at the Quote Companion boundary.
  5. No P0-P2 blocker was found; the remaining P3 should be fixed or explicitly accepted before merge.
  6. Residual gaps: I could not exercise native macOS Finder paste/drop or native Windows drag/drop on this Linux host.

This changes user-visible attachment handling and error presentation, so it requires independent human review under CONTRIBUTING.md.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

// and so does a file moved or made unreadable after it was staged.
// Name the item instead of letting the DOMException become a generic
// send failure.
throw new AttachmentIngestBlockedError('item_unreadable');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Preserve item_unreadable through the Quote Companion send path.

This new classified fallback is lost in one production consumer. preload.ts:2192-2195 correctly returns { reason: 'attachment_blocked', code: 'item_unreadable' }, but create-workbar-services.ts:83-87 replaces every non-outcome_unknown result with a plain Error, and use-quote-companion.ts:1475-1484 renders only the generic “follow-up failed” copy. Thus a folder that reaches this backstop because detection failed open, or an item that became unreadable after staging, gets the new actionable explanation in AppShell/WorkHub but not in Quote Companion.

I reproduced this with the compiled createDesktopWorkbarServices: an attachment_blocked/item_unreadable bridge result became {"name":"Error","message":"Runtime Host refused the follow-up Message","code":null}. Please preserve the classified error through the Workbar adapter, localize it in the companion catch, and cover that production path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, confirmed. The adapter turned every refusal other than outcome_unknown into a plain Error, so Quote Companion lost every attachment_blocked code, not only the new one.

Fixed in 80d7284. The Workbar adapter now throws AttachmentIngestBlockedError(code), and the companion's follow-up catch localizes it with localizedShellErrorMessage. Other failures keep the generic copy. New tests:

  • an adapter regression: an attachment_blocked / item_unreadable result rejects with the classified error
  • two companion tests: the named rule, and the generic failure

Removing either guard makes its test fail.

Claude Code-assisted.

…sation

The Workbar adapter turned every refused follow-up into a plain Error, so a
Quote Companion send blocked for an attachment (item_unreadable, or any other
attachment_blocked code) showed the generic "follow-up failed" copy. Keep the
AttachmentIngestBlockedError through the adapter and localize it where the
companion handles the failure, as the main composer does. Other failures
keep the generic copy.

Generated-by: Claude Code

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This head preserves classified attachment refusals through the Workbar follow-up adapter and localizes them in Quote Companion. That fixes the previously reported follow-up path, but the first-send path still loses the same classification.\n\nFindings: 1 x P3 (inline). No P0-P2 issue found.\n\nValidation completed on 80d7284f7aa82e1b4b857578bd1d0207edc4ec24: build:test; full typecheck/lint/format; ASF headers; Biome on all 17 changed TypeScript files; UI 458/458; focused Desktop attachment/Quote Companion/Workbar 85/85; renderer architecture 112/112; E2E budget 38; renderer production build; hosted test and package; diff check; clean merge with current main 4a42aaeab4a7435a84c7d512e81b4d803f9ee0aa. Full Desktop reached 2,477 passes with 8 cancellations in the unchanged MCP OAuth deadline suite; the same 8 cancellations reproduce when that suite runs alone.\n\nRequired conclusion:\n1. The directory-rejection design remains appropriate, and this commit fixes classified follow-up failures, but initial Side Conversation sends still collapse the same refusal to generic copy.\n2. Production code to delete: none identified.\n3. Tests to delete/replace: none identified; add a first-send regression beside the new follow-up cases.\n4. A deeper refactor is not required. Extend the existing SideChat send result/error mapping so both entry paths preserve the same classification.\n5. No P0-P2 blocker was found; the remaining P3 should be fixed or explicitly accepted before merge.\n6. Residual gaps: native macOS Finder paste/drop and native Windows drag/drop were not exercised on this Linux host.\n\nThis changes user-visible attachment handling and error presentation, so it requires independent human review under CONTRIBUTING.md.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

// stable answer, not a transient failure: name the rule instead of
// inviting a retry.
const message =
failure instanceof AttachmentIngestBlockedError

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Preserve classified attachment refusals on the first Side Conversation send too.\n\nThis new handling only covers submitFollowUp. The initial send path still goes through performCompanionTurn, where SideChatSendResult drops the attachment code and every non-outcome_unknown refusal becomes send_rejected (quote-companion-core.ts:379-383). On this exact head, a compiled production adapter probe with sessions.send -> { ok: false, reason: 'attachment_blocked', code: 'item_unreadable' } returned { status: 'error', code: 'send_rejected' }, while the new follow-up path preserved AttachmentIngestBlockedError/item_unreadable. Therefore an unreadable item on the first prompt still shows generic rejection copy; only a steer/follow-up gets the actionable attachment message. Please carry the classified refusal through SideChatSendResult / performCompanionTurn and add the matching first-send regression.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, confirmed. SideChatSendResult had no attachment_blocked variant, and performCompanionTurn turned every refusal other than outcome_unknown into send_rejected. So the first send lost the code even though the adapter passes the bridge result through unchanged.

Fixed in 80a639f. SideChatSendResult now declares the attachment_blocked variant, and performCompanionTurn keeps its code on the send_rejected result. Quote Companion shows the same attachment copy for the first send and for follow-ups, through one lookup. Other refusals keep the generic copy. New tests:

  • a core regression: performCompanionTurn keeps item_unreadable on a refused first send
  • two companion tests: the named rule, and the generic rejection for any other reason

Replacing either guard with false makes its test fail.

Claude Code-assisted.

…conversation send

performCompanionTurn turned every refused first send other than
outcome_unknown into send_rejected, so a Quote Companion prompt blocked
for an attachment still showed the generic rejection copy. Declare the
attachment_blocked result on SideChatSendResult, keep its code on the
send_rejected result, and show the same attachment copy the follow-up
path uses.

Generated-by: Claude Code

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This head fixes the remaining Side Conversation attachment-error propagation gap. The first-send result now preserves attachment_blocked with its bounded refusal code through SideChatSendResult and performCompanionTurn, and the Quote Companion hook renders the same actionable localized message already used by follow-up sends. The new core and hook regressions exercise both the structured result and the user-visible error.

No P0-P3 finding remains.

Validation completed on exact head 80a639f3b69e1eb062bca0983e5d5743525f1739: build:test; full typecheck/lint/format; ASF headers; UI 458/458; focused Desktop attachment/Quote Companion/Workbar 92/92; renderer architecture 112/112; E2E budget 38; renderer production build; hosted test and package; diff check; clean merge with current main 4a42aaeab4a7435a84c7d512e81b4d803f9ee0aa. Full Desktop reached 2,480 passes with 8 cancellations, all in the unchanged MCP OAuth deadline suite.

Production code or tests to delete: none identified. A deeper refactor is not required; the fix extends the existing bounded error contract across the one missing entry path. Residual gap: native macOS Finder paste/drop and native Windows drag/drop were not exercised on this Linux host.

This changes user-visible attachment handling and error presentation, so it still requires independent human review under CONTRIBUTING.md.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(desktop): pasting a Finder folder stages it as a file and makes send fail

2 participants