packages/providers/src/community/pi/event-bridge.ts (1)

69-83: ⚡ Quick win

AsyncQueue should not treat undefined as “buffer empty” (latent generic correctness issue).

AsyncQueue.iterate() uses:

• const next = this.buffer.shift();
• if (next !== undefined) { yield next; ... }

Because buffer.shift() returns T | undefined, this conflates “queue item is actually undefined” with “no buffered item”. Since AsyncQueue is generic, this can become a silent correctness bug if it’s ever used with T that allows undefined in the future.

♻️ Proposed fix

   private async *iterate(): AsyncGenerator<T> {
     while (true) {
-      const next = this.buffer.shift();
-      if (next !== undefined) {
-        yield next;
-        continue;
-      }
+      if (this.buffer.length > 0) {
+        // `shift()` can return undefined only when empty; length guard avoids that.
+        const next = this.buffer.shift() as T;
+        yield next;
+        continue;
+      }
       if (this.closed) return;
       const result = await new Promise<IteratorResult<T>>(resolve => {
         this.waiters.push(resolve);
       });
       if (result.done) return;
       yield result.value;
     }
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/providers/src/community/pi/event-bridge.ts` around lines 69 - 83,
AsyncQueue.iterate treats a shifted undefined value as "empty" which breaks
generic correctness; change the check to test buffer length first (e.g., if
(this.buffer.length > 0) { const next = this.buffer.shift()!; yield next;
continue; }) so actual undefined items are yielded; keep the rest of the loop
(this.closed, waiters promise handling and result.done handling) unchanged and
reference AsyncQueue.iterate, this.buffer, this.waiters, and this.closed when
applying the change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/cli/src/commands/setup.ts (1)

878-899: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Two residual gaps in the default-assistant override branches still exclude pi — partly addressing the concern raised in the previous review.

Bug 1 — Line 897 forces 'codex' when hasPi && hasCodex && !hasClaude:
At line 869–876, defaultAssistant is correctly computed from enabledProviders (which includes 'pi'). For example, if process.env.DEFAULT_AI_ASSISTANT === 'pi' and the user selected codex + pi, the computed value is 'pi'. The unconditional else if (hasCodex && !hasClaude) override on line 897 then clobbers that to 'codex'.

Bug 2 — Lines 879–884 filter to p.builtIn only, excluding pi when all three are selected:
getRegisteredProviders().filter(p => p.builtIn) is evaluated only when hasClaude && hasCodex. If hasPi is also true the select prompt never surfaces 'pi' as a choice.

🐛 Proposed fix — generalise both branches to `enabledProviders`

-  if (hasClaude && hasCodex) {
+  if (enabledProviders.length > 1) {
     const providerChoices = getRegisteredProviders()
-      .filter(p => p.builtIn)
+      .filter(p => enabledProviders.includes(p.id))
       .map(p => ({
         value: p.id,
         label: p.id === 'claude' ? `${p.displayName} (Recommended)` : p.displayName,
       }));

     const defaultChoice = await select({
       message: 'Which should be the default AI assistant?',
       options: providerChoices,
     });

     if (isCancel(defaultChoice)) {
       cancel('Setup cancelled.');
       process.exit(0);
     }

     defaultAssistant = defaultChoice;
-  } else if (hasCodex && !hasClaude) {
-    defaultAssistant = 'codex';
   }

With enabledProviders.length > 1, the single-provider cases (any single selection) skip the prompt entirely and keep defaultAssistant from the already-correct initial computation at line 869. All multi-provider combinations — including hasPi && hasCodex && !hasClaude and hasPi && hasClaude && hasCodex — now show a selection drawn from exactly the enabled set.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/setup.ts` around lines 878 - 899, The
default-assistant logic incorrectly excludes 'pi' by using
getRegisteredProviders().filter(p => p.builtIn) for the multi-provider prompt
and by unconditionally setting defaultAssistant = 'codex' in the hasCodex &&
!hasClaude branch; change both to use the already-computed enabledProviders
list: build providerChoices from enabledProviders (map id/displayName) when
prompting via select(), and remove or replace the unconditional assignment of
'codex' so that defaultAssistant preserves the initial computed value from
enabledProviders (or is set from the user's select() result, guarded by
isCancel()/cancel()). Ensure you reference enabledProviders, defaultAssistant,
getRegisteredProviders(), select(), isCancel(), and cancel() while making these
edits.

packages/core/src/db/conversations.test.ts (1)

72-92: ⚡ Quick win

Tests depend on live loadConfig() — non-deterministic against real config files or future built-in providers

getOrCreateConversation now calls loadConfig() when assistantType is still undefined, but this test file doesn't mock ../config/config-loader. The hardcoded 'claude' expectation in this test (line 90) and falls back to claude when codebase not found (line 167) will silently break if:

• registerBuiltinProviders() inside loadConfig() registers any built-in provider whose ID isn't 'claude' (the first built-in takes priority per getDefaults()), or
• a ~/.archon/config.yaml with assistant: pi (or similar) is present in the test runner's home.

Since loadConfig is importable and only config.assistant is consumed in this path, a module-level mock is straightforward to add. As per coding guidelines, "keep tests deterministic — no flaky timing or network dependence without guardrails."

♻️ Suggested fix — add a module-level mock before the imports

+mock.module('../config/config-loader', () => ({
+  loadConfig: async () => ({ assistant: 'claude' }),
+}));
+
 import {
   getOrCreateConversation,

Note: mock.module() is process-global in Bun and must appear before the module under test is imported; the partial return value is sufficient because only config.assistant is read by getOrCreateConversation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/db/conversations.test.ts` around lines 72 - 92, Tests call
getOrCreateConversation which will call loadConfig() when assistantType is
undefined; the test currently doesn't mock ../config/config-loader so it can
pick up real config and change the expected 'claude' default. Add a module-level
mock for '../config/config-loader' before importing the module under test (use
mock.module() / jest.mock at top of the test file) that returns an object with
config.assistant set to 'claude' (or the expected default), so
getOrCreateConversation and the assertions that expect 'claude' remain
deterministic.

packages/server/src/routes/api.ts (1)

14-14: Consider using named import for js-yaml for consistency

The current default import import jsYaml from 'js-yaml' is valid because esModuleInterop is enabled in tsconfig. However, named imports are more explicit and eliminate any future concerns if esModuleInterop is ever adjusted. This is an optional refactoring:

Suggested improvement

-import jsYaml from 'js-yaml';
+import { dump as jsYamlDump } from 'js-yaml';

Then update the call sites:

-      yamlContent = jsYaml.dump(definition);
+      yamlContent = jsYamlDump(definition);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/server/src/routes/api.ts` at line 14, Change the default import of
the YAML library to a named import to avoid relying on esModuleInterop: replace
the default import of "js-yaml" with a named import (e.g., import { load, dump }
from 'js-yaml') and update all call sites that currently reference
jsYaml.load/jsYaml.dump to use the corresponding named functions (or re-export
them if you prefer). Locate the import in the top of the file where "import
jsYaml from 'js-yaml'" appears and adjust usages throughout this module (calls
to jsYaml.load, jsYaml.dump, etc.) to the chosen named identifiers.

packages/core/src/handlers/clone.ts (1)

49-52: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

DEFAULT_AI_ASSISTANT is still effectively deprioritized and unvalidated (regression).

Line 49-Line 52 still prefers the first built-in provider over process.env.DEFAULT_AI_ASSISTANT, so user-configured pi won’t be selected in common cases. Also, when env is used, it can flow into persistence without provider validation.

Suggested fix

-  const { getRegisteredProviders } = await import('@archon/providers');
-  const defaultProvider =
-    getRegisteredProviders().find(p => p.builtIn)?.id ??
-    process.env.DEFAULT_AI_ASSISTANT ??
-    'claude';
+  const { getRegisteredProviders, isRegisteredProvider } = await import('@archon/providers');
+  const envAssistant = process.env.DEFAULT_AI_ASSISTANT?.trim();
+
+  const defaultProvider = (() => {
+    if (envAssistant) {
+      if (!isRegisteredProvider(envAssistant)) {
+        const available = getRegisteredProviders().map(p => p.id).join(', ');
+        throw new Error(
+          `DEFAULT_AI_ASSISTANT='${envAssistant}' is not registered. Available providers: ${available}`
+        );
+      }
+      return envAssistant;
+    }
+
+    // Intentional fallback order when env is unset.
+    return getRegisteredProviders().find(p => p.builtIn)?.id ?? 'claude';
+  })();

As per coding guidelines, "Prefer throwing early with a clear error for unsupported or unsafe states; never silently swallow errors or broaden permissions; document intentional fallback behavior with a comment."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/handlers/clone.ts` around lines 49 - 52, The current
defaultProvider selection prefers a built-in provider over
process.env.DEFAULT_AI_ASSISTANT and doesn't validate the env value; update the
logic in the defaultProvider initialization (and/or surrounding code that uses
getRegisteredProviders()) to first check process.env.DEFAULT_AI_ASSISTANT, then
validate that its value matches an id returned by getRegisteredProviders(), use
that validated provider id if present, otherwise fall back to the first built-in
or a hardcoded fallback ('claude'); if the env value is set but not recognized,
throw a clear error rather than silently using a different provider so
persistence never receives an unvalidated provider id.

packages/server/src/routes/api.ts (1)

1411-1428: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Conversation bootstrap errors are still swallowed before dispatch.

The past review comment on this block is marked as addressed, but the current code still catches both the lookup and getOrCreateConversation() failures, logs them, and falls through. When conv ends up null, the handler skips persistence at line 1431 and still calls dispatchToOrchestrator(conversationId, …) at line 1467 — accepting work with no DB linkage, so history, titles, and assistant-message persistence are silently lost.

Either bubble the error (return 500) when getOrCreateConversation fails, or retry findConversationByPlatformId once to handle a racing insert and only 500 if that retry still has no row. As per coding guidelines, "Prefer throwing early with a clear error for unsupported or unsafe states; never silently swallow errors or broaden permissions; document intentional fallback behavior with a comment".

🛡️ Suggested fix

     let conv: Awaited<ReturnType<typeof conversationDb.findConversationByPlatformId>> = null;
     try {
       conv = await conversationDb.findConversationByPlatformId(conversationId);
     } catch (e: unknown) {
       getLog().error({ err: e, conversationId }, 'conversation_lookup_failed');
+      return apiError(c, 500, 'Failed to look up conversation');
     }
-    // Auto-create conversation if not found (web chat creates ID but never persists it)
-    if (!conv) {
-      try {
-        conv = await conversationDb.getOrCreateConversation('web', conversationId);
-      } catch (e: unknown) {
-        const err = e as Error;
-        getLog().error({ err, conversationId }, 'conversation_auto_create_failed');
-      }
-    }
+    // Auto-create conversation if not found (web chat creates ID but never persists it)
+    if (!conv) {
+      try {
+        conv = await conversationDb.getOrCreateConversation('web', conversationId);
+      } catch (e: unknown) {
+        // Handle racing insert: re-check before failing.
+        try {
+          conv = await conversationDb.findConversationByPlatformId(conversationId);
+        } catch {
+          conv = null;
+        }
+        if (!conv) {
+          getLog().error({ err: e as Error, conversationId }, 'conversation_auto_create_failed');
+          return apiError(c, 500, 'Failed to persist conversation');
+        }
+      }
+    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/server/src/routes/api.ts` around lines 1411 - 1428, The handler
currently swallows conversation bootstrap errors and proceeds to call
dispatchToOrchestrator with conv==null; change this so that if
conversationDb.getOrCreateConversation throws or conv is null after the first
lookup, retry conversationDb.findConversationByPlatformId once (to handle a
racing insert) and if that retry still returns null, log the error with context
and throw/return a 500 (do not fall through to dispatchToOrchestrator); update
the block around conversationDb.findConversationByPlatformId and
conversationDb.getOrCreateConversation to implement the retry + clear error
throw, and ensure logs include the original error and conversationId for
debugging.

packages/core/src/db/conversations.ts (2)

248-248: ⚡ Quick win

Use the cached getLog() helper instead of re-creating the logger.

Lines 12–15 define getLog() specifically so test mocks can intercept createLogger. Calling createLogger('db.conversations') directly here defeats that pattern (and the rest of the file consistently uses getLog()).

♻️ Proposed change

 export async function updateConversationTitle(id: string, title: string): Promise<void> {
   const dialect = getDialect();
-  const log = createLogger('db.conversations');
   const result = await pool.query(
     `UPDATE remote_agent_conversations SET title = $1, updated_at = ${dialect.now()} WHERE id = $2`,
     [title, id]
   );
   if (result.rowCount === 0) {
-    log.warn({ conversationId: id }, 'update_conversation_title_not_found');
+    getLog().warn({ conversationId: id }, 'db.conversation_title_update_not_found');
   }
 }

The renamed event (db.conversation_title_update_not_found) also matches the {domain}.{action}_{state} convention used by the sibling db.conversation_update_not_found event on line 163. As per coding guidelines, "Use Pino logger factory with event naming format {domain}.{action}_{state}".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/db/conversations.ts` at line 248, Replace the direct call
to createLogger('db.conversations') with the cached logger helper getLog() so
tests can mock the logger consistently; also rename the event string where the
title-update-not-found is emitted to "db.conversation_title_update_not_found" to
match the existing naming convention used by "db.conversation_update_not_found"
and the {domain}.{action}_{state} format. Ensure you update the symbol usage
where createLogger was used to instead call getLog() and replace the old event
name with the new one.

---

86-112: ⚡ Quick win

Add validation for assistantType sourced from database rows.

When assistantType is set from parent.rows[0].ai_assistant_type or codebase.rows[0].ai_assistant_type, it bypasses provider registration validation. The value only reaches isRegisteredProvider() indirectly if it remains undefined and falls back to loadConfig().assistant. A stale database row referencing a removed provider will propagate unchecked. Validate with isRegisteredProvider() and fallback to config.assistant when the stored value is unknown, matching the pattern used in config-loader.ts.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/db/conversations.ts` around lines 86 - 112, Validate
assistantType after pulling from parent.rows[0].ai_assistant_type or
codebase.rows[0].ai_assistant_type by calling
isRegisteredProvider(assistantType); if it returns false, load the merged config
via loadConfig() and set assistantType = config.assistant (fallback), ensuring
the database-derived value does not bypass provider registration checks; update
the logic around assistantType assignment in the block that reads from parent
and codebase so both sources are validated and fallback to
loadConfig().assistant when unregistered.

packages/server/src/routes/api.ts (1)

2210-2210: ⚡ Quick win

Pin jsYaml.dump options for deterministic YAML serialization.

Both calls at lines 2210 and 2340 use jsYaml.dump(definition) with defaults that can introduce unwanted mutations:

• lineWidth: 80 wraps long strings, potentially altering workflow prompts/comments persisted to disk
• noRefs: false emits YAML anchors/aliases for duplicate objects, which some downstream parsers handle inconsistently

Consider passing explicit options { lineWidth: -1, noRefs: true } and reusing a single constant across both serialization sites.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/server/src/routes/api.ts` at line 2210, The YAML serialization calls
using jsYaml.dump(definition) are non-deterministic; update both call sites (the
jsYaml.dump invocation near where yamlContent is set and the second call around
line 2340) to pass explicit options { lineWidth: -1, noRefs: true } and factor
these options into a shared constant (e.g., YAML_DUMP_OPTIONS) so both places
reuse the same settings to prevent line-wrapping and YAML anchors/aliases.

packages/providers/src/community/pi/event-bridge.ts (1)

426-432: 💤 Low value

Stale line-number references inside the finally comment.

The inline cross-references are off: queue.close() is at line 413 (comment says "line 392") and the .then() handlers are at line 358 (comment says "line 344"). This won't affect runtime but will mislead future readers following the breadcrumbs.

📝 Proposed update

-    // Don't await promptPromise. The queue is closed above (line 392), and the
-    // .then() handlers attached at construction (line 344) only push to that
+    // Don't await promptPromise. The queue is closed above (queue.close()), and the
+    // .then() handlers attached at construction (session.prompt().then(...)) only push to that

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/providers/src/community/pi/event-bridge.ts` around lines 426 - 432,
Update the stale inline comment in the finally block by removing or correcting
the hard-coded line numbers: reference the symbols instead (queue.close(), the
.then() handlers attached when promptPromise is created, session.prompt(),
dispose(), and generator.return()) so readers can find the relevant code without
depending on line numbers; either correct the line numbers to their current
locations or, preferably, replace them with these symbolic references and a
brief note explaining why we must not await promptPromise.