Skip to content

feat: initialize Next.js app in /web directory#1

Merged
dvargasfuertes merged 6 commits into
mainfrom
feat/initial-nextjs-setup
Feb 7, 2026
Merged

feat: initialize Next.js app in /web directory#1
dvargasfuertes merged 6 commits into
mainfrom
feat/initial-nextjs-setup

Conversation

@vargas-jr
Copy link
Copy Markdown

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

Summary

Initial Next.js setup extracted from velly-prototype, organized as a monorepo.

Structure

/
├── web/               # Next.js application
│   ├── src/
│   │   ├── app/       # App router pages & API routes
│   │   ├── components/
│   │   └── lib/       # DB, auth, GCP utilities
│   ├── public/
│   └── package.json
├── editor-templates/  # Agent editor templates
└── .github/           # CI workflows

Changes from velly-prototype

  • ✅ Replaced Convex with Drizzle ORM + PostgreSQL
  • ✅ Removed Vercel deployment (deployment TBD)
  • ✅ Updated branding to Vellum Assistant
  • ✅ Simplified auth (placeholder for now)
  • ✅ Reorganized into monorepo structure with /web directory

Tech Stack

  • Next.js 16 (App Router)
  • Tailwind CSS v4
  • Drizzle ORM + PostgreSQL
  • TypeScript

Getting Started

cd web
npm install
npm run dev

Open with Devin

vargas-jr Bot added 5 commits February 7, 2026 21:58
Migrated all Next.js portions from velly-prototype including:

- App Router pages and API routes
- Components (Layout, RightPanel, etc.)
- Database migrations and lib
- GCP integration (Compute, Storage)
- Anthropic AI integration
- Editor templates
- GitHub Actions workflows
- Tailwind CSS v4 setup

Ready for deployment on Vercel.
- Removed vercel.json
- Removed @vercel/functions dependency
- Removed waitUntil usage (now awaits directly)
- Updated APP_URL fallbacks to localhost
- Removed vercel.svg
- Updated README
- Replaced @neondatabase/serverless with drizzle-orm + postgres
- Added Drizzle schema (src/lib/schema.ts) with typed tables
- Added drizzle.config.ts for migrations
- Updated db.ts with both Drizzle and raw SQL compatibility
- Removed old SQL migration files (now using Drizzle)
- Updated GCS prefix from velly-prototype to vellum-assistant
- Updated README and .env.example
- Updated package.json scripts for Drizzle
Reorganized into monorepo structure:
- /web - Next.js application
- /editor-templates - Agent editor templates
- / - Root config and CI
@vargas-jr vargas-jr Bot changed the title feat: initialize Next.js app from velly-prototype feat: initialize Next.js app in /web directory Feb 7, 2026
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 6 potential issues.

View 7 additional findings in Devin Review.

Open in Devin Review

Comment thread web/src/lib/db.ts
Comment on lines +36 to +39
// Legacy compatibility - getDb returns the drizzle instance
export function getDb() {
return db;
}
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.

🔴 Duplicate getDb() function definitions return different types, breaking all raw SQL callers

The db.ts file defines getDb() twice: once at line 22 returning the raw postgres SQL client, and again at line 37 returning the Drizzle ORM instance. The second definition shadows the first.

Root Cause and Impact

The first getDb() (line 22-24) returns the raw sql postgres client that supports template tag queries like sql\SELECT * FROM agents`. The second getDb()(line 37-39) returns the Drizzledb` instance, which has a completely different API.

Every API route in the codebase calls getDb() and uses it with template tag syntax, e.g.:

const sql = getDb();
const result = await sql`SELECT * FROM agents WHERE id = ${id}`;

If the second definition takes precedence (as it would in JS), the Drizzle instance does NOT support template tag queries, causing all these API routes to fail at runtime. This affects web/src/app/api/agents/route.ts:75, web/src/app/api/agents/[id]/route.ts:11, and virtually every other API route.

Impact: All API endpoints that use getDb() with raw SQL template tags will crash at runtime.

Suggested change
// Legacy compatibility - getDb returns the drizzle instance
export function getDb() {
return db;
}
// Legacy compatibility - getDb returns the raw SQL client
// (duplicate removed - see line 22)
Open in Devin Review

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

Comment on lines +82 to +98
for (const line of lines) {
if (line.startsWith("event: ")) {
const eventType = line.slice(7);
const dataLineIndex = lines.indexOf(line) + 1;
const dataLine = lines[dataLineIndex];
if (dataLine?.startsWith("data: ")) {
const data = JSON.parse(dataLine.slice(6));
if (eventType === "progress") {
setProgressMessage(data.message);
} else if (eventType === "complete") {
router.push(`/agents/${data.agent.id}`);
return;
} else if (eventType === "error") {
throw new Error(data.message);
}
}
}
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.

🔴 SSE parser uses indexOf on iterated array, causing wrong data line lookup for duplicate event types

The SSE parsing logic in handleCreateAgent uses lines.indexOf(line) to find the index of the current event line, then looks up the next line as the data line. indexOf returns the index of the first occurrence, not the current one.

Detailed Explanation

At web/src/app/agents/page.tsx:85, the code does:

const dataLineIndex = lines.indexOf(line) + 1;
const dataLine = lines[dataLineIndex];

When the SSE stream contains multiple events of the same type (e.g., multiple event: progress lines), lines.indexOf(line) always returns the index of the first event: progress line in the lines array, not the current one being iterated. This means subsequent progress events will read the data from the first progress event's data line instead of their own.

For example, if lines contains:

["event: progress", "data: {step:naming}", "", "event: progress", "data: {step:upload}", ""]

When iterating over the second event: progress (index 3), indexOf returns 0, so dataLineIndex = 1, reading {step:naming} instead of {step:upload}.

Impact: Progress messages during agent creation may show stale/incorrect status, and the "complete" event could be missed if a prior event line has the same text.

Suggested change
for (const line of lines) {
if (line.startsWith("event: ")) {
const eventType = line.slice(7);
const dataLineIndex = lines.indexOf(line) + 1;
const dataLine = lines[dataLineIndex];
if (dataLine?.startsWith("data: ")) {
const data = JSON.parse(dataLine.slice(6));
if (eventType === "progress") {
setProgressMessage(data.message);
} else if (eventType === "complete") {
router.push(`/agents/${data.agent.id}`);
return;
} else if (eventType === "error") {
throw new Error(data.message);
}
}
}
let currentEventType: string | null = null;
for (const line of lines) {
if (line.startsWith("event: ")) {
currentEventType = line.slice(7);
} else if (line.startsWith("data: ") && currentEventType) {
const data = JSON.parse(line.slice(6));
if (currentEventType === "progress") {
setProgressMessage(data.message);
} else if (currentEventType === "complete") {
router.push(`/agents/${data.agent.id}`);
return;
} else if (currentEventType === "error") {
throw new Error(data.message);
}
currentEventType = null;
} else if (line === "") {
currentEventType = null;
}
}
Open in Devin Review

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

Comment on lines +69 to +75
await createChatMessage({
agent_id: agentId,
role: "assistant",
content: outboxMsg.content,
status: "delivered",
gcs_message_id: outboxMsg.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.

🔴 createChatMessage called with snake_case keys but Drizzle schema expects camelCase

The createChatMessage function uses Drizzle ORM's insert().values() which expects keys matching the schema's camelCase property names, but callers pass snake_case keys.

Root Cause and Impact

The Drizzle schema at web/src/lib/schema.ts:19 defines agentId (camelCase) mapped to column agent_id, and gcsMessageId mapped to gcs_message_id. The NewChatMessage type inferred from this schema expects camelCase keys.

However, at web/src/app/api/agents/[id]/messages/route.ts:69-75 and route.ts:188-194, the callers pass:

await createChatMessage({
  agent_id: agentId,      // should be agentId
  role: "assistant",
  content: outboxMsg.content,
  status: "delivered",
  gcs_message_id: outboxMsg.id,  // should be gcsMessageId
});

Drizzle will not recognize agent_id or gcs_message_id as valid column references. The agent_id column is NOT NULL, so the insert will either fail with a database constraint violation (missing required agent_id) or silently insert NULL values.

Impact: All message creation (both user messages and synced assistant outbox messages) will fail, breaking the chat functionality entirely.

Suggested change
await createChatMessage({
agent_id: agentId,
role: "assistant",
content: outboxMsg.content,
status: "delivered",
gcs_message_id: outboxMsg.id,
});
await createChatMessage({
agentId: agentId,
role: "assistant",
content: outboxMsg.content,
status: "delivered",
gcsMessageId: outboxMsg.id,
});
Open in Devin Review

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

Comment on lines +188 to +194
const dbMessage = await createChatMessage({
agent_id: agentId,
role: "user",
content,
status: "sent",
gcs_message_id: agentData.messageId,
});
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.

🔴 Second createChatMessage call also uses snake_case keys

Same issue as BUG-0003 but for the user message creation path at line 188-194.

Root Cause

At web/src/app/api/agents/[id]/messages/route.ts:188-194:

const dbMessage = await createChatMessage({
  agent_id: agentId,       // should be agentId
  role: "user",
  content,
  status: "sent",
  gcs_message_id: agentData.messageId,  // should be gcsMessageId
});

This will fail for the same reason as BUG-0003 - Drizzle expects camelCase property names.

Impact: Sending messages to agents will fail.

Suggested change
const dbMessage = await createChatMessage({
agent_id: agentId,
role: "user",
content,
status: "sent",
gcs_message_id: agentData.messageId,
});
const dbMessage = await createChatMessage({
agentId: agentId,
role: "user",
content,
status: "sent",
gcsMessageId: agentData.messageId,
});
Open in Devin Review

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


// Obfuscate .env file contents
const filename = path.split("/").pop() || "";
if (filename === ".env" || filename.endsWith(".env")) {
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.

🔴 .env obfuscation bypass for files like .env.local, .env.production

The env file detection logic fails to catch common env file variants that also contain secrets.

Root Cause

At web/src/app/api/agents/[id]/cat/route.ts:60:

if (filename === ".env" || filename.endsWith(".env")) {

This only matches files named exactly .env or files ending with .env. It does NOT match common variants like .env.local, .env.production, .env.development, etc., because those end with .local, .production, etc.

A user browsing the file system can read the full unobfuscated contents of .env.local or .env.production files, which typically contain the same sensitive secrets as .env.

Impact: Secret values in .env.* variant files are exposed in plaintext through the file viewer API.

Suggested change
if (filename === ".env" || filename.endsWith(".env")) {
if (filename === ".env" || filename.startsWith(".env.") || filename.endsWith(".env")) {
Open in Devin Review

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

Comment on lines +68 to +71
if (line.startsWith('event: ')) {
const eventType = line.slice(7);
continue;
}
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.

🔴 FocusView SSE parser discards eventType due to continue statement, never processes events

The SSE parser in FocusView.tsx reads the event type but immediately continues, so the event type is never used to determine how to handle the data.

Root Cause

At web/src/components/AssistantInitialization/FocusView.tsx:68-71:

if (line.startsWith('event: ')) {
  const eventType = line.slice(7);
  continue;  // <-- skips to next iteration
}

The eventType variable is declared inside the if block and immediately discarded by continue. The subsequent if (line.startsWith('data: ')) block processes data lines but has no access to the event type. This means the parser cannot distinguish between progress, complete, and error events.

The code partially works because it checks data.step and data.agent properties to infer the event type, but it never detects error events properly. If the server sends an error event, the data line will be processed but the error message check at line 105 (data.message.includes('error')) only logs it - it doesn't set the error state or stop processing.

Impact: Error events from the agent creation SSE stream are silently ignored. The user sees a perpetual loading state instead of an error message.

Prompt for agents
In web/src/components/AssistantInitialization/FocusView.tsx, the SSE parser at lines 67-111 needs to be restructured to properly track the event type across lines. Currently, the event type is read at line 69 but immediately discarded by the continue statement at line 70. The fix should:

1. Declare a variable outside the for loop (e.g., let currentEventType: string | null = null) to track the current event type.
2. When an event: line is encountered, store the event type in that variable (don't continue).
3. When a data: line is encountered, check currentEventType to determine how to handle it.
4. For 'error' events, call setError(data.message) to show the error UI.
5. For 'complete' events, extract the agent ID.
6. Reset currentEventType after processing a data line or on empty lines.
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
…riction

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>
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 14, 2026
Fix issues #1-3 from manual code review:

1. **Eliminate duplicated socket path resolution logic**: Extract
   resolveSocketPath() as a standalone function at module level.
   DaemonConfig.default now delegates to it (3 lines vs 14).
   DaemonClient.resolveSocketPath() also delegates for backward
   compatibility with existing tests.

2. **Handle empty hostname from UserDefaults**: Use .flatMap to treat
   empty string as nil, ensuring fallback to "localhost". Prevents
   connection attempts to empty hostname if user sets daemon_hostname="".

3. **Add @mainactor to iOS AppDelegate**: Consistent with macOS
   AppDelegate and required for strict concurrency checking since
   DaemonClient is @MainActor-isolated.

Issues #4-6 acknowledged but not addressed in this PR:
- #4: Silently ignored connection errors (acceptable for scaffold)
- #5: iOS code under macos/ directory (can refactor later if needed)
- #6: Platform-specific DaemonConfig (intentional design tradeoff)

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
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>
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>
siddseethepalli added a commit that referenced this pull request May 10, 2026
…0182)

Three bugs combine to make the Re-pair menu action unable to recover a
server-side-revoked refresh token, leaving the user stuck in
"Authentication failed" until they manually inject credentials:

1. forceReBootstrap deleted only the active env's guardian-token.json on
   remote hatches and preserved the file on local hatches. The CLI's
   seedGuardianTokenFromSiblingEnv (cli/src/lib/guardian-token.ts) then
   restored a stale token from a sibling env on the next vellum wake —
   refresh-window expiry is a clock check, not a server-validity check,
   so revoked-but-not-expired tokens silently re-armed.

2. performInitialBootstrap's HTTP retry loop awaited
   connectionManager.isConnected before calling /v1/guardian/init.
   isConnected only flips true after a 200 health-check response, which
   requires the credential the loop is trying to mint. No auth →
   no isConnected → no bootstrap → no auth.

3. forceReBootstrap passed skipFileImport=false on local hatches with
   the comment "preserving guardian token file as the only recovery
   artifact" — combined with #1, this re-imported the just-restored
   stale token.

Fixes:
- forceReBootstrap now sweeps guardian-token.json across every
  VellumEnvironment config dir via the new
  GuardianTokenFileReader.deleteTokenFileAcrossAllEnvs helper, and
  always passes skipFileImport=true.
- HTTP retry loop drops the isConnected gate; bootstrapActorToken
  surfaces gateway-down errors via its own return value.
- Adds VellumPaths.allEnvs(...) factory and CaseIterable conformance
  on VellumEnvironment so the sweep can iterate all env config dirs.

Tests cover the new helper across sibling-env layouts and ensure
unrelated assistants are untouched.

Co-authored-by: Vellum Assistant <assistant@vellum.ai>
noanflaherty pushed a commit that referenced this pull request May 10, 2026
…COMMAND_INVENTORY (#30159)

* feat(cli): registerCommand helper + transport tags for 21 commands + §3.7 daemon-down message (#30156)

* feat(cli): registerCommand helper + transport tags for 21 commands + §3.7 daemon-down message

* fix(lint): sort imports in cache.ts, clients.ts, register-command.test.ts

* fix(tests): use console.log in status.ts; update exit-helper test for §3.7 message

* fix(tests): use process.stdout.write in status.ts for testable output

* fix: revert daemon terminology per AGENTS.md; use 'the assistant' in user-facing messages; 'Assistant: running/down' in status

---------

Co-authored-by: credence-the-bot[bot] <test@test.com>

* feat(cli): advisory ESLint rule cli/no-daemon-internals (PR #30157)

* feat(cli): advisory ESLint rule cli/no-daemon-internals

* fix(lint): convert ESLint rule to plain JS so Node.js ESM loader can find it

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

---------

Co-authored-by: credence-the-bot[bot] <test@test.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

* feat(cli): COMMAND_INVENTORY.md + bidirectional CI check + AGENTS.md canonical example (PR #30158)

* feat(cli): COMMAND_INVENTORY.md + bidirectional CI check + AGENTS.md canonical example

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

* fix(lint): sort named imports alphabetically in check-cli-inventory.ts

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

* fix(lint): correct named import sort order in check-cli-inventory.ts

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

---------

Co-authored-by: credence-the-bot[bot] <test@test.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(cli): address Codex + Devin findings on Section B rollup

Cycle 1 fixes for PR #30159 review feedback:

1. Allowlist gap (Codex P2 / Devin 🟡):
   Add `../logger` and `../output` to `local` and `bootstrap` allowlists
   in `cli-no-daemon-internals.js`. Without this, every newly tagged
   `local` command (autonomy, config, completions, keys,
   credential-execution) false-positives on its standard logger/output
   imports, killing the rule's signal-to-noise ratio.

2. Doc reference (Devin 🟡):
   `forbiddenImport` error message pointed to nonexistent `DESIGN.md
   §3.1`. Now points to `src/cli/AGENTS.md` which is the actual shipped
   reference.

3. Target glob too broad (Codex P2):
   The rule fired `missingTransport` on every file under
   `src/cli/commands/**/*.ts` including helper modules like
   `oauth/shared.ts` that aren't command registrars. Narrowed by making
   the rule skip files that don't call `registerCommand` directly (now a
   tri-state `findTransport`: `string` for tagged, `MISSING_TRANSPORT`
   for registrars without transport prop, `null` for non-registrars).
   Helper modules pass silently. Backdoor closed because helpers can't
   be imported via blessed paths anyway (sibling `./helper.js` doesn't
   match any allowlist prefix).

4. Type-only imports (extends Codex P2 #1):
   `import type {...}` is erased at compile time and doesn't constitute
   a runtime boundary violation. Rule now skips type-only imports.
   Drops 3 false-positives on `memory-v2.ts` and `mcp.ts` type imports
   from runtime/routes/.

5. Canonical-example claim (Devin 🟡):
   `AGENTS.md` claimed `pending.ts` uses `exitFromIpcResult` but it
   didn't. Migrated `pending.ts` to actually use the helper added in
   Section A. Now matches the doc.

6. mcp.ts misclassification (Devin 🚩):
   `mcp.ts` was marked `THIN` in COMMAND_INVENTORY but imports
   `mcp/client.js`, `mcp/mcp-oauth-provider.js`, `util/browser.js` —
   actual daemon internals. Reclassified to `LEGACY` with a note that
   Section C/D will thin it. Until then the advisory rule will warn on
   this file (and that's the right signal).

Lint: 18 warnings remain (down from 21) on partially-migrated commands
(config, keys, credential-execution, status, task, mcp). All are
legitimate signals — those files have non-allowlisted imports that
need migration before Section J flips the rule from advisory → error.

Tests: rule test suite extended with helper-module + type-only +
local-with-logger regression cases; 5/5 manual smoke tests pass; tsc
clean; lint:inventory passes.

* fix(eslint-rule): cycle-1 review findings — chained registerCommand + inline-type imports

Codex P2 (cli-no-daemon-internals.js:88) — traverse callees:
The AST walk only enqueued CallExpression.arguments, never the callee.
Chained patterns like `registerCommand({...}).command(...).description(...)`
were treated as if no registerCommand call existed (findTransport returned
null), silently skipping all checks for those command-entry files.
Fix: enqueue node.callee on CallExpression and node.object on
MemberExpression so the walk reaches through the receiver chain.

Codex P2 (cli-no-daemon-internals.js:183) — inline type-only specifiers:
The rule only checked importNode.importKind === "type", which catches
`import type {...}` but not `import { type X }` — for the inline form
the declaration's importKind stays "value" and each ImportSpecifier
carries its own importKind. All-type-only inline imports are erased at
compile time and must not flag forbidden-import.
Fix: also skip when every specifier is an ImportSpecifier with
importKind === "type". Side-effect-only imports (no specifiers) and
mixed value+type imports still get linted.

* fix(cli): status — narrow zero-exit fallback to daemon-unreachable cases (cycle-3)

---------

Co-authored-by: credence-the-bot[bot] <277301654+credence-the-bot[bot]@users.noreply.github.com>
Co-authored-by: credence-the-bot[bot] <test@test.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: credence-the-bot <195577519+credence-the-bot@users.noreply.github.com>
Co-authored-by: credence-the-bot <226213748+credence-the-bot@users.noreply.github.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>
credence-the-bot Bot pushed a commit that referenced this pull request May 11, 2026
…_connection in route

Codex flagged two P1s on the original PR:

1. **Picker hidden for stale bindings.** When a profile's
   `providerConnection` referenced a now-disabled or deleted connection
   AND the workspace had no active connections for that provider, the
   editor hid the entire Connection field. The user had no way to clear
   the broken binding from the UI, and the profile remained pinned to an
   invalid name. Refactor: visibility now gates on (active matches OR
   non-empty saved binding); when stale, the picker shows a
   '<name> (not found)' option so the trigger renders the bound name,
   and the Not Found badge surfaces the state. Selecting 'Any active …'
   clears the binding back to the daemon's first-active fallback.

2. **`provider_connection` not in `INFERENCE_PROFILE_UI_KEYS`.** The
   profile-replace route only clears keys listed in that set, so
   omitting `provider_connection` from the body (which is what the UI
   does when the user picks 'Any active') silently retained the old
   server-side binding. Added `provider_connection` so the route now
   correctly wipes the stored value when the UI clears it.

Tests:
- 5 new InferenceProfileEditorTests covering staleProviderConnection
  truth table (binding missing / disabled / matches / nil / empty) +
  body-builds-with-stale.
- 2 new conversation-query-routes tests covering write + clear paths
  for `provider_connection` on PUT /v1/config/llm/profiles/:name.

13/13 daemon tests pass; assistant tsc + eslint clean.

Addresses Codex P1 #1 + P1 #2 from `@codex review` on `f0375fae54`.
noanflaherty pushed a commit that referenced this pull request May 11, 2026
* feat(macos): per-profile provider-connection picker

Closes audit finding #5 (profile editor cannot bind to a specific
provider connection). The daemon schema already accepts
`provider_connection` on `ProfileEntry` (see
`assistant/src/config/schemas/llm.ts`); this PR plumbs the field
through the macOS model + editor.

UX:
- New "Connection" sub-dropdown renders below Provider when the
  selected provider has at least one active connection. First option
  is "Any active <provider> connection" (value=""), preserving the
  daemon's legacy first-active-wins behavior. Subsequent options are
  the active connections, labelled by `label` (fallback: `name`).
- Changing the Provider clears the connection binding so a stale
  name never survives a provider switch.
- A "Not found" warning badge surfaces when the saved binding no
  longer matches any active connection (most often: a connection was
  disabled or deleted outside the editor).

Wire shape: snake_case `provider_connection` for the JSON key (matches
the Zod schema); camelCase `providerConnection` for the Swift
property. `preservedJSON` exclusion list updated so the snake_case
key doesn't double-write.

Connections are fetched once by `InferenceProfilesSheet.task` and
re-fetched when the editor closes (in case a connection was added in
another surface during the session). Tolerant on transport failure
(stale list preserved) — same posture as `SettingsStore.providerKeys`.

Tests:
- 4 new `InferenceProfileTests` cases cover wire-shape round-trip,
  empty-string decode, merging-from-fragment, merging-preserves.
- 5 new `InferenceProfileEditorTests` cases cover the per-provider
  filter, empty-state behaviors, the display-name helper, and the
  full body construction with a populated `connections` list.
- `InferenceProfilesSheetTests` updated to inject the mock client.

No daemon changes required.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

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

* fix(profile-picker): keep visible for stale bindings + clear provider_connection in route

Codex flagged two P1s on the original PR:

1. **Picker hidden for stale bindings.** When a profile's
   `providerConnection` referenced a now-disabled or deleted connection
   AND the workspace had no active connections for that provider, the
   editor hid the entire Connection field. The user had no way to clear
   the broken binding from the UI, and the profile remained pinned to an
   invalid name. Refactor: visibility now gates on (active matches OR
   non-empty saved binding); when stale, the picker shows a
   '<name> (not found)' option so the trigger renders the bound name,
   and the Not Found badge surfaces the state. Selecting 'Any active …'
   clears the binding back to the daemon's first-active fallback.

2. **`provider_connection` not in `INFERENCE_PROFILE_UI_KEYS`.** The
   profile-replace route only clears keys listed in that set, so
   omitting `provider_connection` from the body (which is what the UI
   does when the user picks 'Any active') silently retained the old
   server-side binding. Added `provider_connection` so the route now
   correctly wipes the stored value when the UI clears it.

Tests:
- 5 new InferenceProfileEditorTests covering staleProviderConnection
  truth table (binding missing / disabled / matches / nil / empty) +
  body-builds-with-stale.
- 2 new conversation-query-routes tests covering write + clear paths
  for `provider_connection` on PUT /v1/config/llm/profiles/:name.

13/13 daemon tests pass; assistant tsc + eslint clean.

Addresses Codex P1 #1 + P1 #2 from `@codex review` on `f0375fae54`.

---------

Co-authored-by: Credence <credence@vellum.ai>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: credence-the-bot <credence-the-bot@users.noreply.github.com>
noanflaherty pushed a commit that referenced this pull request May 11, 2026
#30330)

* fix(macos): filter provider picker to active connections (iter3 QA #1)

Parity with web PR #6509. The Settings → AI → Profile editor was
listing every provider in the catalog regardless of whether the user
had an active connection for it, so picking one bound the profile to
a route the daemon would skip on dispatch.

`InferenceProfileEditor.availableProviderIds` now:
- Falls back to the full catalog when `connections` is empty (sheet
  is still loading, or an older daemon without the connections API).
- Filters `store.dynamicProviderIds` to providers with at least one
  ACTIVE `ProviderConnection`.
- Always keeps the currently-bound `profile.provider` in the list so
  editing a stale profile (its connection was later disabled) still
  renders a sensible trigger.

Adds an empty-state hint under the picker pointing users to Providers
when no active connections exist, matching the web modal's hint.

4 regression tests covering hide-inactive, stale-binding recovery,
pre-load fallback, and the all-disabled empty case.

* fix(macos): distinguish unloaded from empty connections (Codex P1)

The pre-load fallback in availableProviderIds was `guard !connections.isEmpty`, which conflated two distinct states:

- Connections not yet fetched (`.task` still running, daemon round-trip in flight) → want full catalog so the picker isn't empty during the gap.
- Connections fetched and confirmed empty (fresh workspace, nothing configured) → want filter to yield empty + empty-state hint, otherwise the user can still bind a profile to a non-dispatchable provider — the exact QA trap this PR was supposed to close.

Promote `connections` from `[ProviderConnection] = []` to `[ProviderConnection]? = nil`. `nil` is pre-load. `[]` is loaded-and-empty.

The parent (`InferenceProfilesSheet`) now tracks the same nil-vs-loaded
distinction; `refreshConnections()` continues to only write on a
successful daemon response, so the optional flips nil → [] once we have
ground truth and stays nil through transport failures (stale-but-correct
beats blank).

`availableConnectionsForProvider` falls back to `[]` during pre-load
so the connection sub-dropdown stays hidden until we have data.

Renames `testAvailableProviderIdsFallsBackToFullCatalogWhenConnectionsEmpty` to `...WhenConnectionsAreNil` (the old name was misleading after the type change) and adds `testAvailableProviderIdsIsEmptyWhenConnectionsLoadedButEmpty` to lock in the fresh-workspace case.

Addresses Codex P1 in #30330 (comment).
The web sibling (#6509, already merged) has the same trap; a follow-up
PR will mirror this fix.

---------

Co-authored-by: credence-the-bot <credence-the-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