mirror of
https://github.com/multica-ai/multica.git
synced 2026-08-06 10:50:54 +02:00
bugfix/qoder-login-shell-discovery
18 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Bohan Jiang
|
2187b58292 |
feat(attachments): let callers opt out of pre-signed URLs in bulk responses (MUL-5372) (#6119)
* feat(attachments): let callers opt out of pre-signed URLs in bulk responses
A CloudFront-signed download_url is ~800 chars, ~630 of which are a
Policy+Signature pair re-minted on every request (the policy embeds
now+TTL at second granularity). Emitting one per attachment on every
list response costs three ways: raw payload, a fresh RSA sign per
attachment per request, and — because the bytes differ on every read —
it defeats any cache keyed on response content.
Agents pay all three and use none of it. `multica attachment download
<id>` fetches a fresh signature from the single-attachment endpoint, so
the id is the only part the CLI needs; measured on a 15-attachment
issue, the URL fields were 28.3% of the payload and ~10k chars of that
rotated on every read.
Callers now advertise `stable_attachment_urls` in X-Client-Capabilities
to receive the stable /api/attachments/{id}/download path instead. That
endpoint re-signs and 302s on every hit, so the value stays correct
forever at ~95 chars. The CLI advertises it unconditionally — it is a
protocol detail, not a user-visible flag.
The single-attachment endpoint keeps signing regardless of the
capability: it is the one source of fresh, natively-loadable URLs, and
stable-mode callers exchange a stable path for a signature there. That
carve-out is what makes the rest safe.
The server default never moves. A caller that advertises nothing — every
installed mobile build, every third-party script — gets byte-identical
responses, so this ships ahead of any client and each surface migrates
on its own release cadence. Web/desktop (Phase 2) and mobile (Phase 3)
are deliberately not touched here.
MUL-5372
Co-authored-by: multica-agent <github@multica.ai>
* test(attachments): pin the CLI capability header and the rotation premise
Two review nits from #6119.
The CLI's setHeaders call is the only place Phase 1 is switched on, and
nothing observed it: the server tests prove a request carrying
`X-Client-Capabilities: stable_attachment_urls` gets stable paths, but a
typo in the CLI token — or dropping the header — would have left every
test green while the CLI silently went back to ~800-char signed URLs.
Assert the header verbatim across GET, GET-with-headers, POST, and the
no-token client. Verified the assertion is load-bearing by mutating the
constant to `stable_attachment_url`: the test fails.
TestAttachmentToResponse_SignedModeRotatesButStableDoesNot only checked
the "stable does not" half, so its name overstated coverage. Add the
rotation half. It drives the signer with two explicit expiries rather
than calling attachmentToResponse twice: that function mints its expiry
from time.Now() at second granularity, so consecutive calls usually land
in the same second and asserting on them directly would be a flaky test
of a real property. Also pins that only the query rotates, and that a
fixed expiry re-signs deterministically — without which the rotation
assertion would prove nothing about the clock.
MUL-5372
Co-authored-by: multica-agent <github@multica.ai>
---------
Co-authored-by: Bohan-J <bohan@devv.ai>
Co-authored-by: multica-agent <github@multica.ai>
|
||
|
beast
|
60048172a7 |
fix(lark): ingest inbound images and videos as chat attachments (MUL-4934) (#5580)
* fix: ingest feishu media as chat attachments * fix: ingest feishu post embedded media * fix(lark): make inbound media retries safe * fix lark media resource limit * fix(lark): move inbound media off ack path * fix(channel): make inbound media runs durable * fix(channel): close enqueue-vs-append race on media deferral EnqueueChatTask read the session-wide media deadline in one statement and sealed the input batch in a later one. Under READ COMMITTED a media message committing between the two got sealed into a task the deadline read had already decided was 'queued', so the daemon could claim it before its attachment bound — the agent received the bare placeholder, and the later media-ready promotion was a no-op against a non-deferred task. After the seal, re-derive the deferral from the sealed batch itself in the same transaction (DeferChatTaskForSealedPendingMedia): if any sealed message still carries an unexpired media marker, flip the task to deferred with fire_at aligned to the latest marker. The existing post-commit promote fence already covers the opposite direction (marker cleared mid-transaction). Adds a deterministic regression test that injects the media append between the deadline read and the seal via a wrapped pgx.Tx. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): keep committed chat task out of enqueue error path The post-commit media-ready fence returned its error from EnqueueChatTask even though the deferred task was already durably committed. The router flush treats any enqueue error as "no task exists": it clears the typing indicator and logs an enqueue failure while the run still happens at its fire_at deadline. Log the fence failure instead — the claim-path deferred promoter re-queues the task regardless. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): cap global media resolution concurrency Media jobs were serialized per session but unbounded across sessions: a burst could open arbitrarily many concurrent 45s Lark downloads, and each unknown-length upload may buffer up to the 100 MiB resource cap in memory. Gate resolveAndBindMedia behind a global slot semaphore (default 8, RouterConfig.MediaConcurrency). Per-session ordering is unchanged; on shutdown a job cancelled while waiting for a slot proceeds straight to the bounded DB finalize so marker clearing stays prompt. Also document that the per-message media budget spans queue/slot waits (it must match the persisted fire_at) and why timed-out uploads cannot leak unbounded orphans. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(chat): keep channel-sealed user messages on task cancel Sealing the channel input batch stamps task_id onto channel user messages, which exposed them to the cancel draft-restore path: an empty-transcript cancel would DeleteUserChatMessageByTask the sealed Feishu/Slack messages and detach their attachments. Those messages are the durable record of what the platform sender wrote — the sender has no Multica composer to restore a draft into. Gate the restore-delete on ChatSessionHasChannelBinding in both the synchronous finalize and the deferred finalize (the latter covers markers left by an older replica during a rolling deploy); a bound session now settles as "Stopped." instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): skip the media pipeline for messages without media Every inbound message on a Media-enabled platform persisted a 45s media deadline and queued a resolution job, so a plain text message could wait behind the global media semaphore (its task deferred while other sessions download 100 MiB videos) and a crash between append and clear delayed a pure-text run to the full 45s fallback. Add MediaResolver.HasMedia — a pure in-memory probe the Router calls on the ACK path — and only persist the deadline / enqueue the job when the message actually references platform media. The Feishu resolver decodes the already-received payload and reports standalone image or video keys and post-embedded img/media spans. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(chat): gate cancel restore on immutable channel provenance The previous guard keyed the cancel restore-delete off ChatSessionHasChannelBinding, but a binding only proves routing exists right now: archiving a session and rebinding an installation both delete the binding while preserving chat history, so a still-cancellable sealed task could again restore-delete the original inbound messages. Persist provenance on the message instead: migration 203 adds chat_message.channel_ingested, stamped inside the channel append transaction and never mutated, and both cancel finalize paths now gate on TaskHasChannelIngestedMessages over the task's sealed batch. The binding-existence query is removed. Regression tests cover ingest -> archive/unbind -> cancel for a queued and a started task. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): reclaim media uploads that never gain an attachment row Deadline expiry dropped already-resolved refs and a BindMedia failure was log-only, leaving uploaded objects with no attachment row and no reclaim path — the dedup mark commits with the message before media runs, so a redelivery is dropped as a duplicate and never re-resolves (and thus never overwrites) those keys, and workspace/session deletion only enumerates the attachment table. Add MediaResolver.DiscardMedia — a best-effort delete by StorageKey — and call it from both failure paths in resolveAndBindMedia. The Feishu resolver forwards to the storage backend's Delete. Tests cover a partial upload discarded at the deadline, discard on bind failure, and key-level deletion in the resolver. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(server): refresh comments stale after detached media ingestion Channel tasks now seal a self-owned input batch, media ingestion is no longer out of scope for the flattener, and MediaRefs are filled by the detached resolver after append rather than by feishuChannel pre-engine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(chat): stop keying channel empty-completion silence off chat_input_task_id Sealing gave channel tasks a self input-owner, which broke writeChatCompletionOutcome's discriminator: it treated any owned task as direct, so an empty channel completion wrote the no_response fallback row and the outbound patcher — which forwards any non-empty chat:done content verbatim — pushed the English fallback body to Feishu/Slack, violating the MUL-4351 contract. Silence is now decided by the immutable channel_ingested provenance of the task's input batch, looked up by the batch OWNER id (chat_input_task_id): auto-retry clones inherit the owner while their sealed messages stay tagged with the parent's id, so keying off the task's own id would misread a channel retry as direct. The cancel-path provenance gates switch to the same owner key via chatInputOwnerID. chat_input_task_id is back to meaning only "input batch owner". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(migrations): renumber to 203/204 after upstream took 202 Upstream main merged 202_runtime_profile_add_qwen while this branch held 202/203, tripping TestMigrationNumericPrefixesStayUniqueAfterLegacySet on the CI merge tree. channel_media_pending becomes 203 and channel_ingested becomes 204; no content changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channels): gate outbound delivery on channel provenance, not owner Merging main brought #5645 (keep direct chat replies in Multica), whose outbound gate assumed channel tasks leave chat_input_task_id NULL. Sealed channel tasks own an input batch too, so on the merge tree every channel reply and failure notice was classified as direct and silently dropped — agents stopped replying in Feishu/Slack. Both outbound gates now call engine.TaskInputIsChannelIngested: a NULL owner keeps #5645's deliver-by-default for pre-sealing tasks, an owned batch delivers only when it carries the immutable channel_ingested stamp (keyed by the owner id, so auto-retry clones inherit the verdict). Direct replies stay in Multica; sealed channel replies reach the platform. Tests cover both directions on both platforms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): discard media orphans on a fresh context, after finalize DiscardMedia shared finalizeCtx with BindMedia, so a bind that failed because the finalize deadline expired handed the storage deletes an already-dead context — the compensation silently no-opped and the orphans leaked anyway. The deadline path also ran S3 deletes before the marker clear, eating the same 5s budget the user-facing bind/promotion needed. Collect the refs from both failure paths, run bind + promotion on the finalize budget first, then delete on a fresh discard context. The bind-failure test now pins that discard receives a live context. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): compensate result-uncertain media uploads and commits The compensation protocol treated "the call returned an error" as "the side effect did not happen", which is wrong in both directions across the result-uncertain windows: - An upload error can follow a server-side write (lost response, deadline mid-write). The attempted key never reached the router, so nothing could reclaim it and dedup guarantees no re-resolve. The resolver now idempotently deletes the deterministic key on a fresh budget right at the failure site. - A commit error is not a rollback guarantee: a lost ack can report failure after Postgres durably committed the attachment rows, and the router's discard would then delete objects those rows reference. BindMediaRefs now converges the ambiguity on a fresh budget — any of the batch's URLs present proves the atomic commit landed (bind reports success); none proves the rollback (discard stays safe); a failed verification returns ErrMediaBindResultUnknown and the router keeps the uploads, preferring a rare orphan over a broken attachment. Fault-injection coverage: an upload error deletes the attempted key; a lost-ack commit keeps the bound attachment and reports success; a verified rollback stays a discardable error; the router keeps uploads on the unknown-outcome sentinel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(migrations): renumber to 207/208 after upstream took 203-206 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(channel): note DiscardMedia self-invocation and the unknown-outcome skip Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(migrations): renumber to 212/213 after upstream took 207-211 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(channel): replace inline media compensation with an intent ledger and reconciler Inline best-effort compensation cannot answer "did my side effect happen?" at the moment it needs the answer — the DELETE/PUT reordering and the empty-read-vs-in-flight-COMMIT gaps were both instances of the same two-system atomicity problem. Persist the intent instead and let an asynchronous reconciler settle it: - channel_media_pending_object (migration 214; claim index 215 as its own single-statement CONCURRENTLY migration): a state machine row ('pending' -> 'deleting') with lease, attempt, and backoff columns. - The resolver upserts the row BEFORE each PUT, state-guarded so a key the reconciler owns is never resurrected (the resource is skipped). ObjectURL is a pure function of configuration, so the row carries the attachment URL pre-upload. - BindMediaRefs deletes the batch's rows INSIDE the attachment-insert transaction: commit landed <=> intents gone, atomically, so an ambiguous COMMIT never needs adjudication. A key already claimed to 'deleting' is skipped (placeholder stays). - Nothing is ever deleted inline. The reconciler — an independent worker so storage latency cannot starve other sweepers — claims due rows ('pending' past the settle delay, or expired leases) under a fresh lease, checks for a durable attachment reference only AFTER the claim (race-free: bind can no longer succeed on the key), deletes unreferenced objects outside any transaction, and backs off failed deletes with attempt-based retry. Crash windows converge for free. - The settle delay is a fixed constant carrying NO correctness weight; invariant tests pin it at >=10x every pipeline budget. Metrics cover deletes, referenced clears, delete failures, and ledger backlog. Removed: MediaResolver.DiscardMedia, ErrMediaBindResultUnknown, the post-commit verification, and both router discard branches. Tests: intent-before-upload ordering; upload error leaves the row and deletes nothing; bind-wins vs reconciler-wins on the same key; lost-ack and rolled-back commit injections (intent cleared iff the attachment landed); reconciler three-state settle; expired-lease reclaim; delete failure backoff and retry; settle invariants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): never build or sweep the media reconciler without storage store is nil when S3 is unconfigured AND the local upload dir fails to initialize, but the reconciler was constructed unconditionally and main only gates the goroutine on the reconciler pointer — the first unreferenced ledger row (rows can pre-exist from a boot where storage worked) would nil-pointer panic a bare goroutine and take down the process. Construct the reconciler only when a storage backend exists, and guard RunOnce defensively: with no deleter it skips the sweep without claiming, so rows are not stranded in 'deleting' until lease expiry. Test covers the pre-existing-row + missing-storage boot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(migrations): renumber to 213-216 after upstream took 212 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * refactor(channel): remove the dead pre-resolved MediaRefs ingress path lark.InboundMessage.MediaRefs and the resolver's early-returns for pre-populated refs were vestiges of the pre-detached synchronous design — no producer fills them before the router anymore. Worse, the intent ledger made the path actively misleading: refs arriving without ledger rows would be silently skipped at bind (with a log blaming the reconciler), contradicting the field's "already persisted" contract. Delete the field, its channelMessageFromLark mapping, and both early-returns; channel.InboundMessage.MediaRefs is now documented as what it actually is — ResolveMedia's output channel, always empty on ingress, attachable only through a claimed ledger intent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): enforce workspace tenancy on every ledger query The intent-ledger upsert's conflict branch guarded only on state, so a cross-workspace storage_key collision could rewrite the row's workspace/message/url ownership; release and delete keyed on (storage_key, lease_token) alone. The derived key embeds the workspace UUID so none of this is reachable today — but tenancy must be enforced by the workspace column in every query, never derived from the key string (MUL-3515 rule, restated in this PR's review). The upsert now updates only within the same workspace (a cross-tenant conflict updates nothing, returns no row, and the resolver skips the upload — the fail-safe direction), and release/delete take (workspace_id, storage_key, lease_token). Tests pin that a foreign workspace can neither steal, release, nor delete a row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(migrations): build the ledger primary key via a concurrent index storage_key TEXT PRIMARY KEY created its unique index implicitly at CREATE TABLE, against the repo convention that every migration index — including a new table's unique index — is built CONCURRENTLY in its own single-statement migration (the exact three-step pattern client_usage_daily shipped in 207-209). The table now declares storage_key NOT NULL, 216 builds the unique index concurrently, and 217 attaches the primary key USING INDEX; the claim index moves to 218. ON CONFLICT (storage_key) still resolves against the constraint, and the full down/up round-trip is verified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): bound each reconciler object delete with its own timeout DeleteObject ran on the worker-lifetime context and the SDK's default HTTP client has no overall request timeout, so one black-holed connection would wedge the sequential sweep loop — and with it every later batch and the backlog gauge — forever; a single-replica deployment has no other worker to reclaim the lease. Each delete now gets a 30s timeout (well under the 2min lease), and a timed-out delete takes the existing release/backoff path. Covered by a blocking-deleter test with an injectable timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): anchor the media deadline to the DB clock and bound queue waits by it Two deadline gaps from review: - The persisted marker was an application-clock timestamp compared against SQL now() everywhere it is read, so a skewed app node could shrink the fallback window and hand the agent a placeholder before the resolver's local budget ended. The append transaction now anchors a relative budget (MediaPendingSeconds) with now() + make_interval, writer and readers sharing one clock; the local resolve budget stays monotonic app-side. A DB test pins that the remaining budget measured by the DB clock equals the requested one. - enqueueMedia's waits (per-session order, global slot) only watched shutdown, so in a burst an already-expired job kept its goroutine and payload until it reached the front. Both waits now also watch the message's deadline; on expiry the job skips the resolver entirely and runs only the empty finalize (marker clear + promotion), which also unblocks the session's later messages. Covered by a queued-expiry test that finalizes while the only slot is deterministically held. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(migrations): renumber to 216-221 after upstream took 213-215 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): start the local media budget before the append transaction The DB anchors the durable fallback at insert-time now(), but the local monotonic budget started only after AppendMessage returned — so the resolver outlived the fallback by the append/commit latency, a window where the deferred task is already claimable while the resolver still runs and the agent reads a placeholder that binds moments later. Capture the local deadline before calling AppendMessage, restoring the ordering local-gives-up <= durable-fallback-fires. A slow-append test pins that the resolver's context deadline is measured from the pre-append instant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(migrations): renumber to 224-229 after upstream took 216-223 Verified against the merged tree: the numeric-prefix uniqueness test passes and the full migration set applies cleanly from scratch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): heartbeat the reconciler lease per row One claim covers up to 50 rows under a single 2-minute lease, but the batch is processed sequentially and each delete may run its full 30s timeout — a few stalled deletes could outlive the lease mid-batch, letting another replica reclaim the tail: duplicate concurrent deletes, inflated attempt/backoff on rows whose owner was alive, and skewed metrics. The lease is now renewed before EACH row's settle work, so it only ever needs to cover one row's worst case (invariant-tested: lease >= 2x the per-delete timeout). A renewal that matches no row means another worker reclaimed it after a genuine expiry — the row is skipped, leaving the new owner's state untouched. Test simulates a mid-batch reclaim and pins the skip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): dedup post media resources and make local writes atomic A rich post may reference the same image_key/file_key in several spans. The object key derives from (message, type, key), so duplicates uploaded to the SAME key twice: LocalStorage.UploadStream truncated the destination up front and removed it outright on a copy error, so a second failing attempt destroyed the object the first success had produced — leaving an attachment row pointing at nothing. A second succeeding attempt instead produced two attachment rows for one object. Collapse duplicate spans by (fetch type, platform key) before the upload loop, and write local uploads through a temp file renamed into place so a failed write can only discard its own temp file. Tests cover a duplicated span uploading once and a failed re-upload leaving the previous object intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): fence late-materializing PUTs with a tombstone schedule A DELETE cannot be ordered against a PUT the client already abandoned: the store may materialize the object after the delete completes. The reconciler cleared the ledger row right after deleting, so such an object had no row and nothing to reclaim it — which made the settle delay the de-facto correctness barrier for the PUT/DELETE race, exactly what the design says it must not be. The row is now kept as a tombstone ('tombstoned' state, migration 226's CHECK) and re-deleted on a widening schedule (15m, 1h, 6h, 24h, the pass index carried in last_error), so a late materialization is reclaimed by a later pass; only after the schedule is exhausted is the row dropped. Claim, heartbeat, lease, and tenancy predicates are unchanged — a tombstone is claimed exactly like any other due row. A separate gauge reports tombstones so they cannot be mistaken for a backlog of objects awaiting reclaim, and the header comment now states precisely what state fences (bind/commit) versus what the schedule fences (late PUTs). Tests: the reviewer's interleaving — DELETE completes, the abandoned PUT materializes right after, and the object is gone by the end of the schedule — plus a full schedule walk asserting the object is counted once and the row clears at the end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): tombstones must re-delete, not re-ask the reference question A tombstone revisit ran the same reference check as a first settle, so an attachment carrying the same URL — a re-ingested copy of the object — sent the row down the "referenced, keep it" branch: the object was kept and the row cleared, abandoning the re-delete schedule that fences the ORIGINAL object against an abandoned PUT. A tombstone has already been judged unreferenced and deleted; it exists only to re-delete whatever materializes later, so it now goes straight to the delete + schedule tail (extracted as settleDeletedObject, shared with the first settle). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(channel): keep the tombstone schedule position in its own column The re-delete pass index was encoded into last_error, which the failure path also writes: one failed re-delete erased the position and restarted the walk. A store failing intermittently could therefore keep a tombstone alive indefinitely — every recovery would resume at pass 1 and the row would never reach the end of the schedule to be dropped. tombstone_pass is now its own column (the table is introduced in this PR, so migration 226 carries it), advanced only by a successful delete, and the tombstone write clears the now-stale last_error. Test walks the schedule across a failed re-delete and asserts it resumes rather than restarts, and that the row still terminates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(lark): derive media object keys per chat message The object key was derived from the platform message alone, so a second ingest of the same Feishu message reused the first ingest's ledger row. That row can be a tombstone (up to ~31h while the re-delete schedule runs), and the intent upsert refuses anything that has left 'pending', so the second ingest skipped the upload and silently produced a placeholder with no attachment. A re-ingest is reachable: the inbound dedup claim is reclaimable once 60s stale and the dedup row is only vacuumed after 24h. Keying on the chat message the object will attach to keeps the two ingests independent, and nothing leaks: each one's objects are covered by its own ledger row. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(storage): route both local upload paths through one atomic write UploadStream wrote through a temp file and renamed into place, but the buffered Upload path still truncated the destination up front — the destructive shape the stream path exists to avoid, one caller away from coming back. Both now share writeAtomic, which also restores the 0644 the direct write used (CreateTemp makes files 0600). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(channel): gofmt the media-pending append fields Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(storage): keep the local upload chmod best-effort The rename-into-place rewrite made a failed chmod fail the whole upload. CreateTemp's 0600 has to be widened to the 0644 the direct write used, but an upload dir on a mount that ignores chmod (SMB/NFS/FUSE) accepted the old direct write fine — turning those deployments' uploads into hard errors would be a regression for a cosmetic property. Log and continue. Tests pin 0644 on both upload paths, and that a failed buffered upload leaves no temp litter and no damage to a previous object. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(channel): never re-delete an object an attachment references The tombstone pass skipped the reference check and deleted unconditionally, so a durable attachment carrying that URL lost the only object it can read — the dangling attachment the intent ledger exists to prevent, and the opposite of the posture every other path here takes ("a reclaimable orphan beats a broken attachment"). The check now runs on every pass. A positive result on a tombstone is unreachable by design — keys are per (chat message, resource) and a bind cannot attach a key that has left 'pending' — so reaching it means an invariant broke: keep the object, clear the row, log it, and count it on a dedicated reconciler_tombstone_referenced_total counter. The test's contract is flipped to assert the referenced object survives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(storage): make the local staging file reclaimable after a crash os.CreateTemp's random suffix meant a crash between the staging write and the rename left a file nothing could name: the ledger records only the final storage key, and DeleteObject removed only the object and its sidecar. Each leftover can approach the 100 MiB resource cap and they accumulate without bound. The staging path is now derived from the object key, so DeleteObject removes it alongside the object — which makes the media reconciler reclaim it too, since the intent row is written before the upload. Opening it 0644 directly also drops the chmod the previous commit had to make best-effort. Both read paths refuse the staging name (keys come from the request URL, and a half-written body should not be readable); a user-supplied ".tmp" extension is unaffected, since object keys are generated and never dot-prefixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(channel): renumber the media migrations after merging main main took 224 (agent_task_session_rollout_missing), so the ledger group moves to 225-230 and the cross-references inside the table migration follow. main's CompleteTask also grew a sessionRolloutMissing parameter; the three call sites this PR added to chat_input_ownership_test.go pass false. Verified the way the numbering is meant to be verified: full migration set applied from scratch on the merged tree, and the whole server suite run against that database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(channel): let Postgres compute every reconciler deadline The reconciler built settle cutoffs, lease expiry, backoff and re-delete times from the process clock and compared them against the database's now(). A replica whose clock had drifted would therefore settle rows whose upload was still in flight (the object is deleted and the bind then refuses to attach — media silently lost), hand out leases that are born expired (rows churn between workers, attempt/backoff inflate), or compress the tombstone schedule that fences a late-materializing PUT. The four settle queries now take durations and derive their timestamps from now(), so every replica reads one clock. The parameter types are the guard: an app-side timestamp can no longer be passed. Test asserts the persisted lease, backoff and re-delete deadlines all track the database's now(). The generated code also picks up main's new agent_task_queue column in the two RETURNING task.* queries this PR adds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(lark): drop unrelated gofmt-only churn from this PR Six files carried whitespace/comment-reformatting with no functional change, unrelated to the inbound media pipeline. Reverting them to the base revision keeps the diff focused on the feature (75 -> 69 files): server/internal/service/empty_claim_cache.go server/internal/integrations/lark/markdown_detect.go server/internal/integrations/lark/ws_chunk_assembler.go server/internal/integrations/lark/ws_chunk_assembler_test.go server/internal/integrations/lark/ws_frame_test.go server/internal/integrations/lark/registration_test.go Verified: `git diff -w` against these files was already empty, so no behavior is affected. go vet clean; tests covering these files pass. Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Bohan-J <bohan@devv.ai> Co-authored-by: multica-agent <github@multica.ai> |
||
|
kami
|
156c592c9d |
fix(attachments): presign desktop inline media (#5946)
* fix(attachments): presign desktop inline media * refactor(attachments): unify inline-media presign to DownloadPresigner The GetAttachmentByID inline-media presign branch asserted storage.Presigner while resolveAttachmentDownloadMode keys its presign decision on storage.DownloadPresigner. Both are implemented by S3Storage today, but a storage implementing only one would make the mode resolution and the presign call silently disagree. Route the branch through DownloadPresigner with an empty content disposition (identical to PresignGet: the object's stored Content-Disposition is inherited) so the two can never diverge. Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: Bohan-J <bohan@devv.ai> Co-authored-by: multica-agent <github@multica.ai> |
||
|
n374
|
30d3aca600 |
feat(attachments): support HTTP Range resume on proxy download (MUL-3962)
Add HTTP Range support to the attachment proxy-download path so an interrupted download can resume from where it left off instead of restarting at byte 0. - Seekable backends (local disk) delegate to http.ServeContent for full Range / If-Range / 206 / Content-Range / 416 handling. - Forward-only backends (S3/MinIO streaming) get a single-range fallback that advertises Accept-Ranges and serves 206 + Content-Range, with a rangeParseOutcome that returns 416 only for genuinely unsatisfiable byte ranges and otherwise ignores unsupported/empty-object ranges (full 200), matching the seekable path. Closes #4831. MUL-3962. |
||
|
Ryan Yu
|
ac75a0df72 |
fix(attachments): align text preview whitelist (#4834)
Co-authored-by: Cursor <cursoragent@cursor.com> |
||
|
Bohan Jiang
|
dbf11f0958 |
fix(attachments): relax frame-ancestors on local /uploads static route (MUL-3821) (#4777)
Self-hosted local-disk deployments serve document previews straight from the public /uploads/* static route. That route inherited the global `frame-ancestors 'none'` CSP from the middleware, so iframe-based previews (PDF/HTML) were blocked by the browser — only the /api/attachments/* download endpoint had been exempted (#4635 / #4679). Serve /uploads/* through a new Handler.ServeLocalUpload that applies the same preview security headers as the download endpoint (setAttachmentPreviewSecurityHeaders), so the relaxed, config-aware `frame-ancestors 'self' <configured origins>` policy applies to both same-origin and split frontend/backend origin setups. Inline <img> rendering is unaffected (frame-ancestors does not gate images); cloud storage (S3/CloudFront) never hits this route. Adds regression tests covering the relaxed CSP on /uploads and the non-local-storage 404 guard. Refs #4477 Co-authored-by: J <j@multica.ai> Co-authored-by: multica-agent <github@multica.ai> |
||
|
Bohan Jiang
|
24754f091b |
fix: allow framed attachment redirects (#4635)
Co-authored-by: J <j@multica.ai> Co-authored-by: multica-agent <github@multica.ai> |
||
|
Bohan Jiang
|
35e5455953 |
fix: allow split-origin attachment previews (#4539)
Co-authored-by: J <j@multica.ai> Co-authored-by: multica-agent <github@multica.ai> |
||
|
Bohan Jiang
|
cb7cc82ecb |
fix: allow same-origin attachment previews (#4504)
Co-authored-by: J <j@multica.ai> Co-authored-by: multica-agent <github@multica.ai> |
||
|
Multica Eve
|
abf99eb700 |
fix(attachments): server-driven markdown_url + legacy compat (MUL-3192) (#3991)
Comment / issue / chat images uploaded inside the Desktop app rendered
as the broken-image fallback. The editor was persisting a site-relative
`/api/attachments/<id>/download` URL into markdown — that path only
resolves when the document origin proxies /api to the API host (apps/web
via Next.js rewrite). On Electron's file:// origin it never resolved.
Per GPT-Boy's plan, move the durable-URL choice from the client to the
server so the persisted shape is correct regardless of which client
performed the upload.
Server:
- AttachmentResponse gains a markdown_url field, computed by
buildMarkdownURL from the deployment policy:
• storage URL is already absolute + unsigned (public CDN, S3 public
bucket, LocalStorage with MULTICA_LOCAL_UPLOAD_BASE_URL on https) →
use it verbatim;
• CloudFront-signed mode → never expose the raw S3 URL (private
bucket); return cfg.PublicURL + /api/attachments/<id>/download so
the server can re-sign on every request;
• LocalStorage relative + cfg.PublicURL set → same prefixed API
endpoint;
• cfg.PublicURL unset → fall back to site-relative path so web's
Next.js rewrite still works.
- isDurablePublicURL helper rejects URLs carrying CloudFront / S3
signature query params, so a freshly-signed download_url can never
leak into persistence — the original MUL-3130 bug stays closed.
Frontend:
- Attachment type + AttachmentResponseSchema (and apps/mobile mirror)
carry markdown_url. Schema lenient-defaults to '' so a backend old
enough to predate this field doesn't break clients.
- useFileUpload picks markdownLink with three-layer fallback:
(1) att.markdown_url (modern server),
(2) attachmentDownloadPath(att.id) — legacy site-relative shape,
retained for backends old enough to omit markdown_url,
(3) att.url — no-workspace avatar branch with no attachment-row id.
- attachment.tsx keeps the relative→absolute absolutize pass, but
reframed as the legacy-compat fallback for already-persisted
/api/attachments/<id>/download or /uploads/<key> URLs in old
bodies. New content writes absolute URLs and skips this path.
- ContentEditor still tracks freshly-uploaded records into
AttachmentDownloadProvider so Quick Create's editor can swap the URL
via the resolver during the same session even before the server-side
binding lands.
Tests:
- server/internal/handler/file_test.go: 5 new buildMarkdownURL matrix
tests (public CDN passthrough, CloudFront-signed swap, relative
prefixing, PublicURL unset fallback, trailing-slash strip) + 15
table-driven isDurablePublicURL cases.
- packages/core/hooks/use-file-upload.test.ts: new file, 4 cases
covering modern server / legacy server / no-id avatar / oversize.
- packages/views/editor/attachment.test.tsx + content-editor.test.tsx:
10 cases for the absolutize matrix and in-session attachment merge.
- 6 existing test fixtures updated to include markdown_url.
Verification: 1236 @multica/views tests pass; 514 @multica/core tests
pass (4 new); server handler package tests pass for the new matrix
plus all pre-existing TestAttachmentToResponse* and TestDownload*
cases. Typecheck green for views/core/web/desktop. Lint clean on
touched files.
Quick Create attachment_ids binding (orphaned attachment relationship
on the resulting issue) is a follow-up — it requires a new --attachment-id
CLI flag and daemon prompt-template work and is intentionally scoped
out of this PR.
Refs: MUL-3192
Co-authored-by: Eve <eve@multica-ai.local>
Co-authored-by: multica-agent <github@multica.ai>
|
||
|
Multica Eve
|
13e9485a3b |
MUL-3130: persist stable /api/attachments/<id>/download URL in comment markdown (#3937)
* MUL-3130: persist a stable attachment download URL in comment markdown Comment image attachments rendered as broken placeholders ~30 minutes after upload because the editor was persisting a short-lived HMAC-signed URL into the comment body. After PR #3903 (MUL-3132) hardened /uploads/* with auth, `attachmentToResponse` started signing `attachment.url` as `/uploads/<key>?exp=<unix>&sig=<HMAC>` for LocalStorage so token-auth clients could keep loading inline images. The signature has a 30-min TTL by design — but `useFileUpload` was returning that signed value as `link` and the editor was writing `` straight into the markdown, so the comment permanently captured a URL that stopped working as soon as the signature expired. The fix is to persist a stable per-attachment URL that the server can re-sign on every request: * `useFileUpload` now returns `link = /api/attachments/<id>/download` (avatar uploads without an id still fall back to `att.url` so the pre-attachment-row code paths keep working). * `DownloadAttachment` self-resolves the workspace from the attachment row instead of reading X-Workspace-Slug / X-Workspace-ID headers, and the route is registered under the auth-only group so a native browser <img>/<video> resource load (which cannot attach those headers) succeeds. Membership is checked inside the handler with a 404 deny shape so the route does not act as an IDOR oracle. * A new `GetAttachmentByIDOnly` SQL query supports the workspace- derivation step. * `AttachmentDownloadProvider` now extracts the attachment id from the stable URL when matching markdown refs to attachment records, with a fallback to the existing url-equality check for legacy comments (and S3/CloudFront markdown that points straight at the CDN). * `contentReferencesAttachment` covers both URL shapes for the composer / standalone-list dedup paths so an attachment uploaded before the fix and one uploaded after both deduplicate cleanly. Tests: - New unit tests for the URL helpers (16 tests, packages/core). - Backend regression test: bare `<img src>`-style request without workspace headers now succeeds for a member (200) and 404s for a non-member, replacing the previous "400 without workspace context" contract. - Existing TestDownload*, TestServeLocalUpload*, TestAttachmentTo Response* and the 1220 frontend views tests all pass. Refs: MUL-3130, GitHub issue #3891 Co-authored-by: multica-agent <github@multica.ai> * MUL-3130: address PR review — split markdown link from upload link, swap render src Two follow-ups from GPT-Boy's review on PR #3937. (1) Don't reroute every upload consumer through the workspace-gated download endpoint. The previous change made `useFileUpload`'s `link` field unconditionally return `/api/attachments/<id>/download` whenever the upload had an id. But `useFileUpload` is also used by avatar / logo pickers (account-tab, workspace-tab, agents/avatar-picker, squads/squad-detail-page) that persist `result.link` directly into `avatar_url`. Avatars are referenced cross-workspace (mention chips, member lists, inbox items), so binding their URL to a workspace-membership-gated endpoint would silently break cross-workspace avatar visibility. The fix splits the URL into two semantically distinct fields: - `link` — same as `att.url` (legacy contract). Avatar / logo callers continue to use this and remain on whatever URL semantics the storage backend dictates. - `markdownLink` — the stable per-attachment URL `/api/attachments/<id>/download`. Only the editor's markdown-persisting flow consumes this. Falls back to `link` for the no-workspace upload branch (where there is no attachment-row id to address). `editor/extensions/file-upload.ts` switches `image.src` and `fileCard.href` to `markdownLink ?? link` so comment markdown gets the stable shape while avatar callers stay on `link` unchanged. (2) Make the render-time img src loadable for token-mode clients. Persisting the stable `/api/attachments/<id>/download` URL fixes the expiry problem but the path itself sits behind `middleware.Auth`, which expects either a `multica_auth` cookie or a Bearer token in `Authorization`. Native `<img>`/`<video>` resource loads from token-mode clients (Electron's default mode, the mobile app, legacy-token web sessions) cannot attach the Authorization header, so the bare URL would 401 immediately rather than 30 minutes later. `Attachment.normalize` now runs the resolved record through a new `pickInlineMediaURL` helper that returns: - `record.download_url` when it's an absolute URL with a recognised CDN signature query (CloudFront-signed `Signature` / `Expires` / `Key-Pair-Id`, or `X-Amz-Signature` for raw S3 presigns) — these load as native resource src in any client. - else `record.url`, which on the LocalStorage backend carries a freshly-minted `/uploads/<key>?exp&sig` query whose signature IS the auth (token-mode-loadable). On non-CF S3 backends this is the raw stored URL — same behaviour as today. - else the original input URL (legacy / unresolved markdown keeps its existing path). This gives the same effect for both `kind: "record"` and `kind: "url"` attachment inputs: once a record is in hand, the rendered media src is whichever URL the current backend exposes a working signature on. Tests: - New `file-upload.test.ts` regression pinning that `markdownLink` is what lands in the markdown body when the upload result returns both a short-lived storage URL and a stable download path. - Updated `attachment.test.tsx` to reflect the new render-time swap (the rendered img src now follows the freshly signed URL, not the raw storage URL) and added a record-mode regression pinning the LocalStorage default — when `download_url` is the bare /api/attachments/<id>/download path, the renderer must fall through to the signed `record.url`. - Updated `chat-input.test.tsx` makeUpload helper for the new `markdownLink` UploadResult field. - 1222 frontend views tests + 507 core tests + typecheck across @multica/{core,ui,views} all pass. Refs: MUL-3130, GitHub issue #3891. Builds on |
||
|
Naiyuan Qing
|
569b43136c |
fix(editor): download attachments without blank web tab (#3752)
* fix(editor): download attachments without blank web tab Co-authored-by: multica-agent <github@multica.ai> * fix(attachments): preserve workspace in web download URLs Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: multica-agent <github@multica.ai> |
||
|
Multica Eve
|
ae27058b0a |
fix(attachments): unified download endpoint with mode + presign + proxy (MUL-2976) (#3747)
Fix attachment download for self-hosted deployments using private S3-compatible buckets without CloudFront. Closes #3721. **Server** - New unified `GET /api/attachments/{id}/download` endpoint that picks CloudFront / S3 presign / server proxy at request time. - `ATTACHMENT_DOWNLOAD_MODE=auto|cloudfront|presign|proxy` and `ATTACHMENT_DOWNLOAD_URL_TTL` env knobs; `auto` routes Docker hostnames / localhost / private IPs through the proxy and public S3 endpoints through presign. - `Storage.PresignGet` capability; S3 implementation generates presigned GET URLs. - `attachmentToResponse` returns the unified relative endpoint instead of leaking raw unsigned S3 URLs when CloudFront is not configured. Proxy path streams via `io.Copy` with `Content-Disposition` / `Content-Length` / `Cache-Control: no-store` / `X-Content-Type-Options: nosniff`. **Clients** - CLI / Desktop / Mobile resolve relative `download_url` values against the configured API base. Desktop covers the Electron native download bridge and the media preview modal; Mobile covers `Linking.openURL`, the markdown image RN loader, and the composer's completed non-image file chip. - Mobile gains a minimal Node-environment vitest lane wired into `mobile-verify.yml`. **Docs** - `.env.example`, `docker-compose.selfhost.yml`, `SELF_HOSTING_ADVANCED.md`, and the `environment-variables` doc set updated with the new env keys and the `ATTACHMENT_DOWNLOAD_MODE=proxy` recommendation for Docker / VPC-internal object stores. **Tests** - `internal/storage`, `internal/cli`, `internal/handler` (download endpoint, mode selection, proxy header, `/content` non-regression), `cmd/server` (trusted proxy parser). - `packages/views/editor/use-download-attachment.test.tsx` and `attachment-preview-modal.test.tsx` exercise relative URL resolution + absolute pass-through. - `apps/mobile/lib/attachment-url.test.ts` covers every helper branch plus the composer non-image chip case. |
||
|
Naiyuan Qing
|
454c8e3d1a |
feat: in-app preview for non-image attachments (#2528)
* feat(storage): add GetReader to Storage interface Adds a streaming read method to the Storage abstraction so callers can pull object bytes without forcing a full in-memory load. S3Storage wraps GetObject; LocalStorage opens the file with path-traversal and sidecar guards. Tests cover happy path, traversal rejection, sidecar rejection, and missing key. Used in the next commit by the attachment-preview proxy endpoint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(server): add attachment preview proxy endpoint GET /api/attachments/{id}/content streams the raw bytes of a text-previewable attachment back to the client. Exists to (a) bypass CloudFront CORS, which is not configured on the CDN, and (b) bypass Content-Disposition: attachment which Chromium honors for iframe document loads. Media types (image/video/audio/pdf) intentionally do NOT go through this endpoint — clients render them directly from the signed CloudFront download_url, which is already served with Content-Disposition: inline. Hard cap: 2 MB. Larger files return 413. Anything outside the text whitelist returns 415. The whitelist (isTextPreviewable) mirrors the client-side dispatcher; the cross-reference comment in file.go flags the manual sync until a JSON SSOT generator lands. Response always uses Content-Type: text/plain; charset=utf-8 so a hostile HTML payload can't be re-interpreted as a document. The original MIME ships via X-Original-Content-Type for client dispatch. Cache-Control: no-store so revoked attachment access takes effect immediately on the next request. Tests cover happy path (md), extension fallback when content_type is generic, 415 (pdf), 413 (>2MB), foreign workspace (404 isolation), and the isTextPreviewable table. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(core/api): add getAttachmentTextContent + preview error types Adds an ApiClient method that fetches the text body of an attachment via the new /api/attachments/{id}/content proxy. Two typed errors — PreviewTooLargeError (413) and PreviewUnsupportedError (415) — let the preview modal render specific fallbacks instead of a generic failure. Refactors the private fetch() into a shared fetchRaw() helper so the new method inherits the standard infra: auth headers, 401 → handleUnauthorized recovery, X-Request-ID, error logging, and the ApiError contract. The previous draft bypassed all of these by calling window.fetch directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(views/editor): add AttachmentPreviewModal + Eye entry points In-app preview for non-image attachments. An Eye icon now sits next to the existing Download button on file cards / readonly file cards / the standalone AttachmentList. Clicking it opens a full-screen modal that dispatches by content_type: pdf: <iframe src={download_url}> — Chromium PDFium video/*: <video controls src={download_url}> — native controls audio/*: <audio controls src={download_url}> — native controls md: <ReadonlyContent> — full markdown pipeline html: <iframe srcdoc sandbox=""> — fully restricted text: <code class="hljs"> — lowlight highlight Media types render directly from the signed CloudFront download_url (server marks them inline-disposition). Text types fetch through the new /api/attachments/{id}/content proxy via TanStack Query, wrapped in useAttachmentPreview() so each entry point owns its own modal state without depending on a global Provider mount. Modal sizing: max-w-6xl × min(90vh, 100vh - 2rem) — slightly larger than create-issue's max-w-4xl since PDF / video need room, but capped to viewport on small screens. Sub-renderers use h-full to follow the fixed modal height instead of viewport-relative units. Images are intentionally NOT touched — the existing ImageLightbox (extensions/image-view.tsx) already handles them correctly. The new modal would be churn without user-visible benefit. Adds i18n keys under attachment.* (en + zh-Hans) and registers Preview/Download/Upload in the conventions glossary so future translations stay consistent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(desktop): enable Chromium PDF viewer for attachment preview Adds webPreferences.plugins: true to the main BrowserWindow so the bundled Chromium PDFium plugin activates inside iframes — required for the attachment preview modal's PDF dispatch. Default is false in Electron; without it <iframe src=*.pdf> renders blank. Security trade-off, accepted intentionally and documented inline: 1. This window already runs with webSecurity: false + sandbox: false, so plugins: true does NOT meaningfully widen the renderer's attack surface beyond what is already accepted. 2. The only PDFs that reach an iframe here are signed CloudFront URLs we ourselves issued; user-supplied URLs are routed through setWindowOpenHandler → openExternalSafely and cannot land in this renderer. 3. Chromium's PDFium plugin is itself sandboxed and only handles application/pdf — no Flash/Java/other historical plugin surfaces. If we ever tighten webSecurity / sandbox, the follow-up is to host the PDF viewer in a dedicated BrowserView with plugins scoped to that view, keeping the main renderer plugin-free. Old desktop builds ship without the preview modal, so the Eye button never appears and PDF preview is gated by the same release — zero regression risk for users on stale clients. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Naiyuan Qing
|
86aa5199fc |
feat(chat): support attachments & images in chat input (#2445)
* docs(plans): chat attachment & image support implementation plan Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * feat(db): add chat_session_id/chat_message_id to attachment Co-authored-by: multica-agent <github@multica.ai> * feat(db): sqlc — chat_session_id on CreateAttachment + LinkAttachmentsToChatMessage Co-authored-by: multica-agent <github@multica.ai> * feat(file): upload-file accepts chat_session_id form field Co-authored-by: multica-agent <github@multica.ai> * feat(chat): SendChatMessage links uploaded attachments to the new message Co-authored-by: multica-agent <github@multica.ai> * feat(api): uploadFile accepts chatSessionId; sendChatMessage accepts attachmentIds Co-authored-by: multica-agent <github@multica.ai> * feat(core): useFileUpload supports chatSessionId context Co-authored-by: multica-agent <github@multica.ai> * feat(chat): support paste/drag/upload attachments in chat input Co-authored-by: multica-agent <github@multica.ai> * test(e2e): chat input attachment upload + send round-trip Co-authored-by: multica-agent <github@multica.ai> * chore(chat): keep lazy-created session title empty so untitled fallback localizes Co-authored-by: multica-agent <github@multica.ai> * fix(chat): address review — dedupe ensureSession + parse upload response - chat-window: cache in-flight createSession promise in a ref so a file drop followed by a quick send no longer spawns two sessions (and orphans the attachment on the losing one). - Attachment type + EMPTY_ATTACHMENT + AttachmentResponseSchema: include the new chat_session_id / chat_message_id fields the server now returns. - uploadFile: route the response through parseWithFallback so a malformed body returns EMPTY_ATTACHMENT instead of an undefined-keyed Attachment, matching the API boundary rule. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * fix(chat): address PR #2445 review — test ctx, send gating, attachment surface 1. Backend test was 400ing because the handler reads workspace from middleware-injected ctx, and `newRequest` only sets the header. Helper `withChatTestWorkspaceCtx` mirrors the agent-access-test pattern and loads the member row + SetMemberContext before invoking the handler. 2. Attachment metadata now flows end-to-end: - new sqlc `ListAttachmentsByChatMessageIDs` (batch lookup, mirrors the comment-side query) - `chatMessageToResponse` takes `attachments` and `ChatMessageResponse` surfaces them — same shape as CommentResponse - `ListChatMessages` loads them via a new `groupChatMessageAttachments` helper so the chat bubble can render file cards - daemon claim path pulls `ListAttachmentsByChatMessage` for the latest user message and ships `ChatMessageAttachments` to the daemon - `buildChatPrompt` lists id+filename+content_type and instructs the agent to `multica attachment download <id>` — fixes the private-CDN expiring-URL problem where the markdown URL would have expired by the time the agent acts - TS `ChatMessage` gains an optional `attachments` field 3. Chat composer now blocks send while uploads are in flight: - `pendingUploads` counter increments in handleUpload, SubmitButton uses it to disable - handleSend also gates on `editorRef.current.hasActiveUploads()` to catch the Mod+Enter path that bypasses the button - new vitest covers the "drop large file → immediate send" scenario where attachment id would otherwise be silently dropped Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> * chore: drop implementation plan doc Process artefact, not something the repo needs to keep. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai> |
||
|
Naiyuan Qing
|
f0f3cb5c3a |
fix(server): resolve X-Workspace-Slug in middleware-less handlers (#1165)
Problem ------- The v2 workspace URL refactor (#1141) switched the frontend from sending X-Workspace-ID (UUID) to X-Workspace-Slug. The workspace middleware was updated to accept the slug and translate it via GetWorkspaceBySlug. But the handler package maintained a PARALLEL resolver (`resolveWorkspaceID` in handler.go) used by endpoints that sit outside the workspace middleware — and that resolver was never updated. It only checked context / ?workspace_id / X-Workspace-ID, never the slug. /api/upload-file is the one production route that hit the broken path: it's user-scoped (not behind workspace middleware) because it also serves avatar uploads (no workspace). Post-refactor requests from the frontend arrived with only X-Workspace-Slug; the handler resolver returned "", the code fell into the "no workspace context" branch, and every file upload since v2 landed in S3 with no corresponding DB attachment row — files orphaned, invisible to the UI. Root cause is structural: two resolvers doing the same job, written independently, diverged silently when one was updated. Fix --- Collapse to a single shared helper. middleware.ResolveWorkspaceIDFromRequest is the new canonical resolver; both the middleware's internal `resolveWorkspaceUUID` (for middleware gating) and the handler-side `(h *Handler).resolveWorkspaceID` (promoted from a package function) now delegate to it. Priority order matches what the middleware has had since v2: context > X-Workspace-Slug header > ?workspace_slug query > X-Workspace-ID header > ?workspace_id query. Impact analysis --------------- 47 call sites of the old `resolveWorkspaceID(r)` are renamed to `h.resolveWorkspaceID(r)`. 46 of them sit behind workspace middleware, so they hit the context fast path and see zero behavior change. The one caller that actually gains capability is UploadFile — which now correctly recognizes slug requests and creates DB attachment rows. Tests ----- - New table-driven unit test for ResolveWorkspaceIDFromRequest covers all priority levels and the unknown-slug fallback. - Regression tests for UploadFile: once with X-Workspace-Slug only (the broken path), once with X-Workspace-ID only (legacy CLI/daemon compat path). Both assert that a DB attachment row is created. - Full Go test suite passes; typecheck + pnpm test unaffected. Plan ---- See docs/plans/2026-04-16-unify-workspace-identity-resolver.md for the full first-principles writeup. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
Naiyuan Qing
|
53cb01cc91 |
refactor(editor): remove hardcoded CDN domain, unify file card rendering
- Add GET /api/config endpoint exposing cdn_domain from CLOUDFRONT_DOMAIN - Create packages/core/config/ zustand store, fetched at app startup - Extract file card preprocessing to packages/ui/markdown/file-cards.ts with isCdnUrl(url, cdnDomain) using exact hostname match - Add file card support to packages/ui/markdown/Markdown.tsx (was missing) - Remove hardcoded .copilothub.ai hostname check from file-card.tsx - Fix LocalStorage.CdnDomain() to return hostname not full URL - Always run preprocessFileCards regardless of cdnDomain availability (!file syntax works without CDN domain, only legacy matching needs it) - Use useConfigStore hook in common/markdown.tsx for reactive updates Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |
||
|
devv-eve
|
8c0708bb5d |
fix(server): validate workspace membership for subscriptions and uploads (#935)
* fix(server): validate workspace membership for subscription targets and file uploads Closes MED-1 (cross-workspace subscription injection) and MED-2 (file upload missing workspace member validation) from the security audit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(server): add negative tests for cross-workspace subscription and upload Address PR review feedback: - Add tests verifying cross-workspace user_id is rejected with 403 on subscribe and unsubscribe - Add test verifying upload with foreign workspace_id is rejected with 403 - Make isWorkspaceEntity explicitly enumerate "member"/"agent" and reject unknown user types Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Devv <devv@Devvs-Mac-mini.local> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> |