Merge pull request #726 from rowboatlabs/runtime-refactor

fix(x): close out the runtime review — findings 4-10

apps/x/ANALYTICS.md

@@ -41,9 +41,9 @@ Every `llm_usage` emit point in the codebase:
 
 | `use_case` | `sub_use_case` | `agent_name`? | Where | File:line |
 |---|---|---|---|---|
-| `copilot_chat` | (none) | yes | User chat in renderer (default for any `createRun` without `useCase`) | `packages/core/src/agents/runtime.ts:1313` (finish-step in `streamLlm`) |
+| `copilot_chat` | (none) | yes | User chat in renderer (turn runtime; the ALS default when no caller set a use case) | `packages/core/src/runtime/turns/bridges/real-usage-reporter.ts` (`reportModelUsage`); legacy runs (code-mode carve-out) still emit from `packages/core/src/runtime/legacy/engine.ts` (`streamLlm` finish-step) |
 | `copilot_chat` | `scheduled` | yes | Background scheduled agent runner | `packages/core/src/agent-schedule/runner.ts:167` |
-| `copilot_chat` | `file_parse` | inherits | `parseFile` builtin tool inside any chat | `packages/core/src/application/lib/builtin-tools.ts:770` |
+| `copilot_chat` | `file_parse` | inherits | `parseFile` builtin tool inside any chat | `packages/core/src/runtime/tools/domains/parsing.ts:179` |
 | `live_note_agent` | `routing` | no | Pass 1 routing classifier (`generateObject`) | `packages/core/src/knowledge/live-note/routing.ts:93` |
 | `live_note_agent` | `manual` | yes | Pass 2 agent run — user clicked Run / called the `run-live-note-agent` tool | `packages/core/src/knowledge/live-note/runner.ts:140` (createRun, `subUseCase: trigger`) |
 | `live_note_agent` | `cron` | yes | Pass 2 agent run — cron expression matched | same call site |

apps/x/VIDEO_MODE.md

@@ -176,10 +176,10 @@ Push-to-talk is disabled while a call owns the mic.
 
 | Prompt | Where |
 |--------|-------|
-| `# Video Mode (Live Camera)` system section — how to use webcam frames, coaching guidance, screen-share rules ("treat the screen as the primary subject", "last screen frame is current"), etiquette (never comment on appearance) | `packages/core/src/runtime/assembly/capabilities/modes.ts` (the `VIDEO_MODE` fragment of the `video-mode` capability, composed by `agents/compose-instructions.ts`) |
+| `# Video Mode (Live Camera)` system section — how to use webcam frames, coaching guidance, screen-share rules ("treat the screen as the primary subject", "last screen frame is current"), etiquette (never comment on appearance) | `packages/core/src/runtime/assembly/capabilities/modes.ts` (the `VIDEO_MODE` fragment of the `video-mode` capability, composed by `runtime/assembly/compose-instructions.ts`) |
 | `# Practice Session (Coach Mode)` system section — coaching persona: specific/actionable feedback after each take, one-sentence interjections mid-flow, structured debrief on wrap-up | `capabilities/modes.ts` (the `COACH_MODE` fragment, directly after the video capability) |
 | "Driving the app" paragraph in the video-mode section — on calls, prefer app-navigation read-view/open-item (show while telling) over describing or squinting at frames | same `# Video Mode` section; full action docs in the `app-navigation` skill (`runtime/assembly/skills/app-navigation/skill.ts`) |
-| Per-message frame context line `[Video mode: N live webcam frames … and M frames of the user's shared screen …]` + group labels | `packages/core/src/runtime/legacy/engine.ts` (`convertFromMessages`) |
+| Per-message frame context line `[Video mode: N live webcam frames … and M frames of the user's shared screen …]` + group labels | `packages/core/src/runtime/assembly/message-encoding.ts` (`convertFromMessages`) |
 | `videoMode` / `coachMode` composition overrides (session-sticky; flips bust prefix cache) | `packages/core/src/runtime/turns/bridges/real-agent-resolver.ts` (`CompositionOverrides`); set from `App.tsx` `sendConfig` |
 
 Voice input/output prompt sections (`# Voice Input`, `# Voice Output`) are

apps/x/packages/core/docs/turn-runtime-design.md

@@ -1,9 +1,10 @@
 # Turn Runtime Technical Specification
 
 Status: implemented and live. All chat, background, and knowledge callers
-run on this runtime; the legacy runs runtime remains only for code-mode
-sessions (see the carve-out section in the repo-root `AGENTS.md`). The
-companion session layer is specified in `session-design.md`.
+run on this runtime; the legacy runs runtime (`src/runtime/legacy/`) remains
+only for code-mode sessions and the mini-apps host API, and is deleted as a
+unit when those migrate. The companion session layer is specified in
+`session-design.md`.
 
 This document specifies a new turn-oriented agent loop for `@x/core`. It is
 intended to replace the behavioral responsibilities of the current run runtime

apps/x/packages/core/src/application/lib/errors.ts

@@ -0,0 +1,7 @@
+// One home for the error -> human-readable message idiom. The legacy runs
+// engine's getErrorDetails also unwrapped RunFailedError's multi-error list;
+// live callers run agents through the headless runner, whose HeadlessRunError
+// carries the failure detail in .message — so this is the whole contract.
+export function getErrorDetails(error: unknown): string {
+    return error instanceof Error ? error.message : String(error);
+}

apps/x/packages/core/src/filesystem/files.ts

@@ -68,12 +68,19 @@ const MAX_BYTES_LABEL = `${MAX_BYTES / 1024} KB`;
 let knowledgeCommitTimer: ReturnType<typeof setTimeout> | null = null;
 let canonicalWorkspaceRoot: string | null = null;
 
-function isPathInside(parent: string, child: string): boolean {
+// Exported as the one live containment check: permission decisions
+// (assembly/permission-metadata) key on it, so a divergent copy is a
+// permission-bypass risk, not a style issue. (legacy/repo.ts keeps its own
+// frozen copy — the quarantine must not import live modules.)
+export function isPathInside(parent: string, child: string): boolean {
   const relative = path.relative(parent, child);
   return relative === '' || (!!relative && !relative.startsWith('..') && !path.isAbsolute(relative));
 }
 
-function expandHomePath(inputPath: string): string {
+// Exported for tool inputs that take user/model-supplied paths (code cwd,
+// bg-task projectDir): the blank guard matters there because callers feed
+// the result to path.resolve, and resolve('') silently becomes process.cwd().
+export function expandHomePath(inputPath: string): string {
   const trimmed = inputPath.trim();
   if (!trimmed) {
     throw new Error('Path is required');

apps/x/packages/core/src/knowledge/agent_notes.ts

@@ -4,7 +4,7 @@ import { google } from 'googleapis';
 import { WorkDir } from '../config/config.js';
 import { runWhenPossible } from '../runtime/assembly/headless-app.js';
 import { getKgModel } from '../models/defaults.js';
-import { getErrorDetails } from '../runtime/legacy/utils.js';
+import { getErrorDetails } from '../application/lib/errors.js';
 import { serviceLogger } from '../services/service_logger.js';
 import { loadUserConfig, updateUserEmail } from '../config/user_config.js';
 import { GoogleClientFactory } from './google-client-factory.js';

apps/x/packages/core/src/knowledge/build_graph.ts

@@ -3,7 +3,7 @@ import path from 'path';
 import { WorkDir } from '../config/config.js';
 import { getKgModel } from '../models/defaults.js';
 import { runWhenPossible, toolInputPaths } from '../runtime/assembly/headless-app.js';
-import { getErrorDetails } from '../runtime/legacy/utils.js';
+import { getErrorDetails } from '../application/lib/errors.js';
 import { serviceLogger, type ServiceRunContext } from '../services/service_logger.js';
 import {
     loadState,

apps/x/packages/core/src/knowledge/label_emails.ts

@@ -3,7 +3,7 @@ import path from 'path';
 import { WorkDir } from '../config/config.js';
 import { runWhenPossible, toolInputPaths } from '../runtime/assembly/headless-app.js';
 import { getKgModel } from '../models/defaults.js';
-import { getErrorDetails } from '../runtime/legacy/utils.js';
+import { getErrorDetails } from '../application/lib/errors.js';
 import { serviceLogger } from '../services/service_logger.js';
 import { limitEventItems } from './limit_event_items.js';
 import {

apps/x/packages/core/src/knowledge/tag_notes.ts

@@ -3,7 +3,7 @@ import path from 'path';
 import { WorkDir } from '../config/config.js';
 import { runWhenPossible, toolInputPaths } from '../runtime/assembly/headless-app.js';
 import { getKgModel } from '../models/defaults.js';
-import { getErrorDetails } from '../runtime/legacy/utils.js';
+import { getErrorDetails } from '../application/lib/errors.js';
 import { serviceLogger } from '../services/service_logger.js';
 import { limitEventItems } from './limit_event_items.js';
 import {

apps/x/packages/core/src/runtime/assembly/copilot/instructions.ts

@@ -7,7 +7,6 @@ import {
     isSlackAvailable,
 } from "../connections.js";
 import { composioAccountsRepo } from "../../../composio/repo.js";
-import { isConfigured as isComposioConfigured } from "../../../composio/client.js";
 import { CURATED_TOOLKITS } from "@x/shared/dist/composio.js";
 import { knowledgeSourcesRepo } from "../../../knowledge/sources/repo.js";
 import { listApps } from "../../../apps/indexer.js";
@@ -52,7 +51,10 @@ When a question matches what an app tracks, PREFER the app over external calls:
  * Lists connected toolkits and explains the meta-tool discovery flow.
  */
 async function getComposioToolsPrompt(slackConnected: boolean = false, googleConnected: boolean = false): Promise<string> {
-    if (!(await isComposioConfigured())) {
+    // connections.js, not the raw composio client: the skill catalog's
+    // availability filter uses the same check, so the prompt's Composio
+    // section and the catalog's composio skill can never disagree.
+    if (!(await isComposioAvailable())) {
         return '';
     }
 

apps/x/packages/core/src/runtime/assembly/permission-metadata.ts

@@ -11,15 +11,10 @@ import { z } from "zod";
 import { ToolPermissionMetadata } from "@x/shared/dist/runs.js";
 import { isBlocked, extractCommandNames } from "../../application/lib/command-executor.js";
 import { getFileAccessAllowList, type FileAccessGrant, type FileAccessOperation } from "../../config/security.js";
-import { resolveFilePathForPermission } from "../../filesystem/files.js";
+import { isPathInside, resolveFilePathForPermission } from "../../filesystem/files.js";
 
 type ToolPermissionMetadataValue = z.infer<typeof ToolPermissionMetadata>;
 
-function isPathInside(parent: string, child: string): boolean {
-    const relative = path.relative(parent, child);
-    return relative === '' || (!!relative && !relative.startsWith('..') && !path.isAbsolute(relative));
-}
-
 function fileGrantCoversPath(grant: FileAccessGrant, operation: FileAccessOperation, resolvedPath: string): boolean {
     return grant.operation === operation && isPathInside(path.resolve(grant.pathPrefix), path.resolve(resolvedPath));
 }

apps/x/packages/core/src/runtime/assembly/registry.ts

@@ -14,24 +14,20 @@ import { lazyResolve } from "../../di/lazy-resolve.js";
 import type { IAgentsRepo } from "./repo.js";
 
 // The registry of built-in agents: one table instead of the historical
-// if (id === ...) ladder. An entry owns its builder and its traits; traits
-// replace stringly-typed id comparisons ("copilot" || "rowboatx") scattered
-// across the assembly layer. User-defined agents are the fallthrough,
-// fetched from the agents repo.
+// if (id === ...) ladder. An entry owns its builder. Traits (which replace
+// stringly-typed id comparisons scattered across the assembly layer) live in
+// traits.ts — a leaf module, because this file's builders transitively reach
+// di/container and upstream trait readers would cycle. Re-exported here so
+// assembly-level consumers keep one import.
 
-export interface AgentTraits {
-    // Receives workspace context — agent notes and the user work directory —
-    // composed into its system prompt.
-    workspaceContext?: boolean;
-    // Session-loaded skills (activeSkills) re-attach their tools on later
-    // turns. Distinct from workspaceContext: a trait per concern, so neither
-    // silently inherits the other's meaning.
-    skillCarryForward?: boolean;
-}
+export {
+    carriesSkillsForward,
+    hasWorkspaceContext,
+    type AgentTraits,
+} from "./traits.js";
 
 interface BuiltinAgentDefinition {
     build: () => Promise<z.infer<typeof Agent>> | z.infer<typeof Agent>;
-    traits?: AgentTraits;
 }
 
 // Prompt-file agents: instructions ship as a raw string whose optional YAML
@@ -55,7 +51,6 @@ function promptFileAgent(id: string, getRaw: () => string): BuiltinAgentDefiniti
 // definition object.
 const COPILOT: BuiltinAgentDefinition = {
     build: buildCopilotAgent,
-    traits: { workspaceContext: true, skillCarryForward: true },
 };
 
 const builtinAgents: Record<string, BuiltinAgentDefinition> = {
@@ -75,26 +70,6 @@ export function builtinAgentIds(): string[] {
     return Object.keys(builtinAgents);
 }
 
-// Trait lookups for assembly decisions. Unknown/user agents have no traits.
-function hasTrait(
-    agentId: string | null | undefined,
-    trait: keyof AgentTraits,
-): boolean {
-    return (
-        agentId != null &&
-        Object.hasOwn(builtinAgents, agentId) &&
-        builtinAgents[agentId].traits?.[trait] === true
-    );
-}
-
-export function hasWorkspaceContext(agentId: string | null | undefined): boolean {
-    return hasTrait(agentId, "workspaceContext");
-}
-
-export function carriesSkillsForward(agentId: string | null | undefined): boolean {
-    return hasTrait(agentId, "skillCarryForward");
-}
-
 export async function loadAgent(id: string): Promise<z.infer<typeof Agent>> {
     // Object.hasOwn: a plain lookup would traverse Object.prototype, so a
     // user agent named "constructor"/"toString" would hit an inherited

apps/x/packages/core/src/runtime/assembly/skills/index.ts

@@ -36,8 +36,6 @@ const CATALOG_PREFIX = "src/runtime/assembly/skills";
 // carry loadSkill references under this prefix, so it stays a resolvable alias.
 const LEGACY_CATALOG_PREFIX = "src/application/assistant/skills";
 
-// console.log(liveNoteSkill);
-
 // A skill IS the model-activated variant of the capability record
 // (capabilities/types.ts): lazy guidance the model pulls in via loadSkill,
 // plus the BuiltinTools it owns. `id` doubles as the folder name. Tool names

apps/x/packages/core/src/runtime/assembly/traits.ts

@@ -0,0 +1,49 @@
+// Built-in agent traits: static metadata with deliberately zero imports.
+// The registry (registry.ts) owns the id -> builder table, but its builders
+// drag in the copilot instruction chain, which transitively reaches
+// di/container — so upstream layers (sessions) reading a trait through the
+// registry would create a module cycle back into themselves. Traits live in
+// this leaf so trait checks are safe to import from anywhere.
+
+export interface AgentTraits {
+    // Receives workspace context — agent notes and the user work directory —
+    // composed into its system prompt.
+    workspaceContext?: boolean;
+    // Session-loaded skills (activeSkills) re-attach their tools on later
+    // turns. Distinct from workspaceContext: a trait per concern, so neither
+    // silently inherits the other's meaning.
+    skillCarryForward?: boolean;
+}
+
+// "rowboatx" is a legacy alias for the copilot: both ids share one traits
+// object. Agents absent from this table have no traits (user agents, and
+// builtins that need none).
+const COPILOT_TRAITS: AgentTraits = {
+    workspaceContext: true,
+    skillCarryForward: true,
+};
+
+const agentTraits: Record<string, AgentTraits> = {
+    copilot: COPILOT_TRAITS,
+    rowboatx: COPILOT_TRAITS,
+};
+
+// Trait lookups for assembly decisions. Unknown/user agents have no traits.
+function hasTrait(
+    agentId: string | null | undefined,
+    trait: keyof AgentTraits,
+): boolean {
+    return (
+        agentId != null &&
+        Object.hasOwn(agentTraits, agentId) &&
+        agentTraits[agentId][trait] === true
+    );
+}
+
+export function hasWorkspaceContext(agentId: string | null | undefined): boolean {
+    return hasTrait(agentId, "workspaceContext");
+}
+
+export function carriesSkillsForward(agentId: string | null | undefined): boolean {
+    return hasTrait(agentId, "skillCarryForward");
+}

apps/x/packages/core/src/runtime/legacy/utils.ts

@@ -20,16 +20,6 @@ export class RunFailedError extends Error {
     }
 }
 
-export function getErrorDetails(error: unknown): string {
-    if (error instanceof RunFailedError) {
-        return error.errors.join("\n\n");
-    }
-    if (error instanceof Error) {
-        return error.message;
-    }
-    return String(error);
-}
-
 /**
  * Extract the assistant's final text response from a run's log.
  * @param runId

apps/x/packages/core/src/runtime/sessions/sessions.test.ts

@@ -943,6 +943,24 @@ describe("active-skill carry-forward", () => {
         });
     });
 
+    it("does not inject activeSkills for agents without the skillCarryForward trait", async () => {
+        // The resolver would ignore them anyway (real-agent-resolver gates on
+        // the same trait); this pins that sessions doesn't persist an
+        // ever-growing list into the requested composition either.
+        const { sessions, fake } = makeSessions();
+        const sessionId = await sessions.createSession();
+        const { turnId } = await sessions.sendMessage(sessionId, user("one"), {
+            agent: { agentId: "my-user-agent" },
+        });
+        await flush();
+        fake.setLog(turnId, skillLoadLog(turnId, sessionId, { agentId: "my-user-agent" }));
+
+        await sessions.sendMessage(sessionId, user("two"), {
+            agent: { agentId: "my-user-agent" },
+        });
+        expect(fake.createTurnInputs[1].agent).toEqual({ agentId: "my-user-agent" });
+    });
+
     it("accumulates across turns and unions with caller-supplied skills, preserving order", async () => {
         const { sessions, fake } = makeSessions();
         const sessionId = await sessions.createSession();

apps/x/packages/core/src/runtime/sessions/sessions.ts

@@ -28,6 +28,10 @@ import type {
     TurnExternalInput,
     TurnOutcome,
 } from "../turns/api.js";
+// traits.js, not registry.js: the registry's builders transitively reach
+// di/container, which imports this module — a cycle. The traits leaf exists
+// exactly so upstream layers can gate on traits.
+import { carriesSkillsForward } from "../assembly/traits.js";
 import type { IClock } from "../turns/clock.js";
 import {
     type ISessions,
@@ -461,7 +465,15 @@ function withActiveSkills(
     agent: SendMessageConfig["agent"],
     activeSkills: string[],
 ): SendMessageConfig["agent"] {
-    if (isInlineAgentRequest(agent) || activeSkills.length === 0) {
+    // Mirrors the resolver's carriesSkillsForward gate (real-agent-resolver):
+    // the resolver would ignore activeSkills for a non-trait agent anyway,
+    // but injecting them here would still persist an ever-growing list into
+    // every turn's requested composition.
+    if (
+        isInlineAgentRequest(agent) ||
+        activeSkills.length === 0 ||
+        !carriesSkillsForward(agent.agentId)
+    ) {
         return agent;
     }
     const composition = agent.overrides?.composition;

apps/x/packages/core/src/runtime/tools/domains/background-tasks.ts

@@ -10,7 +10,7 @@ import container from "../../../di/container.js";
 import { BackgroundTaskSchema, TriggersSchema } from "@x/shared/dist/background-task.js";
 import * as gitService from "../../../code-mode/git/service.js";
 import type { ICodeProjectsRepo } from "../../../code-mode/projects/repo.js";
-import { expandHome } from "../paths.js";
+import { expandHomePath } from "../../../filesystem/files.js";
 
 // Inputs for the bg-task builtin tools. Reuse the canonical schema field
 // descriptions; only `triggers` gets a tighter contextual override (the
@@ -45,7 +45,7 @@ export const PatchBackgroundTaskInput = BackgroundTaskSchema.pick({
 export async function resolveCodeProject(dirPath: string): Promise<
     { ok: true; projectId: string; path: string; warning?: string } | { ok: false; error: string }
 > {
-    const abs = path.resolve(expandHome(dirPath));
+    const abs = path.resolve(expandHomePath(dirPath));
     const projectsRepo = container.resolve<ICodeProjectsRepo>('codeProjectsRepo');
     let project: Awaited<ReturnType<ICodeProjectsRepo['add']>>;
     try {

apps/x/packages/core/src/runtime/tools/domains/code.ts

@@ -12,7 +12,7 @@ import { ICodeModeConfigRepo } from "../../../code-mode/repo.js";
 import type { ApprovalPolicy, CodeRunEvent as CodeRunEventType } from "@x/shared/dist/code-mode.js";
 import type { CodeRunFeed } from "../../../code-mode/feed.js";
 import type { ToolContext } from "../exec-tool.js";
-import { expandHome } from "../paths.js";
+import { expandHomePath } from "../../../filesystem/files.js";
 import { BuiltinToolsSchema } from "../types.js";
 
 
@@ -61,7 +61,7 @@ export const codeAgentRunTools: z.infer<typeof BuiltinToolsSchema> = {
             // cwd argument over the session's. Expand `~` and resolve to an absolute path:
             // the engine is spawned with this as the child's cwd, and `child_process.spawn`
             // does NO shell tilde expansion.
-            const effectiveCwd = path.resolve(expandHome(ctx.codeCwd ?? cwd));
+            const effectiveCwd = path.resolve(expandHomePath(ctx.codeCwd ?? cwd));
             // Fail loudly if the directory is missing. Otherwise the spawn below fails with
             // Node's misleading "spawn <command> ENOENT" (it blames the executable, not the
             // bad cwd), which reads as "the coding engine isn't installed" — see the enriched

apps/x/packages/core/src/runtime/tools/domains/notifications.ts

@@ -7,6 +7,7 @@ import container from "../../../di/container.js";
 import type { ToolContext } from "../exec-tool.js";
 import { getCurrentUseCase } from "../../../analytics/use_case.js";
 import type { INotificationService } from "../../../application/notification/service.js";
+import type { ITurnRepo } from "../../turns/repo.js";
 import { notifyIfEnabled } from "../../../application/notification/notifier.js";
 import { BuiltinToolsSchema } from "../types.js";
 
@@ -42,14 +43,19 @@ export const notificationTools: z.infer<typeof BuiltinToolsSchema> = {
                     return { success: false, error: 'Notifications are not supported on this system' };
                 }
                 let uc = getCurrentUseCase()?.useCase;
-                // ALS doesn't reliably propagate across the run's async generator,
-                // so when the in-context use-case is missing, fall back to the
-                // persisted use case on the run record via ctx.runId.
+                // The ALS context can be missing when the turn is advanced
+                // outside the chain that started it (crash recovery, resume
+                // paths). The durable signal is the agent id on the turn
+                // record — only the background-task runner starts
+                // 'background-task-agent' turns. ctx.runId is the turn id.
                 if (!uc && ctx?.runId) {
                     try {
-                        const { fetchRun } = await import("../../legacy/runs.js");
-                        const run = await fetchRun(ctx.runId);
-                        uc = run.useCase;
+                        const turnRepo = container.resolve<ITurnRepo>('turnRepo');
+                        const [created] = await turnRepo.read(ctx.runId);
+                        if (created?.type === 'turn_created'
+                            && created.agent.resolved.agentId === 'background-task-agent') {
+                            uc = 'background_task_agent';
+                        }
                     } catch {
                         // best effort — fall through to the default branch
                     }

apps/x/packages/core/src/runtime/tools/paths.ts

@@ -1,18 +0,0 @@
-// Lenient ~-expansion for user-supplied paths in tool inputs. Distinct from
-// filesystem/files.ts' private expandHomePath, which throws on empty input —
-// tool entries want pass-through semantics for their own validation.
-
-import * as path from "path";
-import * as os from "os";
-
-// Turn a user-supplied directory into a registered code project id. Reuses the
-// same idempotent registry the Code-section picker writes to (add() validates the
-// dir exists & is a directory, and dedupes by resolved path). Returns a soft
-// `warning` — not an error — when the repo isn't yet worktree-ready, so the task
-// still gets created and the copilot can tell the user what to fix.
-export function expandHome(p: string): string {
-    const t = p.trim();
-    if (t === '~') return os.homedir();
-    if (t.startsWith('~/') || t.startsWith(`~${path.sep}`)) return path.join(os.homedir(), t.slice(2));
-    return t;
-}