Skip to content

fix(host-transfer): remove auto-resolve for single clients to restore backward compat#29462

Closed
credence-the-bot[bot] wants to merge 2 commits into
mainfrom
credence/fix-transfer-auto-resolve
Closed

fix(host-transfer): remove auto-resolve for single clients to restore backward compat#29462
credence-the-bot[bot] wants to merge 2 commits into
mainfrom
credence/fix-transfer-auto-resolve

Conversation

@credence-the-bot
Copy link
Copy Markdown
Contributor

@credence-the-bot credence-the-bot Bot commented May 4, 2026

Problem

Phase 3 (PR #29440) added auto-resolution of targetClientId when exactly one host_file-capable client is connected. This makes the content routes require the x-vellum-client-id header on all GET/PUT transfer requests, even for untargeted calls.

Pre-Phase 3 macOS clients (0.7.1-staging.5; both Vellum.app and Vellum Staging.app still at May 1 builds) do not send this header. Result: the content GET returns 400, the Swift client throws rather than calling post-transfer-result, and the pending interaction times out after 120 seconds — every time, consistently.

Diagnosis

From feedback bundle feedback-019df0a7-99a9-7446-8bdf-6228ec86ad08:

  • Both /Applications/Vellum.app and /Applications/Vellum Staging.app have vellum-daemon binaries timestamped May 1 (pre-Phase 3)
  • The cloud sandbox daemon is Phase 3 (latest main)
  • Phase 3 daemon: if (capable.length === 1) resolvedTargetClientId = capable[0].clientId → makes x-vellum-client-id mandatory on content routes
  • Pre-Phase 3 Swift HostProxyClient does not send X-Vellum-Client-Id → 400 on GET /v1/transfers/:transferId/contentTransferError.pullFailed(400) thrown → host-transfer-result never posted → 120 s hang

Fix

Remove the auto-resolve block from both requestToHost() and requestToSandbox(). Untargeted transfers broadcast without targetClientId — backward-compatible with all existing macOS clients. Explicit target_client_id from the tool input still routes correctly (Phase 3 cross-client targeting feature intact).

Changes

  • assistant/src/daemon/host-transfer-proxy.ts — remove auto-resolve else blocks from both methods
  • assistant/src/__tests__/host-transfer-proxy-targeted.test.ts — flip the two auto-resolve test cases to assert targetClientId is undefined for untargeted single-client calls

All 47 proxy tests pass.


Open in Devin Review

credence-the-bot added 2 commits May 4, 2026 00:34
…hen disabled

- Add 'analyze-conversation' assistant-scope flag to feature-flag-registry.json
  (defaultEnabled: false)
- ConversationTitleActionsControl + ConversationActionsMenuContent: make
  onAnalyzeConversation optional ((() -> Void)? = nil); gate VMenuItem on
  presence of non-nil closure
- ConversationTitleOverlay: make onAnalyzeConversation optional to propagate
  nil cleanly through the chain
- TopBarView: inject AssistantFeatureFlagStore via @Environment; pass nil for
  onAnalyzeConversation when flag is off
- SidebarView: gate onAnalyze on assistantFeatureFlagStore.isEnabled("analyze-conversation")
- ConversationSwitcherDrawer: inject AssistantFeatureFlagStore via @Environment;
  gate onAnalyze the same way
… backward compat

Phase 3 (PR #29440) added auto-resolution of targetClientId when exactly one
host_file-capable client is connected. This causes the content routes to require
the x-vellum-client-id header on all GET/PUT transfer requests, even for
untargeted calls.

Pre-Phase 3 macOS clients (e.g. 0.7.1-staging.5, both Vellum.app and Vellum
Staging.app still at May 1 builds) do not send this header. Result: the content
GET returns 400, the Swift client throws rather than calling post-transfer-result,
and the pending interaction times out after 120 seconds.

Fix: remove the auto-resolve block from both requestToHost() and requestToSandbox().
Untargeted transfers broadcast without targetClientId — backward-compatible with all
existing macOS clients. Explicit target_client_id from tool input still routes
correctly (Phase 3 feature intact).

Tests updated: auto-resolve assertions flipped to expect undefined targetClientId
for untargeted single-client calls.
@credence-the-bot
Copy link
Copy Markdown
Contributor Author

@codex please review

@devin please review

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

@credence-the-bot credence-the-bot Bot closed this May 4, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3284903916

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

"key": "analyze-conversation",
"label": "Analyze Conversation",
"description": "Show the 'Analyze' / 'Analyze conversation' option in conversation context menus and the conversation title actions dropdown.",
"defaultEnabled": false
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep analyze-conversation enabled by default

This change gates every manual Analyze entry point behind analyze-conversation, but the newly introduced registry default is false, so existing users lose the Analyze action immediately after upgrade unless a per-assistant override is already present. Because this commit does not add any bootstrap/migration path that seeds the flag to true, it introduces a user-facing behavior regression unrelated to the host-transfer fix.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants