Skip to content

feat: add platform terraform for GKE deployment#2

Merged
dvargasfuertes merged 1 commit into
mainfrom
feat/platform-terraform
Feb 7, 2026
Merged

feat: add platform terraform for GKE deployment#2
dvargasfuertes merged 1 commit into
mainfrom
feat/platform-terraform

Conversation

@vargas-jr
Copy link
Copy Markdown

@vargas-jr vargas-jr Bot commented Feb 7, 2026

Summary

Terraform configuration for deploying Vellum Assistant to GKE with assistant.vellum.ai domain.

Architecture

assistant.vellum.ai
       ↓
Cloud Load Balancer (Static IP + SSL)
       ↓
GKE Ingress
       ↓
Next.js Pods (autoscaling)

Added

/platform/terraform/

  • main.tf - GCP provider, GKE cluster (optional), static IP, managed SSL cert
  • k8s.tf - Kubernetes deployment, service, ingress
  • variables.tf - Configurable inputs
  • outputs.tf - IP address, DNS instructions
  • terraform.tfvars.example - Example configuration

/web/

  • Dockerfile - Standalone Next.js build
  • .dockerignore
  • src/app/api/health/route.ts - Health check endpoint

Features

  • Flexible: Use existing GKE cluster or create new one
  • Production-ready: Health checks, resource limits, autoscaling
  • Secure: Managed SSL, secrets via TF vars
  • Simple DNS: Just point A record to static IP output

Usage

cd platform/terraform
cp terraform.tfvars.example terraform.tfvars
# Edit terraform.tfvars
export TF_VAR_database_url="postgresql://..."
export TF_VAR_anthropic_api_key="sk-ant-..."
terraform init && terraform apply

Open with Devin

- Terraform for GKE cluster (optional) + K8s resources
- Static IP + managed SSL certificate for assistant.vellum.ai
- Deployment, Service, Ingress configuration
- Dockerfile for Next.js standalone build
- Health check endpoint (/api/health)
- Supports existing cluster or creating new one
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 found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment thread web/next.config.ts
outputFileTracingIncludes: {
"/api/agents": ["./agent-templates/**/*"],
},
output: "standalone",
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.

🔴 Removal of outputFileTracingIncludes breaks runtime file access in standalone build

The PR replaces the entire next.config.ts content, removing outputFileTracingIncludes which was required to include agent-templates/**/* in the Next.js output trace. With output: "standalone", Next.js uses file tracing to determine which files to bundle into the standalone output. Since agent-templates files are read dynamically at runtime via path.join(process.cwd(), "agent-templates", agentType) (web/src/lib/gcp.ts:132, web/src/lib/gcp.ts:211), the file tracer cannot detect these dependencies.

Root Cause and Impact

The original config had:

outputFileTracingIncludes: {
  "/api/agents": ["./agent-templates/**/*"],
}

This explicitly told Next.js to include the agent-templates directory in the standalone output for the /api/agents route. The new config only has output: "standalone" with no trace includes.

In web/src/lib/gcp.ts, multiple functions read from agent-templates at runtime:

  • generateAgentFiles() at line 132-133: const templateDir = path.join(process.cwd(), "agent-templates", agentType); const filePaths = readDirectoryRecursive(templateDir);
  • uploadAgentConfigToGCS() at line 211: const templateDir = path.join(process.cwd(), "agent-templates", agentType);

Without outputFileTracingIncludes, the standalone build will not include these template files, causing ENOENT errors at runtime when any agent creation or configuration API is called. The output: "standalone" setting should be kept (it's needed for the Docker build), but outputFileTracingIncludes should also be preserved.

Impact: All agent creation and configuration endpoints will fail with file-not-found errors in the Docker/standalone deployment.

Suggested change
output: "standalone",
output: "standalone",
outputFileTracingIncludes: {
"/api/agents": ["./agent-templates/**/*"],
},
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

ashleeradka added a commit that referenced this pull request Feb 14, 2026
…ound

Issue #1 (Consistency):
- Add #else clause to signing operation switch cases in handleServerMessage
- Ensures all platforms have explicit handling (logs error on unsupported platforms)
- Complements the #error directive in connect() for defense in depth

Issue #3 (Type inference fragility):
- Replace .init shorthand with fully qualified ConfirmationAllowlistOption type
- Consistent with other preview blocks in ToolConfirmationBubble.swift
- Prevents breakage if type inference context changes

Issue #7 (Build robustness):
- Remove empty Resources directory and .process("Resources") declaration
- Eliminates fragile .gitkeep workaround (accidental deletion would break builds)
- VellumAssistantShared has no resources yet; can add back when needed

Issues #2, #4, #5, #6 (Not actionable in this PR):
- #2: VellumAssistantLib platform restriction documented via comment (SPM limitation)
- #4: @mainactor deinit data race is pre-existing, not introduced by this PR
- #5: iOS signing hang acknowledged, needs daemon-side fix
- #6: JSON camelCase/snake_case mixed convention is existing protocol, not changing

✅ Build: 5.83s
✅ More robust against platform/type inference edge cases
✅ Removed unnecessary .gitkeep workaround

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ashleeradka added a commit that referenced this pull request Feb 14, 2026
Issue #1 (🟡 Medium): Build script clean commands broken
- .build directory moved from clients/macos/.build to clients/.build
- build.sh clean and release pre-clean still referenced old location
- Result: Clean commands did nothing, stale artifacts persisted

Fix: Update both clean commands to use $SCRIPT_DIR/../.build
- Line 64: Clean command now removes correct directory
- Line 83: Release pre-clean now removes correct directory

Issue #2 (🔴 High): MainActor.assumeIsolated crash risk in deinit
- Previous fix (9231d63) used assumeIsolated which crashes if deinit
  runs on background thread
- While assumeIsolated reveals lifecycle bugs, it's too aggressive for
  cleanup code that should never crash

Fix: Revert to direct access pattern with detailed justification
- Task.cancel(), NWConnection.cancel(), Continuation.finish() are all thread-safe
- Data races on shouldReconnect and subscribers are benign in deinit
  (object is being destroyed, no other access possible)
- Added comprehensive comment explaining the tradeoff

Tradeoff analysis:
✅ No crashes during deallocation
✅ Clean command actually works now
✅ Release builds get proper artifact cleanup
⚠️  Technically a data race, but benign for cleanup-only code
⚠️  Won't trap on incorrect lifecycle (silent vs. loud failure)

Devin review feedback addressed (both comments from 07:40-07:45)

✅ Build: 1.22s

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ashleeradka added a commit that referenced this pull request Feb 14, 2026
…1821)

* Create VellumAssistantShared library target for code reuse

- Add multi-platform library target to Package.swift (macOS .v14, iOS .v17)
- Move IPC layer (DaemonClient, IPCMessages) to shared library
- Add platform-specific connection logic to DaemonClient:
  - macOS: Unix domain socket at ~/.vellum/vellum.sock
  - iOS: TCP endpoint (configured via UserDefaults)
- Make all IPC types public with necessary initializers
- Update macOS app to import VellumAssistantShared in all files using IPC
- Add public initializers for Decodable messages used in tests

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Restructure directory layout for multi-platform support

Move to flat structure at clients/ level:
- Move Package.swift from clients/macos/ to clients/
- Move vellum-assistant-shared/ to clients/shared/
- Add .gitignore at clients/ level to exclude .build/
- Update Package.swift paths to reference shared/ and macos/

This prepares for iOS app in clients/ios/ without nesting
everything under an Apple-specific directory.

Final structure:
  clients/
    Package.swift        (Swift package for all Apple platforms)
    .gitignore           (ignore .build, .swiftpm, etc)
    shared/              (VellumAssistantShared target)
    macos/               (macOS app)
    ios/                 (future iOS app)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Address PR feedback: fix Resources directory and port overflow

Fixes based on Devin and Codex review comments:

1. Create shared/Resources directory with .gitkeep
   - Package.swift declares resources: [.process("Resources")]
   - Git doesn't track empty directories, causing fresh clone failures
   - Added .gitkeep to ensure directory is tracked

2. Guard iOS daemon port conversion from UserDefaults overflow
   - Previous: Direct cast Int -> UInt16 can crash if value >65535 or negative
   - Fixed: Use UInt16(clamping:) with validation (0-65535 range)
   - Falls back to 8765 for invalid values
   - Prevents runtime crash from malformed persisted settings

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Add missing public initializers and properties

- Add public init to TaskSubmitMessage
- Make SessionItem and HistoryMessageItem properties public
- Make HistoryToolCallItem properties public

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix access control and iOS compatibility issues in shared IPC layer

Address critical compilation issues and improve iOS platform handling:

🔴 Critical fixes:
- Make SkillSummaryItem typealias public for cross-module access
- Make UpdateInfo properties (name, installedVersion, latestVersion) public

🟡 Medium severity fixes:
- Add public initializers to all Encodable IPC message structs:
  • PingMessage, SkillsEnableMessage
  • ConfirmationResponseMessage, SecretResponseMessage
  • AddTrustRuleMessage, TrustRulesListMessage
  • RemoveTrustRuleMessage, UpdateTrustRuleMessage
- Add explicit error logging on iOS for unsupported signing operations
  (signBundlePayload, getSigningIdentity) instead of silently dropping

These changes ensure the shared library has consistent public API surface
for both macOS and iOS targets, and makes iOS behavior more transparent.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Complete public API consistency and add platform safety guards

Address remaining low-severity issues for production-ready shared library:

🔧 API Completeness (Issue #1):
- Add public initializers to 17 remaining Encodable structs:
  • Skills: SkillsDisableMessage, SkillsConfigureMessage, SkillsInstallMessage,
    SkillsUninstallMessage, SkillsUpdateMessage, SkillsCheckUpdatesMessage,
    SkillsSearchMessage, SkillsInspectMessage
  • Apps: AppsListRequestMessage, SharedAppsListRequestMessage,
    SharedAppDeleteRequestMessage, BundleAppRequestMessage, OpenBundleMessage
  • Session: SessionListRequestMessage, HistoryRequestMessage
  • Signing: SignBundlePayloadResponseMessage, GetSigningIdentityResponseMessage
- Ensures consistent public API surface for future iOS code

🛡️ Platform Safety (Issue #4):
- Add #else fallback in DaemonClient.connect() with #error directive
- Prevents silent compilation failures on unsupported platforms (visionOS, watchOS)
- Clear compile-time error: "DaemonClient is only supported on macOS and iOS"

📝 Documentation (Issue #5):
- Update DaemonClient doc comment to describe both connection types:
  • macOS: Unix domain socket at ~/.vellum/vellum.sock
  • iOS: TCP to configurable hostname:port via UserDefaults
- Accurately reflects shared library's cross-platform design

✅ Build: passes in 3.70s
✅ All structs now have consistent public init coverage
✅ Future-proof against unsupported platform builds

Note: iOS signing request handling (Issue #3) remains logged-only since
response messages lack error fields. Proper fix requires daemon-side changes
to avoid sending these requests to iOS clients.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix SendError visibility and clarify VellumAssistantLib platform restriction

Issue #5 (Low severity):
- Make SendError enum public for cross-module error handling
- Make errorDescription public for LocalizedError protocol conformance
- Enables consumers to pattern-match on specific error cases (e.g., .notConnected)

Issue #4 (Low severity - documentation):
- Add comment clarifying VellumAssistantLib is macOS-only
- Links macOS-specific frameworks (AppKit, ScreenCaptureKit, etc.)
- iOS apps should depend only on VellumAssistantShared

Note: Issue #1 (build script) is NOT a problem - SPM automatically searches
parent directories for Package.swift. Build script works correctly as-is.

✅ Build: 4.65s

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix platform coverage consistency and remove fragile Resources workaround

Issue #1 (Consistency):
- Add #else clause to signing operation switch cases in handleServerMessage
- Ensures all platforms have explicit handling (logs error on unsupported platforms)
- Complements the #error directive in connect() for defense in depth

Issue #3 (Type inference fragility):
- Replace .init shorthand with fully qualified ConfirmationAllowlistOption type
- Consistent with other preview blocks in ToolConfirmationBubble.swift
- Prevents breakage if type inference context changes

Issue #7 (Build robustness):
- Remove empty Resources directory and .process("Resources") declaration
- Eliminates fragile .gitkeep workaround (accidental deletion would break builds)
- VellumAssistantShared has no resources yet; can add back when needed

Issues #2, #4, #5, #6 (Not actionable in this PR):
- #2: VellumAssistantLib platform restriction documented via comment (SPM limitation)
- #4: @mainactor deinit data race is pre-existing, not introduced by this PR
- #5: iOS signing hang acknowledged, needs daemon-side fix
- #6: JSON camelCase/snake_case mixed convention is existing protocol, not changing

✅ Build: 5.83s
✅ More robust against platform/type inference edge cases
✅ Removed unnecessary .gitkeep workaround

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix @mainactor deinit data race in DaemonClient

Issue: Swift concurrency bug (pre-existing, but now fixed)

In Swift 5.9+, deinit on a @mainactor class is NOT guaranteed to run on
the main actor. When the last reference to DaemonClient is dropped from
a background thread, deinit could run on that thread, causing data races
when accessing main-actor-isolated properties (shouldReconnect, connection,
subscribers, etc.).

Solution: Wrap deinit body in MainActor.assumeIsolated { }

This ensures:
- Safe access to main-actor-isolated state during cleanup
- Trap with clear error if deinit somehow runs on wrong thread
- Better than silent undefined behavior from data race

Why fix in this PR:
- Already heavily modifying DaemonClient for shared library
- Real correctness issue (undefined behavior, potential crashes)
- Simple fix with clear semantics
- Makes shared library more robust for both macOS and iOS

Reference: SE-0327 (Actors and Deinitializers)

✅ Build: 1.27s
✅ Eliminates undefined behavior from concurrent deinit

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix build script clean paths and revise deinit approach

Issue #1 (🟡 Medium): Build script clean commands broken
- .build directory moved from clients/macos/.build to clients/.build
- build.sh clean and release pre-clean still referenced old location
- Result: Clean commands did nothing, stale artifacts persisted

Fix: Update both clean commands to use $SCRIPT_DIR/../.build
- Line 64: Clean command now removes correct directory
- Line 83: Release pre-clean now removes correct directory

Issue #2 (🔴 High): MainActor.assumeIsolated crash risk in deinit
- Previous fix (9231d63) used assumeIsolated which crashes if deinit
  runs on background thread
- While assumeIsolated reveals lifecycle bugs, it's too aggressive for
  cleanup code that should never crash

Fix: Revert to direct access pattern with detailed justification
- Task.cancel(), NWConnection.cancel(), Continuation.finish() are all thread-safe
- Data races on shouldReconnect and subscribers are benign in deinit
  (object is being destroyed, no other access possible)
- Added comprehensive comment explaining the tradeoff

Tradeoff analysis:
✅ No crashes during deallocation
✅ Clean command actually works now
✅ Release builds get proper artifact cleanup
⚠️  Technically a data race, but benign for cleanup-only code
⚠️  Won't trap on incorrect lifecycle (silent vs. loud failure)

Devin review feedback addressed (both comments from 07:40-07:45)

✅ Build: 1.22s

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Add explicit Network framework linking to VellumAssistantShared

Issue: VellumAssistantShared imports Network (DaemonClient uses NWConnection)
but doesn't explicitly link the Network framework. Currently works because
VellumAssistantLib links Network, but future iOS targets depending only on
VellumAssistantShared would fail.

Fix: Add linkerSettings with Network framework to VellumAssistantShared target

While SPM usually auto-links system frameworks on import, explicit linking
ensures robust behavior when iOS app target depends only on the shared library
(not on VellumAssistantLib which is macOS-only).

✅ Build: 16.92s (full rebuild)
✅ Explicit framework dependencies documented
✅ Future-proof for iOS app target

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Add clients/ directory README for code organization documentation

Created comprehensive README.md at clients/ level to document:
- Directory structure and target organization
- VellumAssistantShared vs VellumAssistantLib vs executable
- Platform-specific vs shared code strategy
- Build instructions and development guidelines
- Migration context from single-platform structure
- Known limitations (iOS TCP, signing, localhost)

Helps developers and other agents understand:
- Why Package.swift is at clients/ level
- What code goes in shared/ vs macos/ vs ios/
- How to add new shared/platform-specific code
- ~45-50% code reuse strategy

Valuable for PR reviewers and future iOS development (PRs 2-13).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Add imageData field to ToolResultMessage for protocol completeness

The imageData field was added to ToolResultMessage in main (commit ea7c3d7)
for browser screenshot support (PR #1864) but was inadvertently dropped
during the migration to the shared library. This field enables the daemon
to send base64-encoded image data from tool contentBlocks.

While not currently consumed by the macOS client, adding it ensures the
shared library's protocol definition matches the daemon's implementation.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Add missing public initializers to IPC message types

Added public initializers to 7 Decodable message types that are used
in test code. These types need explicit public initializers because
Swift's auto-generated memberwise initializers are internal by default:

- AssistantThinkingDeltaMessage
- CuErrorMessage
- CuCompleteMessage
- CuActionMessage
- GenerationHandoffMessage
- MessageDequeuedMessage
- GenerationCancelledMessage
- ErrorMessage
- MessageQueuedMessage

Without these, tests that construct messages directly (rather than
decoding from JSON) fail with "incorrect argument label" errors.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
ashleeradka added a commit that referenced this pull request Feb 15, 2026
1. Add deleteThread() method for permanent thread deletion
   - Allows users to clean up hidden threads in drawer mode
   - Prevents indefinite accumulation of hidden threads

2. Fix hidden thread row click behavior
   - Removed .tag() from hidden thread rows
   - Only explicit restore button triggers restoration now

3. Optimize duplicate filter computation
   - Extract hiddenThreads to local variable
   - Avoid computing filter twice

4. Add delete button to hidden threads UI
   - Trash icon alongside restore button
   - Provides permanent cleanup option

Fixes issues #2, #3, and #4 from review feedback.
Issue #1 (sessionId data loss) was already fixed in previous commit.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ashleeradka added a commit that referenced this pull request Feb 15, 2026
* Add thread drawer mode as alternative to tab layout

Implements a NavigationSplitView-based drawer for accessing chat history,
with a toggle in Display settings to switch between tab and drawer modes.

## Key Features
- New "Show thread list drawer" toggle in Settings > Display
- Drawer mode shows threads in a collapsible sidebar (NavigationSplitView)
- Thread hiding system: X button hides threads instead of deleting
- Hidden threads shown in collapsible "HIDDEN" section at bottom
- Restore button (↺) to unhide threads in drawer mode
- Consistent button behavior across both modes (icon-only toolbar buttons)

## Implementation Details
- ThreadModel: Added `isHidden` property to track hidden threads
- ThreadManager: Modified `closeThread()` to mark threads as hidden
- ThreadManager: Added `showThread()` to restore hidden threads
- MainWindowView: Conditional layout based on `useThreadDrawer` setting
- ChatView: Reduced top padding in drawer mode for better spacing
- Both modes now filter hidden threads from main list

## Technical Choices
- Reuses existing NavigationSplitView component (no custom drawer)
- Leverages ThreadManager's existing thread state management
- Minimal code duplication between tab and drawer modes
- Uses SwiftUI's native collapsible Section for hidden threads

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix thread drawer review issues

- Sync selectedThreadId back from activeThreadId to keep sidebar selection in sync
- Clean up ChatViewModels when threads are hidden to prevent memory leaks
- Recreate ChatViewModels when hidden threads are restored
- Use selectThread for visible threads instead of always calling showThread

Addresses review feedback from devin-ai-integration.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Address PR review feedback — thread management improvements

1. Add deleteThread() method for permanent thread deletion
   - Allows users to clean up hidden threads in drawer mode
   - Prevents indefinite accumulation of hidden threads

2. Fix hidden thread row click behavior
   - Removed .tag() from hidden thread rows
   - Only explicit restore button triggers restoration now

3. Optimize duplicate filter computation
   - Extract hiddenThreads to local variable
   - Avoid computing filter twice

4. Add delete button to hidden threads UI
   - Trash icon alongside restore button
   - Provides permanent cleanup option

Fixes issues #2, #3, and #4 from review feedback.
Issue #1 (sessionId data loss) was already fixed in previous commit.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix sessionId immutability and improve VSplitView transitions

1. ThreadModel.swift: Change sessionId from let to var
   - Fixes critical data loss bug where sessionId couldn't be persisted
   - Enables chat history restoration when threads are unhidden

2. VSplitView.swift: Improve split-view behavior
   - Change from overlay to side-by-side HStack layout
   - Panel now pushes content aside instead of covering it
   - Smoother spring animation (0.3s response, 0.8 damping)
   - Better padding (vertical + leading only)
   - Add clarifying comments

These changes address the sessionId data loss issue (#1) and
improve right panel UX per user feedback.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Restore original tab mode delete behavior

Devin correctly identified that closeThread() now hides threads in both
modes, but tab mode originally deleted them permanently. This is a
behavioral regression.

Fix: Make closeThread() conditional based on useThreadDrawer setting:
- Tab mode (useThreadDrawer = false): Delete threads permanently (original)
- Drawer mode (useThreadDrawer = true): Hide threads (new feature)

This preserves the original tab behavior while enabling the new drawer
restoration feature only where it's accessible via UI.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Revert to original tab behavior - remove hide/restore feature

Remove all hidden thread functionality to match original behavior:
- Removed isHidden field from ThreadModel
- Removed showThread() and deleteThread() methods from ThreadManager
- Restored original simple closeThread() that removes from memory
- Removed HIDDEN section from drawer mode UI
- Removed isHidden filters from thread lists

Original behavior preserved:
- X button removes thread from memory
- Threads restored on app restart from daemon sessions (up to 5 most recent)
- Both tab and drawer modes now have identical thread lifecycle

This simplifies the implementation and matches the original behavior
exactly. Hide/restore can be added as a separate feature later.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix toolbar flash and address Devin review issues

1. Disable toolbar animations during NavigationSplitView transitions
   - Add .transaction modifier to disable animations
   - Wrap columnVisibility changes in withTransaction
   - Prevents toolbar flash when toggling drawer

2. Hide X button on last thread in drawer mode
   - Conditionally show close button only when threads.count > 1
   - Matches tab mode behavior (ThreadTabBar)
   - Prevents non-functional button clicks

3. Fix VSplitView trailing padding
   - Change from .padding(.leading) to .padding(.horizontal)
   - Ensures right-side rounded corners aren't clipped
   - Provides proper spacing on both sides

Addresses Devin review issues from commit c4cb938.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Enable drawer slide animation while preventing toolbar flash

Changed approach to fix toolbar flash:
- Removed broad .transaction modifier that disabled all animations
- Added .animation(nil, value: columnVisibility) to toolbar HStack only
- NavigationSplitView sidebar now slides in/out smoothly
- Toolbar no longer flashes during sidebar transitions

This gives us the best of both worlds: smooth drawer animation
and stable toolbar positioning.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix root cause of toolbar flash - disable automatic sidebar toggle

The "extra button" flash was caused by NavigationSplitView automatically
adding a native sidebar toggle button to the toolbar. When the sidebar
opened/closed, macOS would reposition/show/hide this button, causing
our toolbar buttons to shift and flash.

Fix: Add .toolbar(removing: .sidebarToggle) to disable the automatic
sidebar toggle button that NavigationSplitView adds by default.

Also removed the .animation(nil) workaround since we're now fixing
the root cause instead of hiding the symptom.

This is the proper fix - no animation suppression needed.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Style drawer mode panels and fix layout animations

- Add panel styling to chat content (rounded corners, consistent background)
- Remove top padding from panels so they extend to button bar
- Add smooth spring animation to HStack to prevent jumping when drawer opens/closes
- Match ThreadTabBar layout pattern with VStack wrapper

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix drawer reopening and VSplitView padding issues

- Reopen thread drawer when closing right-side panel (fixes drawer staying hidden)
- Revert VSplitView padding to exclude top padding (panels flush with button bar)

Addresses Devin review feedback on PR #2150

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Clean up code quality issues

- Remove duplicate .ignoresSafeArea(edges: .top) in drawer mode
- Merge duplicate .onAppear blocks into single initialization
- Add comment explaining 78pt padding for traffic light buttons
- Change ThreadModel.sessionId back to let (no longer mutated)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix shared component regressions and styling issues

- Make chat panel styling conditional on drawer mode (fixes tab mode regression)
- Revert VSplitView animation to VAnimation.standard (shared component fix)
- Extract trafficLightPadding constant with detailed documentation

Addresses critical review feedback: tab mode no longer gets unintended
background/clipping, and VSplitView animation is consistent across all uses.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix drawer not preserving closed state when panel closes

When Settings (or any panel) was opened with drawer already closed, closing
the panel would incorrectly reopen the drawer. Now tracks previous drawer
state and only reopens if it was open before.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Remove automatic drawer/panel linking behavior

Drawer and panels now only open/close via manual toggle - no automatic
closing of drawer when panel opens, no automatic reopening. Both are
completely independent.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* Fix thread item close button gesture conflict

Restructured threadItem to use Button-based approach like ThreadTab,
with close button in overlay instead of nested in HStack. This prevents
onTapGesture from intercepting close button clicks.

Fixes Devin review feedback.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
noanflaherty added a commit that referenced this pull request Mar 1, 2026
…gration access_request exclusion

- isAuthorizedGuardianPrincipal: verify actor's principal belongs to an
  active guardian binding before allowing cross-channel approval, closing
  the asymmetric authorization gap (Devin #3/#4)
- local-actor-identity: eagerly create vellum binding via
  ensureVellumGuardianBinding in pre-bootstrap IPC path so downstream
  decisionable requests always have a guardianPrincipalId (Devin #5)
- Migration 126: exclude access_request from expiry sweep in steps 3a
  and 3c since access_request is non-decisionable and proceeds via the
  invite flow (Codex P2 #2)
- Update roundtrip tests to supply guardianPrincipalId for decisionable
  tool_approval requests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
noanflaherty added a commit that referenced this pull request Mar 1, 2026
* feat: add guardianPrincipalId to schema and storage plumbing (#10965)

Co-authored-by: Claude <noreply@anthropic.com>

* M2: Backfill + startup invariants for guardian principal (#10969)

* feat: backfill guardianPrincipalId and enforce startup invariants

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: restrict backfill expiration to expired requests and tighten desktop rebinding predicate

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* feat: propagate guardianPrincipalId through runtime contexts (#10978)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* M4: Bind principal at all canonical request creation sites (#10980)

* feat: bind guardianPrincipalId at all canonical request creation sites

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: add try/catch guards around createCanonicalGuardianRequest in all caller paths

Wrap createCanonicalGuardianRequest calls in try/catch in three locations
to handle IntegrityError when guardianPrincipalId is missing:

1. daemon/server.ts (makePendingInteractionRegistrar) - prevents crash when
   session.guardianContext is undefined during tool approval events
2. runtime/routes/conversation-routes.ts (makeHubPublisher) - same pattern
   for the HTTP hub publisher path
3. runtime/access-request-helper.ts - preserves the intentional no-binding
   fallback path (documented at file header) where access requests proceed
   without guardian identity

All catch blocks log at debug level matching the existing pattern in
daemon/handlers/sessions.ts.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: add try/catch in inbound-message-handler and fix misleading notified:true in access-request-helper

- Wrap createCanonicalGuardianRequest call in inbound-message-handler.ts
  ingress escalation path with try/catch to prevent unhandled IntegrityError
  from crashing the HTTP handler with a 500. On failure, logs a warning and
  continues to the notification pipeline.

- Fix catch block in access-request-helper.ts to return notified: false
  instead of notified: true, since the emitNotificationSignal call is never
  reached when the error is caught.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: pass guardianPrincipalId to all createBinding callsites

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* M5: Cutover decision authorization + remove isTrusted (#10990)

* feat: cutover decision authorization to principal-based and remove isTrusted

Replace the isTrusted compatibility bypass in all runtime types and callsites
with principal-based authorization. Decision authorization is now purely
principal-based: actor.guardianPrincipalId must match request.guardianPrincipalId
for any applied decision. There is no longer a trusted bypass path.

Key changes:
- Remove isTrusted from ActorContext type, replace with guardianPrincipalId
- Add three-step principal validation in applyCanonicalGuardianDecision()
- Add guardianPrincipalId filter to canonical guardian store queries
- Update guardian-reply-router to use principal-based request discovery
- Update all callsites (HTTP routes, daemon handlers, session process,
  inbound message handler) to resolve and pass guardianPrincipalId
- Replace isTrusted-based guardianReplyText conditionals with channel checks

Closes #10960

* fix: resolve cross-channel principal mismatch and non-vellum channel fallback

- conversation-routes.ts: Fall back to session.guardianContext for
  verifiedActorExternalUserId and verifiedActorPrincipalId when
  actorVerification is null (non-vellum channels).
- guardian-decision-primitive.ts: Allow cross-channel guardian decisions
  by checking actor principal against the assistant's canonical vellum
  binding principal when direct principal match fails.
- guardian-reply-router.ts: Add guardianPrincipalId filter to the
  conversation fallback query in findPendingCanonicalRequests so
  guardians only see requests they are authorized to act on.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: add cross-channel principal fallback to code-only clarification check

Extract isAuthorizedGuardianPrincipal() shared helper from the inline
cross-channel fallback logic in applyCanonicalGuardianDecision, and use
it in the router's code-only clarification identity check. This ensures
a desktop guardian entering a request code for a cross-channel request
(e.g. Telegram-originated) sees the clarification context instead of
getting "Request not found".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961) (#11002)

* feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961)

- Update 6 test files to use guardianPrincipalId principal-match pattern
  instead of legacy isTrusted boolean
- Add guard test (no-is-trusted-guard.test.ts) to prevent isTrusted
  reintroduction in production code
- Clean up 3 stale comments referencing trusted bypass and
  desktop/trusted with principal-based terminology
- Verify no production isTrusted references remain (only allowed
  trust-class variable names: isTrustedActor, isTrustedContact,
  isTrustedTrustClass)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: correct guard test pipe and identity mismatch test field

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: use JS-level allowlist filtering in guard test to prevent masking

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address holistic review — decidedByPrincipalId, access_request exempt, backfill expiry, code lookup guard

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: make isAuthorizedGuardianPrincipal symmetric for cross-channel approval

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address reviewer feedback — binding check, pre-bootstrap IPC, migration access_request exclusion

- isAuthorizedGuardianPrincipal: verify actor's principal belongs to an
  active guardian binding before allowing cross-channel approval, closing
  the asymmetric authorization gap (Devin #3/#4)
- local-actor-identity: eagerly create vellum binding via
  ensureVellumGuardianBinding in pre-bootstrap IPC path so downstream
  decisionable requests always have a guardianPrincipalId (Devin #5)
- Migration 126: exclude access_request from expiry sweep in steps 3a
  and 3c since access_request is non-decisionable and proceeds via the
  invite flow (Codex P2 #2)
- Update roundtrip tests to supply guardianPrincipalId for decisionable
  tool_approval requests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: unify channel binding principals + fix test fixtures for IntegrityError guard

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: align migration registry checkpoint key with migration function (_v1 -> _v2)

* fix: narrow CanonicalDecisionResult union before accessing resolverReplyText

---------

Co-authored-by: Claude <noreply@anthropic.com>
m-abboud added a commit that referenced this pull request Mar 1, 2026
- Use Promise.allSettled instead of Promise.all for parallel Reddit fetches
  so a single endpoint failure (e.g. 403 on /comments) doesn't discard data
  from the other endpoint that succeeded (Codex P2, Devin bug #1)

- Add memory_jobs table to inline schema and enqueue embed_item jobs after
  every memory item insert/update so items are discoverable via semantic
  search (Devin critical bug #2)

- Replace hardcoded Anthropic API call with INTERNAL_GATEWAY_BASE_URL-routed
  request through the daemon provider abstraction layer, falling back to
  pattern-based extraction if the gateway is unavailable (Codex P1)

- Use context.memoryScopeId ?? 'default' instead of hardcoded 'default'
  scope so scoped-session imports don't leak into the global memory scope
  (Codex P1)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tkheyfets pushed a commit that referenced this pull request Mar 2, 2026
* feat: add guardianPrincipalId to schema and storage plumbing (#10965)

Co-authored-by: Claude <noreply@anthropic.com>

* M2: Backfill + startup invariants for guardian principal (#10969)

* feat: backfill guardianPrincipalId and enforce startup invariants

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: restrict backfill expiration to expired requests and tighten desktop rebinding predicate

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* feat: propagate guardianPrincipalId through runtime contexts (#10978)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* M4: Bind principal at all canonical request creation sites (#10980)

* feat: bind guardianPrincipalId at all canonical request creation sites

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: add try/catch guards around createCanonicalGuardianRequest in all caller paths

Wrap createCanonicalGuardianRequest calls in try/catch in three locations
to handle IntegrityError when guardianPrincipalId is missing:

1. daemon/server.ts (makePendingInteractionRegistrar) - prevents crash when
   session.guardianContext is undefined during tool approval events
2. runtime/routes/conversation-routes.ts (makeHubPublisher) - same pattern
   for the HTTP hub publisher path
3. runtime/access-request-helper.ts - preserves the intentional no-binding
   fallback path (documented at file header) where access requests proceed
   without guardian identity

All catch blocks log at debug level matching the existing pattern in
daemon/handlers/sessions.ts.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: add try/catch in inbound-message-handler and fix misleading notified:true in access-request-helper

- Wrap createCanonicalGuardianRequest call in inbound-message-handler.ts
  ingress escalation path with try/catch to prevent unhandled IntegrityError
  from crashing the HTTP handler with a 500. On failure, logs a warning and
  continues to the notification pipeline.

- Fix catch block in access-request-helper.ts to return notified: false
  instead of notified: true, since the emitNotificationSignal call is never
  reached when the error is caught.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: pass guardianPrincipalId to all createBinding callsites

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* M5: Cutover decision authorization + remove isTrusted (#10990)

* feat: cutover decision authorization to principal-based and remove isTrusted

Replace the isTrusted compatibility bypass in all runtime types and callsites
with principal-based authorization. Decision authorization is now purely
principal-based: actor.guardianPrincipalId must match request.guardianPrincipalId
for any applied decision. There is no longer a trusted bypass path.

Key changes:
- Remove isTrusted from ActorContext type, replace with guardianPrincipalId
- Add three-step principal validation in applyCanonicalGuardianDecision()
- Add guardianPrincipalId filter to canonical guardian store queries
- Update guardian-reply-router to use principal-based request discovery
- Update all callsites (HTTP routes, daemon handlers, session process,
  inbound message handler) to resolve and pass guardianPrincipalId
- Replace isTrusted-based guardianReplyText conditionals with channel checks

Closes #10960

* fix: resolve cross-channel principal mismatch and non-vellum channel fallback

- conversation-routes.ts: Fall back to session.guardianContext for
  verifiedActorExternalUserId and verifiedActorPrincipalId when
  actorVerification is null (non-vellum channels).
- guardian-decision-primitive.ts: Allow cross-channel guardian decisions
  by checking actor principal against the assistant's canonical vellum
  binding principal when direct principal match fails.
- guardian-reply-router.ts: Add guardianPrincipalId filter to the
  conversation fallback query in findPendingCanonicalRequests so
  guardians only see requests they are authorized to act on.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: add cross-channel principal fallback to code-only clarification check

Extract isAuthorizedGuardianPrincipal() shared helper from the inline
cross-channel fallback logic in applyCanonicalGuardianDecision, and use
it in the router's code-only clarification identity check. This ensures
a desktop guardian entering a request code for a cross-channel request
(e.g. Telegram-originated) sees the clarification context instead of
getting "Request not found".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961) (#11002)

* feat: M6 — tests + guardrails + cleanup for principal-based auth (#10961)

- Update 6 test files to use guardianPrincipalId principal-match pattern
  instead of legacy isTrusted boolean
- Add guard test (no-is-trusted-guard.test.ts) to prevent isTrusted
  reintroduction in production code
- Clean up 3 stale comments referencing trusted bypass and
  desktop/trusted with principal-based terminology
- Verify no production isTrusted references remain (only allowed
  trust-class variable names: isTrustedActor, isTrustedContact,
  isTrustedTrustClass)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: correct guard test pipe and identity mismatch test field

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: use JS-level allowlist filtering in guard test to prevent masking

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address holistic review — decidedByPrincipalId, access_request exempt, backfill expiry, code lookup guard

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: make isAuthorizedGuardianPrincipal symmetric for cross-channel approval

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address reviewer feedback — binding check, pre-bootstrap IPC, migration access_request exclusion

- isAuthorizedGuardianPrincipal: verify actor's principal belongs to an
  active guardian binding before allowing cross-channel approval, closing
  the asymmetric authorization gap (Devin #3/#4)
- local-actor-identity: eagerly create vellum binding via
  ensureVellumGuardianBinding in pre-bootstrap IPC path so downstream
  decisionable requests always have a guardianPrincipalId (Devin #5)
- Migration 126: exclude access_request from expiry sweep in steps 3a
  and 3c since access_request is non-decisionable and proceeds via the
  invite flow (Codex P2 #2)
- Update roundtrip tests to supply guardianPrincipalId for decisionable
  tool_approval requests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: unify channel binding principals + fix test fixtures for IntegrityError guard

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: align migration registry checkpoint key with migration function (_v1 -> _v2)

* fix: narrow CanonicalDecisionResult union before accessing resolverReplyText

---------

Co-authored-by: Claude <noreply@anthropic.com>
velissa-ai added a commit that referenced this pull request May 25, 2026
…31986)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
dvargasfuertes pushed a commit that referenced this pull request May 25, 2026
PR #32009 feedback follow-up:

* sanitize-display-messages.ts
  - Trim header comment: drop the parenthetical about
    `window._vellumDebug.chat.tail()` (tail() no longer touches the
    pipeline directly — see debug-api change below).
  - Replace the redundant JSDoc + sequential lets in
    `sanitizeDisplayMessages` with a `pipeline.reduce` over the three
    named steps — the implementation now is the spec.
  - Drop the local `sortByTimestamp` wrapper; the imported
    `sortedByTimestamp` is the named pipeline step directly.
  - Update Hack #2 docblock + SHORT TERM line to use the codebase's
    'assistant' terminology and drop the inline ticket reference.

* debug-api.ts
  - `tail()` is now logic-free: it reads from a new
    `sanitizedMessagesRef` populated by the render path. DevTools
    mirrors the UI without re-running the sanitize pipeline. Drops the
    `sanitizeDisplayMessages` import inside debug-api entirely.

* chat-page.tsx + chat-route-content.tsx
  - Own `sanitizedMessagesRef` in the composition root, thread it
    through `ChatRouteRefs`, populate it right after the
    `useMemo(() => sanitizeDisplayMessages(messages))`. Rename
    `sortedMessages` → `sanitizedMessages` to match what it actually
    is.

* build-items.ts
  - Remove the comment claiming `messages` must already be sanitized.
    The function takes whatever it's given; sanitization isn't its
    contract.

* debug-api.test.ts
  - Migrate the existing tail() tests to populate
    `sanitizedMessagesRef` (the new contract). Add a contract test
    that pins 'tail() reads from sanitizedMessagesRef, NOT raw
    messagesRef' so the two refs can't be silently swapped back.

Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
velissa-ai added a commit that referenced this pull request May 25, 2026
#31990)

* feat(memory-v3): tree-node on-disk format + node store (#31971)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): config schema + cheap/capable LLM call sites (#31972)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): curated edge-expansion lane (#31973)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): write-path job types + config (no behavior) (#31974)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): gate decision (ready/more) + final selection (#31975)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): tree index with DAG adjacency + cache (#31976)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): always-on scouts over the v2 substrate (#31977)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): compose node index from children + routing hints (#31978)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): fast filter judging dense hits (sticky bypass) (#31979)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): parallel-fan-out traversal with cycle/visited guards (#31980)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): tree validator (orphans, cycles, dangling refs, freshness) (#31981)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): scout-seeded tree-walk descent driver (#31982)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): assistant memory v3 validate/tree CLI + routes (#31983)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): retrieval loop (scouts->filter->tree->edges->gate) (#31984)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): consolidation drains shared buffer into tree + maintains standing-context files (#31985)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): v3 Retriever as comparand #2 in the compare harness (#31986)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): pass-1->pass-2 co-activation logging (#31987)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): weighted, decaying auto-edge learning job (#31988)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* feat(memory-v3): live shadow via memoryRetrieval middleware (inject v2, log v3) (#31989)

Co-authored-by: Vellum Assistant <assistant@vellum.ai>

* fix(memory-v3): null-safe shadow gate when memory.v3 config is absent

The live-shadow middleware runs on every turn and read `config.memory.v3.enabled`
unguarded. Configs built outside the Zod schema (agent-loop test fixtures) have no
`memory.v3` block, so the gate threw `TypeError: undefined is not an object` and
aborted the turn — cascading across ~13 agent-loop test files. Guard with optional
chaining (matches the loop's existing `write?.coactivation` pattern) and add a
regression test for the absent-v3 config.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(memory-v3): add route policies for memory/v3/validate + tree

PR #31983 registered the two read-only v3 routes but never added their
ACTOR_ENDPOINTS entries in route-policy.ts; the per-PR run skipped CI so the
route-policy coverage guard never ran. Add both as settings.read (mirroring the
v2 read routes), satisfying guard-tests.test.ts.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
vellum-apollo-bot Bot added a commit that referenced this pull request May 27, 2026
- PostMessageResult.conversationId is now always string (was string | null
  in the server-mint flow). The assistant is the authoritative source of
  truth; drop the redundant resolvedConversationId field on both result
  shapes.

- Always send conversationKey on the wire when the caller has an id so
  pre-0.8.6 assistants (which don't read conversationId) still resolve
  the conversation via external-key lookup. On 0.8.6+ assistants, also
  send conversationId to trigger the strict internal-id lookup. The
  duplicated id is harmless: pre-0.8.6 ignores the unknown
  conversationId key; 0.8.6+ reads conversationId first.

- Add non-hook assistantSupports(minVersion) to lib/backwards-compat/utils.ts
  so both the React hook (useAssistantSupports) and event-handler/async
  call sites share the same semver-comparison core. Refactor
  supportsServerMintedConversation() to call it instead of duplicating
  the parse/compare logic.

- Replace the invariant throw in use-send-message.ts with a typecheck-
  enforced contract. postChatMessage's success arm now guarantees a
  non-empty conversationId so the runtime throw is no longer needed.

- Codex P2 — server-mint queue race: add pendingDraftMintRef so the
  queue path in sendMessage awaits the in-flight first-message POST
  before posting follow-up messages. Without this, a follow-up
  submitted during the mint window would 404 on 0.8.6+ assistants
  (the assistant minted a different id from the still-unresolved
  local draft key).

- Update tests: wire-field cutover now asserts both fields on 0.8.6+;
  server-mint tests assert the authoritative-id contract; add coverage
  for caller-supplied id being overridden by the assistant's response.

Followup work (NOT in this PR):
- Add PostMessageResult to assistant/src/api/responses/*.ts and zod-
  validate the response shape in postChatMessage (Vargas comments
  #2 + #4).
dvargasfuertes pushed a commit that referenced this pull request May 27, 2026
…32234)

* feat: server-mint vellum conversations on assistant /send and add web compat gate

* feat(web): wire postChatMessage server-mint flow into use-send-message

postChatMessage(conversationId: null) omits both wire fields so the
daemon mints a fresh row on empty-handed sourceChannel="vellum" sends.
The response's conversationId becomes resolvedConversationId, and the
existing draft-key-resolution path in sendMessage swaps optimistic state
and navigates to the canonical URL.

Sender side (use-send-message.ts):
- sendMessageViaStream now takes isDraft and gates on
  supportsServerMintedConversation() to pick null vs. requestConversationId.
- effectiveConversationId narrowed defensively — postChatMessage's
  success contract guarantees a non-null id (the early-return on
  server-mint without a daemon-echoed id surfaces 422 first).

API side (messages.ts):
- PostMessageResult.conversationId: string | null on success arms.
- Body composition omits both wire fields when conversationId is null;
  pickConversationIdWireField stays in charge of the non-null path.
- 422 early-return when server-mint was requested but the daemon
  accepted the message without echoing a conversation id back.

Tests (post-chat-message.test.ts):
- Server-mint flow describe block: omit both fields, return minted id
  as resolvedConversationId, 422 on broken daemon contract, and a
  guard that the legacy non-null path is unchanged.

Queue path (line 519) left unchanged — it requires an existing
conversation by construction. The tight race where a draft's second
send fires before the first's resolveDraft swap is a known corner
case the existing wire-field gate also hits; will revisit if it
surfaces.

* fix(web): address PR feedback on server-minted conversations

- PostMessageResult.conversationId is now always string (was string | null
  in the server-mint flow). The assistant is the authoritative source of
  truth; drop the redundant resolvedConversationId field on both result
  shapes.

- Always send conversationKey on the wire when the caller has an id so
  pre-0.8.6 assistants (which don't read conversationId) still resolve
  the conversation via external-key lookup. On 0.8.6+ assistants, also
  send conversationId to trigger the strict internal-id lookup. The
  duplicated id is harmless: pre-0.8.6 ignores the unknown
  conversationId key; 0.8.6+ reads conversationId first.

- Add non-hook assistantSupports(minVersion) to lib/backwards-compat/utils.ts
  so both the React hook (useAssistantSupports) and event-handler/async
  call sites share the same semver-comparison core. Refactor
  supportsServerMintedConversation() to call it instead of duplicating
  the parse/compare logic.

- Replace the invariant throw in use-send-message.ts with a typecheck-
  enforced contract. postChatMessage's success arm now guarantees a
  non-empty conversationId so the runtime throw is no longer needed.

- Codex P2 — server-mint queue race: add pendingDraftMintRef so the
  queue path in sendMessage awaits the in-flight first-message POST
  before posting follow-up messages. Without this, a follow-up
  submitted during the mint window would 404 on 0.8.6+ assistants
  (the assistant minted a different id from the still-unresolved
  local draft key).

- Update tests: wire-field cutover now asserts both fields on 0.8.6+;
  server-mint tests assert the authoritative-id contract; add coverage
  for caller-supplied id being overridden by the assistant's response.

Followup work (NOT in this PR):
- Add PostMessageResult to assistant/src/api/responses/*.ts and zod-
  validate the response shape in postChatMessage (Vargas comments
  #2 + #4).

* fix(web): simplify wire-field selection and mint race guard

Round 3 PR feedback (#32234):

1. messages.ts — pick exactly ONE wire field instead of duplicating
   conversationKey alongside conversationId on 0.8.6+ assistants.
   - 0.8.6+ with caller id: conversationId only
   - 0.8.6+ with null:      neither (server-mint branch)
   - pre-0.8.6 with id:     conversationKey only
   - pre-0.8.6 with null:   conversationKey: null (legacy create-or-lookup
     remains addressed; defense-in-depth for callers that bypass the
     supportsServerMintedConversation gate)

2. use-send-message.ts — replace the deferred-promise race guard with a
   simpler "block any send while a mint is in flight" gate. The POST
   200s quickly, so rejecting follow-up sends with a brief inline error
   is simpler than threading an unresolved id through the queue path.
   - pendingDraftMintRef: useRef<string | null> (was { draftId, promise })
   - set before POST in sendMessageViaStream, cleared after resolve/reject
   - sendMessage entry returns early if the gate matches the active draft

Tests updated to match the new contract; added coverage for
conversationKey: null on pre-0.8.6 callers. 27/27 pass; tsc + eslint
clean on touched files; assistant send-endpoint-busy 15/15 unchanged.

---------

Co-authored-by: vellum-apollo-bot[bot] <229272765+vellum-apollo-bot[bot]@users.noreply.github.com>
Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
vellum-apollo-bot Bot pushed a commit that referenced this pull request May 28, 2026
Eliminates the executor's mode branch in favor of self-contained execute()
bodies for every host-proxied tool, then deletes the now-redundant
executionMode field entirely.

Why
---
PR #32342 moved executionMode from `Tool` to `ToolDefinition` as a pure
type-level move. This PR (alternative) goes further: it asks whether the
field is needed at all once dispatch is unified.

After the unification, executionMode had two genuinely different
concerns conflated under one name:

1. **Dispatch** — folded into each tool's `execute()`. The executor no
   longer special-cases mode='proxy'; every tool's execute() handles
   its own forwarding (to context.proxyToolResolver for client tools,
   to supervisor.dispatchTool for meet tools, etc.).
2. **Skill projection dedup** — already removed in #32365, which proved
   the owner-only filter on `getAllToolDefinitions` and
   `getRegisteredToolNames` is sufficient. No replacement field needed.

So executionMode just gets deleted outright. No rename.

Changes
-------
- Each proxy tool's `execute()` now forwards via context.proxyToolResolver
  with name-bound closure. No-resolver case returns a structured error
  result (isError: true) instead of throwing.
- executor.ts drops both mode branches (initial dispatch + CES retry).
- `resolveExecutionTarget` drops rule #2 (mode=proxy ⇒ host). Every
  declaration site that used to rely on it already stamps target='host'
  directly.
- `tool-approval-handler.ts` drops the resolver-aware available-tools
  filter — execute() handles no-resolver gracefully now.
- IPC schema in skill-routes/registries.ts drops executionMode from the
  wire payload; contracts-side Tool + client.ts manifest projection
  drop it too.

Field count: -1 executionMode, +0. The win is dispatch unification
plus the simpler tool surface that #32365 enabled.
vellum-apollo-bot Bot pushed a commit that referenced this pull request May 28, 2026
Eliminates the executor's mode branch in favor of self-contained execute()
bodies for every host-proxied tool, then deletes the now-redundant
executionMode field entirely.

Why
---
PR #32342 moved executionMode from `Tool` to `ToolDefinition` as a pure
type-level move. This PR (alternative) goes further: it asks whether the
field is needed at all once dispatch is unified.

After the unification, executionMode had two genuinely different
concerns conflated under one name:

1. **Dispatch** — folded into each tool's `execute()`. The executor no
   longer special-cases mode='proxy'; every tool's execute() handles
   its own forwarding (to context.proxyToolResolver for client tools,
   to supervisor.dispatchTool for meet tools, etc.).
2. **Skill projection dedup** — already removed in #32365, which proved
   the owner-only filter on `getAllToolDefinitions` and
   `getRegisteredToolNames` is sufficient. No replacement field needed.

So executionMode just gets deleted outright. No rename.

Changes
-------
- Each proxy tool's `execute()` now forwards via context.proxyToolResolver
  with name-bound closure. No-resolver case returns a structured error
  result (isError: true) instead of throwing.
- executor.ts drops both mode branches (initial dispatch + CES retry).
- `resolveExecutionTarget` drops rule #2 (mode=proxy ⇒ host). Every
  declaration site that used to rely on it already stamps target='host'
  directly.
- `tool-approval-handler.ts` drops the resolver-aware available-tools
  filter — execute() handles no-resolver gracefully now.
- IPC schema in skill-routes/registries.ts drops executionMode from the
  wire payload; contracts-side Tool + client.ts manifest projection
  drop it too.

Field count: -1 executionMode, +0. The win is dispatch unification
plus the simpler tool surface that #32365 enabled.
dvargasfuertes pushed a commit that referenced this pull request May 28, 2026
Eliminates the executor's mode branch in favor of self-contained execute()
bodies for every host-proxied tool, then deletes the now-redundant
executionMode field entirely.

Why
---
PR #32342 moved executionMode from `Tool` to `ToolDefinition` as a pure
type-level move. This PR (alternative) goes further: it asks whether the
field is needed at all once dispatch is unified.

After the unification, executionMode had two genuinely different
concerns conflated under one name:

1. **Dispatch** — folded into each tool's `execute()`. The executor no
   longer special-cases mode='proxy'; every tool's execute() handles
   its own forwarding (to context.proxyToolResolver for client tools,
   to supervisor.dispatchTool for meet tools, etc.).
2. **Skill projection dedup** — already removed in #32365, which proved
   the owner-only filter on `getAllToolDefinitions` and
   `getRegisteredToolNames` is sufficient. No replacement field needed.

So executionMode just gets deleted outright. No rename.

Changes
-------
- Each proxy tool's `execute()` now forwards via context.proxyToolResolver
  with name-bound closure. No-resolver case returns a structured error
  result (isError: true) instead of throwing.
- executor.ts drops both mode branches (initial dispatch + CES retry).
- `resolveExecutionTarget` drops rule #2 (mode=proxy ⇒ host). Every
  declaration site that used to rely on it already stamps target='host'
  directly.
- `tool-approval-handler.ts` drops the resolver-aware available-tools
  filter — execute() handles no-resolver gracefully now.
- IPC schema in skill-routes/registries.ts drops executionMode from the
  wire payload; contracts-side Tool + client.ts manifest projection
  drop it too.

Field count: -1 executionMode, +0. The win is dispatch unification
plus the simpler tool surface that #32365 enabled.

Co-authored-by: vellum-apollo-bot[bot] <220839023+vellum-apollo-bot[bot]@users.noreply.github.com>
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.

1 participant