Skip to content

ci: add terraform apply workflow on platform changes#3

Merged
dvargasfuertes merged 1 commit into
mainfrom
ci/terraform-apply
Feb 7, 2026
Merged

ci: add terraform apply workflow on platform changes#3
dvargasfuertes merged 1 commit into
mainfrom
ci/terraform-apply

Conversation

@vargas-jr
Copy link
Copy Markdown

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

Summary

GitHub Action that runs terraform apply when platform changes are merged to main.

Trigger

  • Push to main branch
  • Only when platform/** files are modified

Workflow Steps

  1. Checkout code
  2. Setup Terraform 1.7.0
  3. Authenticate to GCP
  4. Get GKE cluster credentials
  5. terraform init
  6. terraform validate
  7. terraform plan
  8. terraform apply

Required Secrets

Secret Description
GCP_SA_KEY GCP service account JSON key
DATABASE_URL PostgreSQL connection string
ANTHROPIC_API_KEY Anthropic API key

Required Variables

Variable Description
GCP_PROJECT_ID GCP project ID
GCP_REGION GCP region (e.g. us-central1)
GKE_CLUSTER_NAME Target GKE cluster name

Open with Devin

Triggers on merge to main when platform/** files change.
Requires secrets: GCP_SA_KEY, DATABASE_URL, ANTHROPIC_API_KEY
Requires vars: GCP_PROJECT_ID, GCP_REGION, GKE_CLUSTER_NAME
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 on lines +39 to +42
run: |
gcloud container clusters get-credentials ${{ vars.GKE_CLUSTER_NAME }} \
--region ${{ vars.GCP_REGION }} \
--project ${{ vars.GCP_PROJECT_ID }}
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.

🟡 Script injection via unsanitized interpolation of vars.* into shell run block

The Get GKE credentials step directly interpolates ${{ vars.GKE_CLUSTER_NAME }}, ${{ vars.GCP_REGION }}, and ${{ vars.GCP_PROJECT_ID }} into a shell run: command. If any of these repository variables contain shell metacharacters (e.g., ; rm -rf / or $(malicious_command)), they will be interpreted by the shell, leading to arbitrary command execution in the CI runner.

Security Details and Recommended Fix

While vars.* are set by repository admins (not arbitrary PR authors), this is still a security anti-pattern per GitHub's security hardening guide. The recommended approach is to assign the values to environment variables first, then reference them in the shell:

- name: Get GKE credentials
  run: |
    gcloud container clusters get-credentials "$GKE_CLUSTER_NAME" \
      --region "$GCP_REGION" \
      --project "$GCP_PROJECT_ID"
  env:
    GKE_CLUSTER_NAME: ${{ vars.GKE_CLUSTER_NAME }}
    GCP_REGION: ${{ vars.GCP_REGION }}
    GCP_PROJECT_ID: ${{ vars.GCP_PROJECT_ID }}

This ensures the values are passed as environment variables and properly quoted, preventing shell interpretation of special characters.

Impact: A compromised or misconfigured repository variable could lead to arbitrary command execution in the CI environment, potentially leaking secrets or modifying infrastructure.

Suggested change
run: |
gcloud container clusters get-credentials ${{ vars.GKE_CLUSTER_NAME }} \
--region ${{ vars.GCP_REGION }} \
--project ${{ vars.GCP_PROJECT_ID }}
run: |
gcloud container clusters get-credentials "$GKE_CLUSTER_NAME" \
--region "$GCP_REGION" \
--project "$GCP_PROJECT_ID"
env:
GKE_CLUSTER_NAME: ${{ vars.GKE_CLUSTER_NAME }}
GCP_REGION: ${{ vars.GCP_REGION }}
GCP_PROJECT_ID: ${{ vars.GCP_PROJECT_ID }}
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
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>
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
…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>
Jasonnnz pushed a commit that referenced this pull request Feb 23, 2026
P1 #3: Add conversationId to TaskSubmit so direct CU QA sessions can set
reportToSessionId. Client passes active thread ID. Daemon routes it to CU
session metadata, enabling attachment creation on finalize.

P1 #4: Add retentionDays to TaskRouted from daemon config. Client reads it
instead of hardcoding 7 days.

Also: create file-backed attachment for recordings without reportToSessionId
so cleanup can track orphan files.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Jasonnnz added a commit that referenced this pull request Feb 23, 2026
…ays (#6951)

P1 #3: Add conversationId to TaskSubmit so direct CU QA sessions can set
reportToSessionId. Client passes active thread ID. Daemon routes it to CU
session metadata, enabling attachment creation on finalize.

P1 #4: Add retentionDays to TaskRouted from daemon config. Client reads it
instead of hardcoding 7 days.

Also: create file-backed attachment for recordings without reportToSessionId
so cleanup can track orphan files.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Jasonnnz added a commit that referenced this pull request Feb 23, 2026
…ays (#6951)

P1 #3: Add conversationId to TaskSubmit so direct CU QA sessions can set
reportToSessionId. Client passes active thread ID. Daemon routes it to CU
session metadata, enabling attachment creation on finalize.

P1 #4: Add retentionDays to TaskRouted from daemon config. Client reads it
instead of hardcoding 7 days.

Also: create file-backed attachment for recordings without reportToSessionId
so cleanup can track orphan files.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Jasonnnz added a commit that referenced this pull request Feb 23, 2026
…ays (#6951)

P1 #3: Add conversationId to TaskSubmit so direct CU QA sessions can set
reportToSessionId. Client passes active thread ID. Daemon routes it to CU
session metadata, enabling attachment creation on finalize.

P1 #4: Add retentionDays to TaskRouted from daemon config. Client reads it
instead of hardcoding 7 days.

Also: create file-backed attachment for recordings without reportToSessionId
so cleanup can track orphan files.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Jasonnnz added a commit that referenced this pull request Feb 23, 2026
…ays (#6951)

P1 #3: Add conversationId to TaskSubmit so direct CU QA sessions can set
reportToSessionId. Client passes active thread ID. Daemon routes it to CU
session metadata, enabling attachment creation on finalize.

P1 #4: Add retentionDays to TaskRouted from daemon config. Client reads it
instead of hardcoding 7 days.

Also: create file-backed attachment for recordings without reportToSessionId
so cleanup can track orphan files.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Jasonnnz added a commit that referenced this pull request Feb 23, 2026
…ays (#6951)

P1 #3: Add conversationId to TaskSubmit so direct CU QA sessions can set
reportToSessionId. Client passes active thread ID. Daemon routes it to CU
session metadata, enabling attachment creation on finalize.

P1 #4: Add retentionDays to TaskRouted from daemon config. Client reads it
instead of hardcoding 7 days.

Also: create file-backed attachment for recordings without reportToSessionId
so cleanup can track orphan files.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Jasonnnz added a commit that referenced this pull request Feb 23, 2026
…ays (#6951)

P1 #3: Add conversationId to TaskSubmit so direct CU QA sessions can set
reportToSessionId. Client passes active thread ID. Daemon routes it to CU
session metadata, enabling attachment creation on finalize.

P1 #4: Add retentionDays to TaskRouted from daemon config. Client reads it
instead of hardcoding 7 days.

Also: create file-backed attachment for recordings without reportToSessionId
so cleanup can track orphan files.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Jasonnnz added a commit that referenced this pull request Feb 23, 2026
…ays (#6951)

P1 #3: Add conversationId to TaskSubmit so direct CU QA sessions can set
reportToSessionId. Client passes active thread ID. Daemon routes it to CU
session metadata, enabling attachment creation on finalize.

P1 #4: Add retentionDays to TaskRouted from daemon config. Client reads it
instead of hardcoding 7 days.

Also: create file-backed attachment for recordings without reportToSessionId
so cleanup can track orphan files.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
Co-authored-by: Claude Opus 4.6 <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 pushed a commit that referenced this pull request May 5, 2026
…hen capable client connected (#29632)

* fix(tool-projection): expose host_file_* on cross-client transports when capable client connected (#29613)

* feat(tool-projection): add CROSS_CLIENT_EXPOSED_CAPABILITIES set

Generalizes the cross-client tool-exposure carve-out in
isToolActiveForContext from a hardcoded host_bash check to a
Set<HostProxyCapability> containing host_bash and host_file. Phase 2
(PR #29398) and Phase 3 (PR #29440) shipped the routing infrastructure
for cross-client host_file_* execution but never extended this
exposure gate, so web/iphone turns silently lacked all four
host_file_* tools even when a macOS client was connected. Also fixes
a latent miscount: the hub query now uses the actual capability under
inspection instead of the hardcoded "host_bash" string.

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

* test(tool-projection): cover host_file_* cross-client exposure paths

Generalizes the per-capability client-count mock from a single
host_bash counter to a Map keyed by capability, then adds 16 new
tests (4 host_file_* tools × {exposed-with-client, blocked-no-client,
blocked-on-chrome-extension, blocked-when-hasNoClient}) plus a
regression guard verifying listClientsByCapability is queried with
the actual capability under inspection (D5 latent-bug fix).

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

* docs(tool-projection): update file docstring for cross-client carve-out

Updates the test file's top-of-file docstring to describe the
generalized CROSS_CLIENT_EXPOSED_CAPABILITIES carve-out (host_bash +
host_file) instead of the Phase-1-only host_bash framing.

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

* fix(tool-projection): add host_file_* executor guards for cross-client transports

Adds the 'no-client' and 'stale-targetClientId' runtime guards to
host_file_read / write / edit (and the missing no-client guard to
host_file_transfer), mirroring the existing host_bash guards in
host-shell.ts. Without these guards, exposing host_file_* on
non-host-proxy transports (web, iphone) via PR #29613's cross-client
carve-out could silently fall through to the daemon container's
filesystem if the macOS client disconnects between tool projection and
tool execution — reading or modifying files inside the cloud daemon
instead of the user's host machine.

Addresses Codex P1 on PR #29613.

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

* fix(tool-projection): scope host_file_* disconnected-target guard to non-host-proxy turns

Codex P2 on PR #29613: the disconnected-target guard added in commit
4ec0e1a rejects any host_file_{read,write,edit} call with
target_client_id when HostFileProxy is unavailable, even on macos turns
where local-fs fallback IS the intended offline behavior. If a stale or
auto-filled target_client_id leaks in from a prior cross-client turn, a
macos turn now errors instead of writing locally — a regression vs the
pre-PR behavior.

Scope the guard to !supportsHostProxy(transport) so it only fires on
web/iphone, where local fs would be the daemon container's filesystem
and falling through is genuinely unsafe. macos turns silently ignore a
stale target_client_id and fall through to FileSystemOps as before.

Adds one regression test per executor (read/write/edit) covering macos +
stale target_client_id + unavailable proxy → falls through to local fs.

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

* fix(tool-projection): scope host_file_transfer disconnected-target guard to non-host-proxy turns

Devin review on PR #29613: transfer.ts had the same scope drift the
prior commit fixed in read/write/edit. The pre-existing post-proxy
guard `if (targetClientId != null) { ... no host client is available }`
fired on macOS too, so a stale/auto-filled target_client_id from a
prior cross-client turn caused host_file_transfer to error on macOS
even though host_file_{read,write,edit} silently fell through to local
fs (the desired offline-mode behavior).

Replaces the unscoped guard with a scoped guard #3 placed at the top of
execute() (matching the read/write/edit pattern), keyed on
!supportsHostProxy(transport). The non-host-proxy + stale-target case
now produces the same "target client ... is no longer connected"
message as the other tools; on macOS the stale target_client_id is
silently ignored and the call falls through to executeLocal as before
PR #29613.

Adds two tests in transfer.test.ts: web + stale target → error, and
macos + stale target → falls through to local copy.

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

---------

Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* fix(tool-projection): self-review cleanup for host_file_* exposure (#29621)

Phase 4 round 1: addresses minor integration-review gaps from the
self-review pass on PR #29613.

- Replace stale "iphone" references with the actual InterfaceId "ios"
  in 6 comment locations (conversation-tool-setup.ts, the 4 host_file_*
  executors).
- Add an explanatory note in each host_file_* executor's
  disconnected-target guard pointing out the deliberate divergence
  from host_bash (host-shell.ts:239-247) and linking to the PR #29613
  review for rationale, so future readers understand the asymmetry is
  intentional.
- Align transfer.ts's "no client connected" guard message with the
  read/write/edit pattern by referring to "host_file" (the capability)
  rather than "host_file_transfer" (the tool name).
- Add a stub mock for assistant-event-hub.js in transfer.test.ts so
  the multi-client guard test runs against an isolated stub rather
  than the live process-wide singleton, matching the read/write/edit
  test pattern.

Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* fix(tool-projection): replace PR cross-reference in divergence comment with inline rationale (#29628)

Phase 4 round 2: Devin BUG findings on PR #29621 flagged the
"see PR #29613 review discussion for rationale" tail of the new
divergence comments as a violation of assistant/AGENTS.md "Code
comments" rule (don't narrate history; describe the current state).

Replaces the PR cross-reference in each of the 4 host_file_*
executors with a short inline statement of the asymmetry — that
host_bash rejects unconditionally for any stale target_client_id
regardless of transport — keeping the divergence call-out without
relying on PR-discussion archaeology.

Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

* fix(tool-projection): close backwards-compat hole in host_file_* disconnected-target guard

Codex P1 on the feature-branch PR (#29632): the disconnected-target
guard added in PR #29613 / cycle 2 only fires when
\`context.transportInterface != null\`. On the documented legacy/
backwards-compat path where transport metadata is missing, a call with
target_client_id and no connected host client skips this error path
and falls through to local FileSystemOps / executeLocal — silently
targeting the daemon container's filesystem instead of the intended
host client. This was a real regression introduced by cycle 3's
removal of transfer.ts's pre-existing unscoped guard.

Restructure the condition so the guard fires for both non-host-proxy
transports AND undefined transport, skipping only when transport is
explicitly host-proxy-capable (macos, where local-fs fallback IS the
intended offline behavior). Keeps the cycle-2 behavior for macos
fall-through while restoring the safety guarantee for legacy callers.

Adds one regression test per executor (read/write/edit/transfer) for
the undefined-transport + target_client_id + no-proxy → reject path.

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

---------

Co-authored-by: credence-the-bot[bot] <277301654+credence-the-bot[bot]@users.noreply.github.com>
Co-authored-by: credence-the-bot[bot] <credence-the-bot[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
credence-the-bot Bot pushed a commit that referenced this pull request May 10, 2026
Four review findings on PR #30198 commits c3416ac + 8b335eb:

* **[Codex P1]** `provider-send-message.ts:127` — provider/connection
  mismatch silent misroute. A profile that names `provider: "openai"`
  together with an Anthropic-flavored `provider_connection` would
  silently dispatch traffic to the connection's backend (Anthropic)
  while metadata still reported the resolved profile (OpenAI). Now we
  verify `connection.provider === expectedProvider` and fall through
  to the legacy registry path (with a warn log) on mismatch instead of
  silently misrouting. `tryResolveProviderForConnectionName` gains an
  optional `expectedProvider` parameter; canonical dispatch passes
  `resolved.provider`, `resolveDefaultProvider` passes
  `profile.provider`, and `CallSiteRoutingProvider`'s async hook
  signature is widened to `(connectionName, expectedProvider)` so the
  shared wrapper threads the value through.

* **[Codex P2 / Devin BUG #1]** `approval-generators.ts:141` — stale
  `listProviders().includes(...)` guard blocks connection-aware
  resolution. In `createApprovalConversationGenerator`, the legacy
  guard threw "No provider available" before the new
  `resolveDefaultProvider(config)` could resolve from connection
  auth. In `your-own` configurations where the default provider lives
  entirely behind a `provider_connection` and never appears in the
  legacy `listProviders()` list, this caused false failures. Removed
  the guard; `resolveDefaultProvider` already returns null on miss
  and we throw on null (right "no provider available" signal).
  `listProviders` import dropped along with it.

* **[Devin BUG #2]** Dead code — `resolveProviderForCallSite` in
  `connection-resolution.ts` was exported for "completeness" but
  never imported or called anywhere. Deleted (and its now-unused
  imports of `resolveCallSiteConfig` + `LLMCallSite`).

* **[Devin BUG #3]** JSDoc for `wrapWithCallSiteRouting` referenced
  helpers "that lived in approval-generators.ts and
  guardian-action-generators.ts" — those were removed in this PR, so
  the comment described history rather than current behavior. Rewrote
  to describe what the function does, not what it replaced.

New gate test in `satellite-connection-routing.test.ts`:
  - profile says provider=openai, provider_connection points at an
    anthropic-flavored row → connection-resolution hook returns null
    on mismatch → legacy `getProvider("openai")` path produces the
    OpenAI stub. Without this fix the connection's anthropic stub
    would have run instead.

Verified:
- `bunx tsc --noEmit` → clean.
- `bun test src/providers/__tests__/` → 103 pass / 0 fail / 209 expects (was 102/0/206; +1 test, +3 expects).
noanflaherty pushed a commit that referenced this pull request May 10, 2026
* feat(inference): wire provider_connection into canonical dispatch

Cycle-3 wiring PR — addresses Codex P1 from #30162: the
`resolveProviderFromConnection` resolver shipped as dead code with zero
call sites. This commit wires the canonical inference dispatch path
(`resolveConfiguredProvider` in provider-send-message.ts) to actually use
the resolver.

Behavior:
1. When the resolved profile has `provider_connection`, dispatch routes
   through `resolveProviderFromConnection(connection, config)`.
2. On any miss (connection not found, resolver returns null, lookup
   throws), warn and fall through to legacy `getProvider(name)` so
   misconfiguration doesn't break inference.
3. Profiles without `provider_connection` keep working unchanged via the
   legacy path.

Also: add `provider_connection?: string` to LLMConfigBase. The field
already flowed through `resolveCallSiteConfig` at runtime
(`profileConfigFragment` strips only `source`/`label`/`description`),
so this aligns the type with reality for type-safe access.

Gate test: dispatch-connection-routing.test.ts is the test cycle-2 was
missing. Four cases:
  - Two profiles, same provider, different `provider_connection` →
    resolver called twice, with the right connection each time, with
    auth bundles distinguishable per profile (mix-and-match goal #2).
  - Profile WITHOUT `provider_connection` → resolver NOT called, legacy
    path takes over.
  - `provider_connection` set but unknown → resolver NOT called, legacy
    fallback succeeds.
  - `provider_connection` set, found, but resolver returns null →
    resolver IS called, legacy fallback succeeds.

If wiring regresses, `resolveProviderCalls.length` invariants break the
first test. That's the gate cycle-2 lacked — it tested DB shape, not
dispatch invocation.

* feat(inference): wire provider_connection through satellite providers

Cycle-3 follow-up to canonical-path wiring (c3416ac). Migrates the
five satellite construction-time call sites — subagent manager, daemon
conversation/approval/guardian generators, and rollup producer — to a
single shared connection-aware default-provider resolution path.

* New `providers/connection-resolution.ts`:
  - `tryResolveProviderForConnectionName(name, config)` — promoted from
    private helper in `provider-send-message.ts`. Looks up a
    `provider_connection` row and resolves a Provider with that
    connection's auth bound. Logs and returns null on any miss so
    callers can fall back to the legacy registry path.
  - `resolveDefaultProvider(config)` — for satellites' construction-time
    path. Reads `config.llm.default.{provider, provider_connection}`,
    routes through the connection if named, otherwise legacy lookup.
  - `resolveProviderForCallSite(callSite, config, opts)` — exported
    completeness analogue.

* `call-site-routing.ts`:
  - `CallSiteRoutingProvider` gains an optional async `resolveByConnection`
    ctor param. `selectProvider` is now async; falls through to legacy
    on miss so existing call sites keep working unchanged.
  - New `wrapWithCallSiteRouting(base, config)` helper — replaces the
    per-file wrappers in `approval-generators` and `guardian-action-generators`
    so all five satellites share one wiring shape.

* Satellites migrated (all five):
  - `subagent/manager.ts` — `resolveDefaultProvider` + `wrapWithCallSiteRouting`.
    Throws on null default (preserves existing failure mode).
  - `daemon/conversation-store.ts` — same pattern; throws on null default.
  - `daemon/approval-generators.ts` — both copy + conversation generators
    use the shared wrapping; copy returns null on miss, conversation throws.
  - `daemon/guardian-action-generators.ts` — both generators use the
    shared wrapper. `getConfiguredProvider` already routes through the
    canonical path, so only the wrapper changes here.
  - `home/rollup-producer.ts` — uses `resolveDefaultProvider`. The
    `RollupProducerDeps.resolveProvider?` injection point is widened to
    `() => Provider | null | Promise<Provider | null>` for async tests.

* `provider-send-message.ts` switches to the shared
  `tryResolveProviderForConnectionName` and drops the now-redundant local
  `tryResolveFromConnection` (and its unused `log`/`getLogger` imports).

* Hard-gate test: `__tests__/satellite-connection-routing.test.ts`. Four
  cases on `CallSiteRoutingProvider` directly:
    1. Alternate-profile callSite with `provider_connection` →
       connection-resolution hook fires with the right name+auth and the
       transport that runs is the connection-bound stub (not default,
       not legacy registry).
    2. Alternate-profile callSite WITHOUT `provider_connection` →
       connection hook never fires; legacy registry path produces the stub.
    3. Alternate-profile callSite with unknown `provider_connection` →
       falls through; system stays operational on default.
    4. No callSite → straight to default; no hook, no registry lookup.

  Combined with the cycle-3 first-commit dispatch tests, that's 8 gate
  tests across both code paths (canonical `provider-send-message`
  resolution + satellite `CallSiteRoutingProvider` wrapping).

Cycle-2 + cycle-3-first-commit shipped `resolveProviderFromConnection` as
dead code (zero call sites). This wiring removes that gap. Cycle-4 will
strip the legacy fallback paths once we've shipped one release window
with connection-awareness active.

Verified:
- typecheck clean (assistant + gateway).
- lint clean on touched files.
- `bun test src/providers/__tests__/` → 102 pass / 0 fail / 206 expects.

* test(config-watcher): mock new registry exports added in #30162 / cycle-3

`config-watcher.test.ts` mocks `providers/registry.js` with a stub
factory that doesn't include the new exports added by the inference
providers refactor: `clearConnectionProviderCache` (added in #30162)
and `resolveProviderFromConnection` (used transitively by the new
`connection-resolution.ts` module). When the test file is loaded in
isolation — or in some test orderings — the import chain through
`providers/inference/connections.ts` and `providers/connection-resolution.ts`
hits a `SyntaxError: Export named '...' not found`.

Adds both names to the mock factory. Verified locally:
  bun test src/__tests__/config-watcher.test.ts → 19 pass / 0 fail.

Pre-existing on main (#30162 added the first import without updating
the mock); landing here so CI can run Test green for this PR.

* fix(inference): address review feedback on provider_connection wiring

Four review findings on PR #30198 commits c3416ac + 8b335eb:

* **[Codex P1]** `provider-send-message.ts:127` — provider/connection
  mismatch silent misroute. A profile that names `provider: "openai"`
  together with an Anthropic-flavored `provider_connection` would
  silently dispatch traffic to the connection's backend (Anthropic)
  while metadata still reported the resolved profile (OpenAI). Now we
  verify `connection.provider === expectedProvider` and fall through
  to the legacy registry path (with a warn log) on mismatch instead of
  silently misrouting. `tryResolveProviderForConnectionName` gains an
  optional `expectedProvider` parameter; canonical dispatch passes
  `resolved.provider`, `resolveDefaultProvider` passes
  `profile.provider`, and `CallSiteRoutingProvider`'s async hook
  signature is widened to `(connectionName, expectedProvider)` so the
  shared wrapper threads the value through.

* **[Codex P2 / Devin BUG #1]** `approval-generators.ts:141` — stale
  `listProviders().includes(...)` guard blocks connection-aware
  resolution. In `createApprovalConversationGenerator`, the legacy
  guard threw "No provider available" before the new
  `resolveDefaultProvider(config)` could resolve from connection
  auth. In `your-own` configurations where the default provider lives
  entirely behind a `provider_connection` and never appears in the
  legacy `listProviders()` list, this caused false failures. Removed
  the guard; `resolveDefaultProvider` already returns null on miss
  and we throw on null (right "no provider available" signal).
  `listProviders` import dropped along with it.

* **[Devin BUG #2]** Dead code — `resolveProviderForCallSite` in
  `connection-resolution.ts` was exported for "completeness" but
  never imported or called anywhere. Deleted (and its now-unused
  imports of `resolveCallSiteConfig` + `LLMCallSite`).

* **[Devin BUG #3]** JSDoc for `wrapWithCallSiteRouting` referenced
  helpers "that lived in approval-generators.ts and
  guardian-action-generators.ts" — those were removed in this PR, so
  the comment described history rather than current behavior. Rewrote
  to describe what the function does, not what it replaced.

New gate test in `satellite-connection-routing.test.ts`:
  - profile says provider=openai, provider_connection points at an
    anthropic-flavored row → connection-resolution hook returns null
    on mismatch → legacy `getProvider("openai")` path produces the
    OpenAI stub. Without this fix the connection's anthropic stub
    would have run instead.

Verified:
- `bunx tsc --noEmit` → clean.
- `bun test src/providers/__tests__/` → 103 pass / 0 fail / 209 expects (was 102/0/206; +1 test, +3 expects).

* fix(inference): catch resolveProviderFromConnection rejections, fall through to legacy

Codex P1 follow-up on PR #30198 (#discussion_r3215235367):

`tryResolveProviderForConnectionName` returned the
`resolveProviderFromConnection(connection, config)` promise directly,
so any rejection from the inner async path — credential read failure
in `resolveAuth`, managed-proxy context lookup throwing, transient
provider-impl construction errors — bubbled out of the helper and
hard-failed dispatch instead of falling through to the legacy
`getProvider(resolved.provider)` path the helper's contract advertises.

Wrap in try/catch; log a warn with `{ err, connectionName }` and
return null so `getConfiguredProvider` and `CallSiteRoutingProvider`
can serve the request through the legacy registry. The behavior now
matches the lookup-failure and not-found branches above.

New gate test in satellite-connection-routing.test.ts: a connection
whose `resolveProviderFromConnection` throws does not fail dispatch.
The wrapper catches the rejection, logs, and falls through; the
profile's resolved provider matches default → reused default instance.
Test infrastructure adds a `connectionsThatThrowOnResolve` Set so
individual tests can opt into the throwing path without affecting
the other cases.

Verified:
- `bunx tsc --noEmit` → clean.
- `bun test src/providers/__tests__/satellite-connection-routing.test.ts` → 6 pass / 0 fail / 23 expects (was 5/0/19; +1 test, +4 expects).

---------

Co-authored-by: credence-the-bot[bot] <test@test.com>
Co-authored-by: Credence <credence@vellum.ai>
ashleeradka pushed a commit that referenced this pull request May 20, 2026
- Rule now visits `TSImportType` so type-position imports like
  `type T = import("@/domains/x/y").Z` are caught.
- 2 new tests cover same-domain (valid) and cross-domain (invalid)
  TSImportType cases.
- Diagnostic in pr-web.yaml's Lint step: on non-zero exit, dump
  node/bun/eslint versions, generated/ status, file count, and
  re-run with --max-warnings 0 to surface anything ESLint flagged
  as a warning. Temporary — reverted once root cause is found.

Addresses Codex P2 #3 (TSImportType not handled).

https://claude.ai/code/session_01JPGu4yGPTL4JoLaPuqpEW2
ashleeradka pushed a commit that referenced this pull request May 20, 2026
- Rule now visits `TSImportType` so type-position imports like
  `type T = import("@/domains/x/y").Z` are caught.
- 2 new tests cover same-domain (valid) and cross-domain (invalid)
  TSImportType cases.
- Diagnostic in pr-web.yaml's Lint step: on non-zero exit, dump
  node/bun/eslint versions, generated/ status, file count, and
  re-run with --max-warnings 0 to surface anything ESLint flagged
  as a warning. Temporary — reverted once root cause is found.

Addresses Codex P2 #3 (TSImportType not handled).

https://claude.ai/code/session_01JPGu4yGPTL4JoLaPuqpEW2
ashleeradka added a commit that referenced this pull request May 20, 2026
* feat(web): enforce cross-domain import boundaries (LUM-1756)

Adds an ESLint guardrail that blocks new cross-domain imports
between `apps/web/src/domains/<x>/` peers. The premise (from
bulletproof-react and Feature-Sliced Design): code consumed by
two or more domains belongs at a top-level shared dir, not
imported peer-to-peer.

The codebase has 148 such imports today, snapshotted into a
migration allow-list. The lint rule passes for existing
violations and fails for any new one — the count can only
shrink. LUM-1753 tracks the cleanup work.

Pieces:

- `eslint-rules/no-cross-domain-imports.mjs` — small inline
  custom rule (no plugin dep, fully visible to readers).
- `eslint-rules/no-cross-domain-imports.test.mjs` — 6 unit
  tests via ESLint's RuleTester, run by `bun test`.
- `scripts/audit-cross-domain-imports.mjs` — single source of
  truth for the allow-list. Run with `bun run audit:cross-domain`
  to regenerate. Supports `--check` (CI gate for drift) and
  `--stats` (count summary).
- `.cross-domain-allowlist.json` — deterministic snapshot of
  current violations, keyed by file path.
- `docs/CONVENTIONS.md` — tightened to "no cross-domain imports"
  with an Enforcement subsection pointing at the rule and the
  regen command.
- `README.md` — calls out the lint-enforced boundary so OSS
  contributors see it on first read.

Verified:
- `bunx eslint src` passes (148 violations allow-listed; only the
  one pre-existing unrelated warning remains).
- 6/6 rule tests pass.
- Manual injection of a new violation correctly fails the lint.

https://claude.ai/code/session_01JPGu4yGPTL4JoLaPuqpEW2

* fix(web): close cross-domain rule gaps + wire --check into CI

Addresses Codex P1/P2, vex-bot, and Devin feedback on #31379.

Gaps closed in the lint rule:
- Barrel form `@/domains/<x>` (no trailing slash) is now caught.
- Side-effect imports `import "@/domains/<x>/..."` are caught.
- `export * from "@/domains/<x>/..."` is caught.
- Relative paths that resolve into another domain
  (`../../<x>/foo.js`) are caught — resolves the path against
  the importer's location and checks the resulting domain.

Gaps closed in the audit script:
- Snapshot generator now matches the same four import forms the
  rule does. Source-of-truth for matching extracted to
  `eslint-rules/cross-domain-matchers.mjs`; both the rule and
  the audit script import from it. This makes future drift
  structurally impossible.

CI gate wired up:
- New `audit:cross-domain:check` package script runs the audit
  in --check mode (exit 1 if the allow-list is stale).
- Both `pr-web.yaml` and `ci-main-web.yaml` run it as a step
  after Lint. Prevents stale allow-list entries from
  accumulating when contributors remove a violation but forget
  to regenerate the snapshot.

Tests expanded from 6 → 12 cases. Allow-list is unchanged
(148 imports) — the new matchers add detection capability but
the codebase has no instances of barrel, side-effect, or
relative cross-domain imports today, so existing snapshot still
covers all current violations.

Verified:
- `bun run audit:cross-domain` produces the same 148-entry snapshot.
- `bun run audit:cross-domain:check` passes (no drift).
- `bunx eslint src` passes.
- 12/12 rule tests pass.

https://claude.ai/code/session_01JPGu4yGPTL4JoLaPuqpEW2

* ci(web): surface audit:check diagnostic output on failure

Lint job is failing in CI but passes locally. Adds diagnostic
output to the audit:cross-domain:check step: if the check
fails, regenerate the snapshot, print the diff, and emit a file
count. This is temporary — will be reverted once the underlying
cause is identified.

https://claude.ai/code/session_01JPGu4yGPTL4JoLaPuqpEW2

* chore(web): handle TSImportType + add richer lint diagnostic

- Rule now visits `TSImportType` so type-position imports like
  `type T = import("@/domains/x/y").Z` are caught.
- 2 new tests cover same-domain (valid) and cross-domain (invalid)
  TSImportType cases.
- Diagnostic in pr-web.yaml's Lint step: on non-zero exit, dump
  node/bun/eslint versions, generated/ status, file count, and
  re-run with --max-warnings 0 to surface anything ESLint flagged
  as a warning. Temporary — reverted once root cause is found.

Addresses Codex P2 #3 (TSImportType not handled).

https://claude.ai/code/session_01JPGu4yGPTL4JoLaPuqpEW2

* docs(web): rewrite cross-domain rule with reasoning, drop external-method name-drops

The previous wording leaned on bulletproof-react and Feature-Sliced
Design as authority for the rule. Reworked to lead with the actual
reason we want this — each domain folder is supposed to be a
self-contained feature, and cross-feature imports create hidden
couplings that make the codebase harder to refactor — and demoted
bulletproof-react to a single "for further reading" link.
Feature-Sliced Design was removed (lower recognition in the
English-speaking React community, no need to ask OSS contributors
to research a second methodology).

Also removed the Linear LUM-1753 link from the README and the rule
header — those aren't accessible to external contributors and
weren't load-bearing for understanding what the rule does.

No behavior change. Allow-list snapshot and tests unchanged
(14/14 pass).

https://claude.ai/code/session_01JPGu4yGPTL4JoLaPuqpEW2

* fix(web): rebase on main, regenerate allow-list for moved files

Main moved 7 conversation-related files from src/domains/chat/
to src/domains/conversations/ (LUM-1754, #31375) while this PR
was in flight. The on-disk allow-list still pointed at the old
chat/ paths, so on this branch the rule fired on the new
conversations/ paths and the existing cross-domain imports
showed up as "new" violations. Rebased onto main and regenerated
the snapshot — 109 files / 148 imports unchanged, just keyed to
the new paths.

Also cleaned up the temporary Lint-step diagnostic now that the
root cause is known. Kept the audit-allowlist diagnostic
(non-temporary) — it's actually useful when a contributor
rebases onto a main that moved files: the failing CI step now
prints the regenerated diff so they can see exactly what
changed and decide whether to accept the regen.

https://claude.ai/code/session_01JPGu4yGPTL4JoLaPuqpEW2

---------

Co-authored-by: Claude <noreply@anthropic.com>
vellum-apollo-bot Bot added a commit that referenced this pull request May 26, 2026
Root cause of DB ghost #3 (May 25): ES module imports at the top of
`test-preload.ts` execute before the `process.env.VELLUM_WORKSPACE_DIR =
testDir` assignment. When `resetDb` transitively required
`drizzle-orm/bun-sqlite` and the worktree's `node_modules` symlink was
broken, the import threw — and the env override line never ran. The
parent daemon's `VELLUM_WORKSPACE_DIR=/workspace` stayed in effect, and
a downstream migration test's `rmSync(getDbPath())` unlinked the live
production DB. Full forensics:
`/workspace/journal/2026-05-25-db-ghost-3-recovery.md`.

Fix
---
Restructure the preload so the env override runs unconditionally before
any source-module import resolves:

1. Only node stdlib (`node:fs`, `node:os`, `node:path`) and `bun:test`
   are imported at the top of the file. These are builtins — they can't
   fail-by-import the way user-code imports can.

2. The env override (`mkdtempSync` + `process.env.VELLUM_WORKSPACE_DIR =
   testDir`) moves to the top of the script body.

3. The four source-module imports become dynamic `await import()` calls
   below the env override. Bun preloads support top-level await, so the
   test runner blocks until the preload module evaluation completes
   (including all dynamic imports) before any test file loads.

If any of the dynamic imports throws, the failure surfaces with
`VELLUM_WORKSPACE_DIR` already redirected to the temp dir — so any
subsequent test code that called `getDbPath()` would resolve to the
temp dir, never `/workspace`. Same guarantee for test files' own
top-level imports: by the time a test file starts loading, the preload
has already executed and the env is set.

Test plan
---------
- All 12 platform.test.ts tests pass (env override path).
- All 12 migration tests pass (db-conversation-fork-lineage,
  db-conversation-inference-profile, db-llm-request-log-provider).
- 92 tests across credential-broker, credential-metadata-store,
  assistant-feature-flag-guard, platform pass — exercises every setup
  hook in the new preload (`_setStorePath`, `_setOverridesForTesting`,
  `installGatewayIpcMock`, `resetDb`).
- 26 tests across db-maintenance, backup-routes pass.
- Lint + typecheck clean.

The safety property is structural: env override on line ~36, dynamic
imports start at line ~53. ES module semantics guarantee top-to-bottom
execution. No behavior-level test is needed to prove the property — it
falls out of the code structure.
vellum-apollo-bot Bot added a commit that referenced this pull request May 26, 2026
…e alternatives

The preload's prior shape called four functions on source modules to set
up test state. Three of those source modules transitively imported
`node_modules` (drizzle, pino, zod) — a broken `node_modules` symlink
could prevent the preload from running and let the live DB env state
leak through (the DB ghost #3 failure shape).

Round 2 of feedback: instead of deferring those imports dynamically,
eliminate them entirely by achieving the same effect with node-module-
free mechanisms.

Changes
-------

1. `_setOverridesForTesting({})` removed. The gateway IPC mock now
   returns `{ __test_default__: false }` as the default for
   `get_feature_flags` so `initFeatureFlagOverrides()` does not enter
   its 7.75 s empty-result retry loop. Tests that need specific flag
   state continue to call `mockGatewayIpc()` or `_setOverridesForTesting()`
   directly.

2. `resetDb()` removed. Was a no-op at preload time: bun test processes
   start with a fresh JS heap, so the db-connection.ts singleton is
   `null`. The first test to call `getDb()` lazily opens a connection
   pointing at the already-set `VELLUM_WORKSPACE_DIR`.

3. `_setStorePath(join(testDir, 'keys.enc'))` removed by restructuring
   the testDir layout. testDir is now `<tmpRoot>/workspace` so
   `vellumRoot()` (which derives from `dirname(VELLUM_WORKSPACE_DIR)`)
   resolves to `<tmpRoot>` per process — `getProtectedDir()` naturally
   isolates per-process at `<tmpRoot>/protected`.

Result: the preload's only static imports are node stdlib, `bun:test`,
and `./mock-gateway-ipc.js` (which is itself stdlib + `bun:test` only).
Zero source-module imports anywhere in the preload chain. A broken
`node_modules` symlink can no longer prevent the env override from
running.
vellum-apollo-bot Bot pushed a commit that referenced this pull request May 26, 2026
…tinel

Dropping the eager source imports from the preload (the ATL-709 invariant)
also dropped the implicit warm-up of common shared modules. Test files that
partial-mock those modules — e.g.

    mock.module("../util/logger.js", () => ({ getLogger }))

— rely on the REAL `logger.js` already being cached when other modules in
the test's dep tree later read named exports the partial mock omits
(`getCliLogger`, `getSqlite`, `getSignalsDir`, …). Bun's `mock.module()`
only redirects FUTURE module loads; modules already in the cache keep
their original exports, which is what made the old preload's static
imports of db-connection / encrypted-store / assistant-feature-flags
work as warm-up.

Restore the warm-up via dynamic imports AFTER the env override. If the
warm-up throws (e.g. DB ghost #3 with a broken `node_modules` symlink),
VELLUM_WORKSPACE_DIR is already set and the production DB stays safe;
individual test files will fail their own imports rather than silently
corrupting data.

Also: the gateway IPC mock now returns a sentinel for `get_feature_flags`
to short-circuit `initFeatureFlagOverrides()`'s 7.75 s empty-result retry
loop, which broke a unit test that exercises the underlying-undefined
path in `ipcGetFeatureFlags()`. Update that test to opt out of the
sentinel explicitly via `mockGatewayIpc(null, { results: { get_feature_flags: undefined } })`.

Fixes the 16 unrelated-looking test failures on this PR's CI run.
vellum-apollo-bot Bot added a commit that referenced this pull request May 26, 2026
Vargas pushback on previous attempts: the issue is architectural, not
just deferred imports. `_setOverridesForTesting`, `resetDb`, and
`_setStorePath` are test-only backdoors that shouldn't have lived in
production source modules to begin with.

This commit:
- Extracts the underlying state of each helper to its own stdlib-only
  module so test code can manipulate it without dragging the production
  module's heavy dependencies (pino, drizzle) through the preload:
    - `src/config/feature-flag-cache.ts` (cachedOverrides + fromGateway)
    - `src/memory/db-singleton.ts` (DB handle + close callback as unknown)
    - `src/security/store-path-override.ts` (storePath + storeKeyPath)
- Deletes `_setOverridesForTesting`, `_setStorePath`, `_setStoreKeyPath`
  from production modules entirely.
- Renames `resetDb` (in `db-connection.ts`) to `closeAssistantDb` —
  legitimate production callers (migration-routes, vbundle-streaming-
  importer, backup/restore, daemon/shutdown-handlers) keep working under
  the new name.
- Adds three test-helper modules in `__tests__/` exposing the test-only
  API surface: `setOverridesForTesting`, `resetDbForTesting`,
  `setStorePathForTesting`, `setStoreKeyPathForTesting`.
- Updates ~50 test files to import the new helpers.

Result: `test-preload.ts` no longer needs ANY warm-up imports of source
modules. Phase 3 of the preload (the `Promise.all([... dynamic imports]))`
block) is gone entirely. The preload imports only node stdlib, bun:test,
and `./mock-gateway-ipc.js` (which is itself stdlib + bun:test). The
DB ghost #3 failure shape (broken node_modules → preload throws →
env override never runs) is now structurally impossible.

The partial-mock test breakage discovered in round 2 (~15 tests doing
mock.module with missing exports) is a separate, pre-existing concern
that follows from removing the implicit warm-up the old preload provided
via its static source imports. Addressed in a follow-up if needed.
vellum-apollo-bot Bot added a commit that referenced this pull request May 26, 2026
Addresses Vargas's CHANGES_REQUESTED review on PR #32091:

1. Test helpers in __tests__/ no longer import from src/. Both sides
   read/write state through a shared globalThis.vellumAssistant
   namespace with locally-declared, typed slot shapes:
     - src/memory/db-singleton.ts          ↔ __tests__/db-test-helpers.ts
     - src/config/feature-flag-cache.ts    ↔ __tests__/feature-flag-test-helpers.ts
     - src/security/store-path-override.ts ↔ __tests__/encrypted-store-test-helpers.ts

   Each side declares its own slot type — duplicated by design, must
   stay in sync. This is the price for keeping the helpers off the
   production import graph (DB ghost #3 protection).

2. Revert the resetDb → closeAssistantDb rename. Per Vargas:
   'those call sites should stay the same. We are just updating how
   some singletons are accessed.' Production callers (backup/restore,
   daemon/shutdown-handlers, vbundle-streaming-importer, migration-routes)
   keep importing resetDb; the function now delegates to clearStoredDb().

3. Apply Vargas's docstring suggestion verbatim on test-preload.ts —
   replaces the wordy 'No setup helpers in the production hot path'
   section with the tighter 'helpers in this same directory / Importing
   from the assistant directly runs the risk' framing.
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