feat(server): agent-definition loader (Phase 1) by dimakis · Pull Request #363 · dimakis/mitzo

dimakis · 2026-05-30T11:21:21Z

Summary

Add server/agent-loader.ts — three-tier recipe discovery (ContexGin API → local .agents/ override → bundled fallback) with 5-minute TTL cache
Store parsed agentDefinition + agentDefinitionSource in ManagedSession (session-registry)
Wire loadAgentDef() fire-and-forget into session start in chat.ts
Add DEFAULT_AGENT_DEFINITION bundled fallback to constants.ts

This is Phase 1 of the agent-definition-loading spec. Future phases will consume the stored recipe for governance enforcement, provider tiering, memory scoping, and output conventions.

Test plan

13 new tests in agent-loader.test.ts — all sources, caching, error handling
45 existing tests (boot-context, session-registry) pass with no regressions
Manual: verify session starts with ContexGin running (source=contexgin)
Manual: verify session starts with ContexGin down (source=fallback)

🤖 Generated with Claude Code

dimakis

Centaur Review

Found 8 issue(s) (1 critical) (4 warning).

`server/agent-loader.ts`

The agent-loader module is well-structured with good test coverage, but has a critical bug: it imports from the yaml package which is not installed (the project uses js-yaml). The local YAML fallback path will fail at runtime, hidden by test mocks. Secondary concerns: const → let without mutation, hardcoded fallback identity name, and cache key missing the server URL.

🔴 bugs (L14): Imports parse from 'yaml' but the project depends on 'js-yaml' (see root package.json and server/session-index.ts which imports from 'js-yaml'). The yaml package is not installed — loadFromLocal() will fail at runtime with a module-not-found error. The test mocks the yaml module so this is invisible in tests. Should be import { load as parseYaml } from 'js-yaml' (note: js-yaml exports load, not parse). [fixable]
🟡 unsafe_assumptions (L208): Cache key is ${agentName}:${cwd} but does not include contexginUrl. If loadAgentDef is ever called with different contexginUrl values for the same agent+cwd pair, the cache will return the result from the first server. Currently safe because contexginUrl is always the default, but fragile if the API surface expands. [fixable]
🟡 bugs (L239): The bundled fallback always returns DEFAULT_AGENT_DEFINITION which hardcodes identity.name: 'mitzo-conversational', regardless of the agentName parameter. If a different agent name is requested and all sources fail, the returned definition has a mismatched name. Consider spreading the constant and overriding identity.name with the requested agentName. [fixable]
🟡 unsafe_assumptions (L127): ContexGin response fields (identity, provider, governance, memory) are cast directly to typed interfaces via as without runtime validation. A malformed ContexGin response (e.g., identity.name is a number) would pass the null-check but produce a silently incorrect AgentDefinition. The identity?.name and identity?.description truthy checks catch null/undefined but not wrong types. [fixable]

`server/chat.ts`

The agent-loader module is well-structured with good test coverage, but has a critical bug: it imports from the yaml package which is not installed (the project uses js-yaml). The local YAML fallback path will fail at runtime, hidden by test mocks. Secondary concerns: const → let without mutation, hardcoded fallback identity name, and cache key missing the server URL.

🔵 style (L888): systemPromptAppend was changed from const to let but is never reassigned anywhere in the function. Should remain const. [fixable]
🟡 regressions (L920): The refactored fetchBootContext no longer passes repoRoot when called from _startChatInner — it uses the default BASE_REPO. The old code used cwd (which may be a worktree path). If the local Python fallback script (build_boot_context.py) needs to run from the worktree rather than BASE_REPO, this changes behavior. Verify the script works correctly when run from BASE_REPO even when the session is in a worktree. [fixable]

`server/tests/agent-loader.test.ts`

The agent-loader module is well-structured with good test coverage, but has a critical bug: it imports from the yaml package which is not installed (the project uses js-yaml). The local YAML fallback path will fail at runtime, hidden by test mocks. Secondary concerns: const → let without mutation, hardcoded fallback identity name, and cache key missing the server URL.

🔵 missing_tests: No test for cache TTL expiration — a cached entry past its 5-minute TTL should trigger a re-fetch. The existing cache tests only verify basic hit/miss and clearCache(), not time-based expiry. [fixable]

`packages/harness/src/session-registry.ts`

The agent-loader module is well-structured with good test coverage, but has a critical bug: it imports from the yaml package which is not installed (the project uses js-yaml). The local YAML fallback path will fail at runtime, hidden by test mocks. Secondary concerns: const → let without mutation, hardcoded fallback identity name, and cache key missing the server URL.

🔵 style (L34): agentDefinition is typed as Record<string, unknown> in ManagedSession but the loader produces a well-typed AgentDefinition. Consider importing and using AgentDefinition from agent-loader to preserve type safety end-to-end, instead of the double cast through unknown at chat.ts:903. [fixable]

dimakis · 2026-05-30T11:24:46Z

+
+import { readFileSync } from 'fs';
+import { join } from 'path';
+import { parse as parseYaml } from 'yaml';


🔴 bugs: Imports parse from 'yaml' but the project depends on 'js-yaml' (see root package.json and server/session-index.ts which imports from 'js-yaml'). The yaml package is not installed — loadFromLocal() will fail at runtime with a module-not-found error. The test mocks the yaml module so this is invisible in tests. Should be import { load as parseYaml } from 'js-yaml' (note: js-yaml exports load, not parse). [fixable]

Fixed — changed to import { load as parseYaml } from 'js-yaml' matching the rest of the codebase.

dimakis · 2026-05-30T11:24:46Z

+  contexginUrl: string = process.env.CONTEXGIN_URL || 'http://localhost:8321',
+): Promise<LoadedAgentDefinition> {
+  // Check cache
+  const cacheKey = `${agentName}:${cwd}`;


🟡 unsafe_assumptions: Cache key is ${agentName}:${cwd} but does not include contexginUrl. If loadAgentDef is ever called with different contexginUrl values for the same agent+cwd pair, the cache will return the result from the first server. Currently safe because contexginUrl is always the default, but fragile if the API surface expands. [fixable]

Fixed — cache key now includes contexginUrl: ${agentName}:${cwd}:${contexginUrl}.

dimakis · 2026-05-30T11:24:46Z

+  // 3. Bundled fallback
+  log.info('using bundled agent definition fallback', { agent: agentName });
+  const fallback: LoadedAgentDefinition = {
+    definition: DEFAULT_AGENT_DEFINITION,


🟡 bugs: The bundled fallback always returns DEFAULT_AGENT_DEFINITION which hardcodes identity.name: 'mitzo-conversational', regardless of the agentName parameter. If a different agent name is requested and all sources fail, the returned definition has a mismatched name. Consider spreading the constant and overriding identity.name with the requested agentName. [fixable]

Fixed — fallback now spreads DEFAULT_AGENT_DEFINITION and overrides identity.name with the requested agentName.

dimakis · 2026-05-30T11:24:46Z

+
+    if (!res.ok) return null;
+
+    const data = (await res.json()) as Record<string, unknown>;


🟡 unsafe_assumptions: ContexGin response fields (identity, provider, governance, memory) are cast directly to typed interfaces via as without runtime validation. A malformed ContexGin response (e.g., identity.name is a number) would pass the null-check but produce a silently incorrect AgentDefinition. The identity?.name and identity?.description truthy checks catch null/undefined but not wrong types. [fixable]

Acknowledged — runtime validation of ContexGin response fields is a valid concern. The current truthy checks catch null/undefined which covers the most common failure modes. Full schema validation could be a Phase 2 addition.

dimakis · 2026-05-30T11:24:47Z


  // Build the system prompt append string (used by both query and comparison)
-  const systemPromptAppend =
+  let systemPromptAppend =


🔵 style: systemPromptAppend was changed from const to let but is never reassigned anywhere in the function. Should remain const. [fixable]

Fixed — systemPromptAppend restored to const (resolved during rebase).

dimakis · 2026-05-30T11:24:47Z

+    });
+
+  // Fire-and-forget: fetch boot context from running ContexGin server
+  fetchBootContext(agentName)


🟡 regressions: The refactored fetchBootContext no longer passes repoRoot when called from _startChatInner — it uses the default BASE_REPO. The old code used cwd (which may be a worktree path). If the local Python fallback script (build_boot_context.py) needs to run from the worktree rather than BASE_REPO, this changes behavior. Verify the script works correctly when run from BASE_REPO even when the session is in a worktree. [fixable]

This is the same observation as PR #361 (now merged). The fallback running against BASE_REPO is correct since the script reads canonical source files, not worktree-specific content.

dimakis · 2026-05-30T11:24:47Z

  /** All worktrees created for this session, keyed by repo name. */
  worktreePaths: Map<string, { path: string; wtId: string }>;
+  /** Parsed agent definition (recipe). Null if loading failed. */
+  agentDefinition: Record<string, unknown> | null;


🔵 style: agentDefinition is typed as Record<string, unknown> in ManagedSession but the loader produces a well-typed AgentDefinition. Consider importing and using AgentDefinition from agent-loader to preserve type safety end-to-end, instead of the double cast through unknown at chat.ts:903. [fixable]

Partially addressed — agentDefinitionSource now uses the concrete union type 'contexgin' | 'local' | 'fallback'. For agentDefinition, keeping Record<string, unknown> to avoid cross-package type duplication (the AgentDefinition interface lives in server/, harness is a separate package). Full type sharing is a Phase 2 concern.

dimakis

Centaur Review

Found 6 issue(s) (2 warning).

`server/agent-loader.ts`

Solid Phase 1 with good test coverage and graceful degradation; main concerns are type erasure through Record<string, unknown> on the session interface (will compound in Phase 2) and an inherited path-traversal pattern on the user-controlled agentName.

🟡 unsafe_assumptions (L164): Path traversal via user-controlled agentName: join(cwd, '.agents', ${agentName}.yaml) does not validate that the resolved path stays under cwd/.agents/. An authenticated WebSocket client sending agentName: '../../../etc/something' could read arbitrary .yaml files on disk. The same pattern already exists at chat.ts:825, so this is pre-existing, but worth fixing as part of this new module — e.g. resolve() the path and check it starts with the expected prefix. [fixable]
🔵 bugs (L239): The bundled fallback always returns DEFAULT_AGENT_DEFINITION with identity.name: 'mitzo-conversational' regardless of which agentName was requested. If a session starts with a non-default agent and all sources fail, the stored definition's name won't match the session's agentName. Consider overriding identity.name with the requested agentName in the fallback path. [fixable]
🔵 style (L165): readFileSync blocks the event loop inside what is otherwise a fully async resolution chain. Since loadAgentDef is already async, consider using readFile from fs/promises and making loadFromLocal async to match. [fixable]

`server/chat.ts`

Solid Phase 1 with good test coverage and graceful degradation; main concerns are type erasure through Record<string, unknown> on the session interface (will compound in Phase 2) and an inherited path-traversal pattern on the user-controlled agentName.

🟡 style (L777): loaded.definition as unknown as Record<string, unknown> double-casts away the typed AgentDefinition into an untyped record. This happens because ManagedSession.agentDefinition is declared as Record<string, unknown> | null instead of importing and using AgentDefinition. Phase 2 consumers will have to cast it back. Consider typing the session field as AgentDefinition | null (import from agent-loader.js) to preserve type safety end-to-end. [fixable]

`packages/harness/src/session-registry.ts`

Solid Phase 1 with good test coverage and graceful degradation; main concerns are type erasure through Record<string, unknown> on the session interface (will compound in Phase 2) and an inherited path-traversal pattern on the user-controlled agentName.

🔵 style (L56): agentDefinitionSource is typed as string? but the actual values are the union 'contexgin' | 'local' | 'fallback' (AgentDefinitionSource in agent-loader.ts). Using the concrete union type would prevent invalid values and give downstream consumers better autocomplete/exhaustiveness checking. [fixable]

`server/tests/agent-loader.test.ts`

Solid Phase 1 with good test coverage and graceful degradation; main concerns are type erasure through Record<string, unknown> on the session interface (will compound in Phase 2) and an inherited path-traversal pattern on the user-controlled agentName.

🔵 missing_tests: Cache TTL expiry is untested — the tests verify cache hits and clearCache(), but don't verify that an expired entry is re-fetched (e.g. by advancing Date.now() with vi.useFakeTimers). Also, the ContexGin 200 with missing provider path (exercising the provider ?? { default: 'claude-opus-4' } fallback at line 146) has no test. [fixable]

dimakis · 2026-05-30T11:30:43Z

+ */
+function loadFromLocal(agentName: string, cwd: string): LoadedAgentDefinition | null {
+  try {
+    const filePath = join(cwd, '.agents', `${agentName}.yaml`);


🟡 unsafe_assumptions: Path traversal via user-controlled agentName: join(cwd, '.agents', ${agentName}.yaml) does not validate that the resolved path stays under cwd/.agents/. An authenticated WebSocket client sending agentName: '../../../etc/something' could read arbitrary .yaml files on disk. The same pattern already exists at chat.ts:825, so this is pre-existing, but worth fixing as part of this new module — e.g. resolve() the path and check it starts with the expected prefix. [fixable]

Fixed — added path traversal guard: resolve() the path and verify it starts with resolve(cwd, '.agents') + '/'. Returns null if traversal detected.

dimakis · 2026-05-30T11:30:43Z

+  // 3. Bundled fallback
+  log.info('using bundled agent definition fallback', { agent: agentName });
+  const fallback: LoadedAgentDefinition = {
+    definition: DEFAULT_AGENT_DEFINITION,


🔵 bugs: The bundled fallback always returns DEFAULT_AGENT_DEFINITION with identity.name: 'mitzo-conversational' regardless of which agentName was requested. If a session starts with a non-default agent and all sources fail, the stored definition's name won't match the session's agentName. Consider overriding identity.name with the requested agentName in the fallback path. [fixable]

Fixed — same as above, fallback now overrides identity.name with the requested agentName.

dimakis · 2026-05-30T11:30:43Z

+function loadFromLocal(agentName: string, cwd: string): LoadedAgentDefinition | null {
+  try {
+    const filePath = join(cwd, '.agents', `${agentName}.yaml`);
+    const raw = readFileSync(filePath, 'utf-8');


🔵 style: readFileSync blocks the event loop inside what is otherwise a fully async resolution chain. Since loadAgentDef is already async, consider using readFile from fs/promises and making loadFromLocal async to match. [fixable]

Fixed — readFileSync replaced with readFile from fs/promises, loadFromLocal is now fully async.

dimakis · 2026-05-30T11:30:43Z

+    .then((loaded) => {
+      const s = registry.get(clientId);
+      if (s) {
+        s.agentDefinition = loaded.definition as unknown as Record<string, unknown>;


🟡 style: loaded.definition as unknown as Record<string, unknown> double-casts away the typed AgentDefinition into an untyped record. This happens because ManagedSession.agentDefinition is declared as Record<string, unknown> | null instead of importing and using AgentDefinition. Phase 2 consumers will have to cast it back. Consider typing the session field as AgentDefinition | null (import from agent-loader.js) to preserve type safety end-to-end. [fixable]

Keeping the cast for now — AgentDefinition lives in server/ (agent-loader) and the harness is a separate package. Removing the cast requires either moving the type to harness or a shared types package. Phase 2 scope.

dimakis · 2026-05-30T11:30:43Z

+  /** Parsed agent definition (recipe). Null if loading failed. */
+  agentDefinition: Record<string, unknown> | null;
+  /** Source of the agent definition: 'contexgin' | 'local' | 'fallback'. */
+  agentDefinitionSource?: string;


🔵 style: agentDefinitionSource is typed as string? but the actual values are the union 'contexgin' | 'local' | 'fallback' (AgentDefinitionSource in agent-loader.ts). Using the concrete union type would prevent invalid values and give downstream consumers better autocomplete/exhaustiveness checking. [fixable]

Fixed — agentDefinitionSource now typed as 'contexgin' | 'local' | 'fallback' instead of string.

Load full agent definitions from ContexGin API, local .agents/ overrides, or bundled fallback at session start. Store parsed recipe in session registry for downstream consumption (governance, provider tiering, memory scope in future phases). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

- Fix YAML import: yaml → js-yaml (load) - Override fallback identity.name with requested agentName - Add path traversal guard in loadFromLocal - readFileSync → async readFile - Cache key includes contexginUrl - Type agentDefinitionSource as union in session registry - Update test mocks for async readFile and js-yaml Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

dimakis

Centaur Review

Found 7 issue(s) (3 warning).

`server/agent-loader.ts`

Solid Phase 1 implementation with correct fallback semantics and good test coverage. Main issues: duplicate AgentDefinitionSource type across packages, untyped ManagedSession.agentDefinition requiring unsafe casts, and missing tests for cache TTL expiry and path traversal guard.

🟡 style (L89): AgentDefinitionSource is defined identically in both server/agent-loader.ts:89 and packages/harness/src/session-registry.ts:12. The server module should import the type from @mitzo/harness (via session-registry.ts) instead of declaring its own copy — two definitions will drift independently. [fixable]
🟡 bugs (L215): Dead null-coalesce: contexginUrl ?? 'default' in the cache key can never trigger because the parameter has a default value (process.env.CONTEXGIN_URL || 'http://localhost:8321'). The ?? guard is unreachable — if the intent is to handle an explicit undefined argument, the default parameter already covers it. Harmless but misleading. [fixable]
🟡 unsafe_assumptions (L170): The path traversal guard (!filePath.startsWith(agentsDir + '/')) is correct for the ../ case but fragile — it relies on resolve() normalizing away traversal segments. A safer pattern is const rel = path.relative(agentsDir, filePath); if (rel.startsWith('..') || path.isAbsolute(rel)) return null; which is the idiomatic Node.js approach. The current guard also doesn't protect against symlink-based escapes if a symlink exists inside .agents/. Low risk since agentName comes from server-controlled config, but worth hardening. [fixable]
🔵 unsafe_assumptions (L130): The ContexGin response fields (identity, provider, governance, memory) are cast with as without validation — if ContexGin returns e.g. identity: "string" instead of an object, the code proceeds with a malformed definition stored in the session. The local YAML path has the same issue. Since this is Phase 1 and the data is from a trusted internal service, this is low-risk, but a lightweight Zod schema would prevent silent corruption. [fixable]

`server/chat.ts`

Solid Phase 1 implementation with correct fallback semantics and good test coverage. Main issues: duplicate AgentDefinitionSource type across packages, untyped ManagedSession.agentDefinition requiring unsafe casts, and missing tests for cache TTL expiry and path traversal guard.

🔵 unsafe_assumptions (L919): The double cast loaded.definition as unknown as Record<string, unknown> is needed because ManagedSession stores agentDefinition as Record<string, unknown> | null while the loader returns a typed AgentDefinition. Consider typing ManagedSession.agentDefinition as AgentDefinition | null (importing the type from agent-loader) to eliminate the cast and get type safety on consumers. The untyped Record makes downstream reads unsafe. [fixable]

`server/tests/agent-loader.test.ts`

Solid Phase 1 implementation with correct fallback semantics and good test coverage. Main issues: duplicate AgentDefinitionSource type across packages, untyped ManagedSession.agentDefinition requiring unsafe casts, and missing tests for cache TTL expiry and path traversal guard.

🔵 missing_tests: No test covers cache TTL expiry — the cache has a 5-minute TTL but no test advances time (via vi.useFakeTimers / vi.advanceTimersByTime) past expiry to verify a refetch occurs. The existing 'refetches after cache clear' test only covers manual invalidation. [fixable]
🔵 missing_tests: No test covers the path traversal guard in loadFromLocal — e.g., calling loadAgentDef with agentName='../../etc/passwd' should fall through to fallback without reading the file. This is the security-critical code path. [fixable]

dimakis · 2026-06-04T22:58:35Z

+  output?: AgentOutput;
+}
+
+export type AgentDefinitionSource = 'contexgin' | 'local' | 'fallback';


🟡 style: AgentDefinitionSource is defined identically in both server/agent-loader.ts:89 and packages/harness/src/session-registry.ts:12. The server module should import the type from @mitzo/harness (via session-registry.ts) instead of declaring its own copy — two definitions will drift independently. [fixable]

dimakis · 2026-06-04T22:58:35Z

+  contexginUrl: string = process.env.CONTEXGIN_URL || 'http://localhost:8321',
+): Promise<LoadedAgentDefinition> {
+  // Check cache
+  const cacheKey = `${agentName}:${cwd}:${contexginUrl ?? 'default'}`;


🟡 bugs: Dead null-coalesce: contexginUrl ?? 'default' in the cache key can never trigger because the parameter has a default value (process.env.CONTEXGIN_URL || 'http://localhost:8321'). The ?? guard is unreachable — if the intent is to handle an explicit undefined argument, the default parameter already covers it. Harmless but misleading. [fixable]

dimakis · 2026-06-04T22:58:35Z

+    // Path traversal guard: ensure resolved path stays under .agents/
+    const agentsDir = resolve(cwd, '.agents');
+    const filePath = resolve(agentsDir, `${agentName}.yaml`);
+    if (!filePath.startsWith(agentsDir + '/')) return null;


🟡 unsafe_assumptions: The path traversal guard (!filePath.startsWith(agentsDir + '/')) is correct for the ../ case but fragile — it relies on resolve() normalizing away traversal segments. A safer pattern is const rel = path.relative(agentsDir, filePath); if (rel.startsWith('..') || path.isAbsolute(rel)) return null; which is the idiomatic Node.js approach. The current guard also doesn't protect against symlink-based escapes if a symlink exists inside .agents/. Low risk since agentName comes from server-controlled config, but worth hardening. [fixable]

dimakis · 2026-06-04T22:58:35Z

+    const data = (await res.json()) as Record<string, unknown>;
+
+    // ContexGin returns identity, provider, governance, memory at the top level
+    const identity = data.identity as AgentIdentity | undefined;


🔵 unsafe_assumptions: The ContexGin response fields (identity, provider, governance, memory) are cast with as without validation — if ContexGin returns e.g. identity: "string" instead of an object, the code proceeds with a malformed definition stored in the session. The local YAML path has the same issue. Since this is Phase 1 and the data is from a trusted internal service, this is low-risk, but a lightweight Zod schema would prevent silent corruption. [fixable]

dimakis · 2026-06-04T22:58:35Z

+    .then((loaded) => {
+      const s = registry.get(clientId);
+      if (s) {
+        s.agentDefinition = loaded.definition as unknown as Record<string, unknown>;


🔵 unsafe_assumptions: The double cast loaded.definition as unknown as Record<string, unknown> is needed because ManagedSession stores agentDefinition as Record<string, unknown> | null while the loader returns a typed AgentDefinition. Consider typing ManagedSession.agentDefinition as AgentDefinition | null (importing the type from agent-loader) to eliminate the cast and get type safety on consumers. The untyped Record makes downstream reads unsafe. [fixable]

dimakis

Centaur Review

Found 8 issue(s) (1 critical) (4 warning).

`server/agent-loader.ts`

Missing js-yaml dependency will crash at runtime. Beyond that, solid implementation with good error handling and test coverage, but the duplicate type definition and Record<string, unknown> typing in ManagedSession should be cleaned up before merge.

🔴 bugs (L14): js-yaml is imported but is not declared as a dependency in any package.json (root, server, or any workspace package), and node_modules/js-yaml does not exist. This will crash at runtime with ERR_MODULE_NOT_FOUND. Either add js-yaml (and @types/js-yaml) to the root or server dependencies, or switch to the yaml package if already present. [fixable]
🟡 style (L89): Duplicate type definition: AgentDefinitionSource is defined identically in both server/agent-loader.ts:89 and packages/harness/src/session-registry.ts:12. The session-registry version is authoritative (it's used in the ManagedSession interface). agent-loader.ts should import it from @mitzo/harness instead of redeclaring it, to avoid drift. [fixable]
🔵 unsafe_assumptions (L169): The path traversal guard !filePath.startsWith(agentsDir + '/') is correct for Unix but would fail on Windows where path.sep is \. Not a practical issue for this project (macOS/Linux deployment), but worth noting if portability is ever a goal. The agentName is also used verbatim as a filename — names containing /, \, or null bytes could produce unexpected paths even after resolve. Consider validating agentName against a safe pattern (e.g., /^[a-z0-9-]+$/). [fixable]
🔵 style (L215): Cache key includes contexginUrl ?? 'default' but contexginUrl is already defaulted to a non-nullish string on line 212 (process.env.CONTEXGIN_URL || 'http://localhost:8321'), so the nullish coalescing is dead code. [fixable]

`packages/harness/src/session-registry.ts`

Missing js-yaml dependency will crash at runtime. Beyond that, solid implementation with good error handling and test coverage, but the duplicate type definition and Record<string, unknown> typing in ManagedSession should be cleaned up before merge.

🟡 regressions (L56): agentDefinition is declared as a required (non-optional) field on ManagedSession, but it's loaded asynchronously via fire-and-forget in chat.ts. Any code that accesses session.agentDefinition immediately after register() will see null even on successful loads. Consider making the field optional (agentDefinition?: ...) to signal that it may not be populated yet, or document the async contract clearly. [fixable]

`server/chat.ts`

Missing js-yaml dependency will crash at runtime. Beyond that, solid implementation with good error handling and test coverage, but the duplicate type definition and Record<string, unknown> typing in ManagedSession should be cleaned up before merge.

🟡 unsafe_assumptions (L919): Double type cast loaded.definition as unknown as Record<string, unknown> is a code smell indicating the ManagedSession.agentDefinition field should use the AgentDefinition type from agent-loader.ts instead of Record<string, unknown>. This erases all type safety — consumers will need their own casts to use the definition. Consider typing ManagedSession.agentDefinition as AgentDefinition | null and re-exporting the type from @mitzo/harness. [fixable]
🟡 unsafe_assumptions (L917): When registry.get(clientId) returns undefined (session already torn down during the async load), the definition is silently discarded with no log. The log.info at line 922 still runs and reports success. Move the log inside the if (s) block or add a warn in the else branch so operators can see when the race is lost. [fixable]

`server/tests/agent-loader.test.ts`

Missing js-yaml dependency will crash at runtime. Beyond that, solid implementation with good error handling and test coverage, but the duplicate type definition and Record<string, unknown> typing in ManagedSession should be cleaned up before merge.

🔵 missing_tests: The test suite covers the loader itself well, but there are no tests for the integration in chat.ts — specifically the fire-and-forget attachment to the session registry, the race where registry.get returns undefined, or the double type cast. A unit test for _startChatInner (or an integration test) verifying session.agentDefinition is populated after the promise settles would catch regressions in the wiring.

dimakis · 2026-06-04T23:01:15Z

+
+import { readFile } from 'fs/promises';
+import { resolve } from 'path';
+import { load as parseYaml } from 'js-yaml';


🔴 bugs: js-yaml is imported but is not declared as a dependency in any package.json (root, server, or any workspace package), and node_modules/js-yaml does not exist. This will crash at runtime with ERR_MODULE_NOT_FOUND. Either add js-yaml (and @types/js-yaml) to the root or server dependencies, or switch to the yaml package if already present. [fixable]

dimakis · 2026-06-04T23:01:15Z

+  output?: AgentOutput;
+}
+
+export type AgentDefinitionSource = 'contexgin' | 'local' | 'fallback';


🟡 style: Duplicate type definition: AgentDefinitionSource is defined identically in both server/agent-loader.ts:89 and packages/harness/src/session-registry.ts:12. The session-registry version is authoritative (it's used in the ManagedSession interface). agent-loader.ts should import it from @mitzo/harness instead of redeclaring it, to avoid drift. [fixable]

dimakis · 2026-06-04T23:01:15Z

+  try {
+    // Path traversal guard: ensure resolved path stays under .agents/
+    const agentsDir = resolve(cwd, '.agents');
+    const filePath = resolve(agentsDir, `${agentName}.yaml`);


🔵 unsafe_assumptions: The path traversal guard !filePath.startsWith(agentsDir + '/') is correct for Unix but would fail on Windows where path.sep is \. Not a practical issue for this project (macOS/Linux deployment), but worth noting if portability is ever a goal. The agentName is also used verbatim as a filename — names containing /, \, or null bytes could produce unexpected paths even after resolve. Consider validating agentName against a safe pattern (e.g., /^[a-z0-9-]+$/). [fixable]

dimakis · 2026-06-04T23:01:15Z

+  contexginUrl: string = process.env.CONTEXGIN_URL || 'http://localhost:8321',
+): Promise<LoadedAgentDefinition> {
+  // Check cache
+  const cacheKey = `${agentName}:${cwd}:${contexginUrl ?? 'default'}`;


🔵 style: Cache key includes contexginUrl ?? 'default' but contexginUrl is already defaulted to a non-nullish string on line 212 (process.env.CONTEXGIN_URL || 'http://localhost:8321'), so the nullish coalescing is dead code. [fixable]

dimakis · 2026-06-04T23:01:15Z

  /** Cached boot_context payload for replay on reconnect/switch. */
  bootContext?: Record<string, unknown>;
+  /** Parsed agent definition (recipe). Null if loading failed. */
+  agentDefinition: Record<string, unknown> | null;


🟡 regressions: agentDefinition is declared as a required (non-optional) field on ManagedSession, but it's loaded asynchronously via fire-and-forget in chat.ts. Any code that accesses session.agentDefinition immediately after register() will see null even on successful loads. Consider making the field optional (agentDefinition?: ...) to signal that it may not be populated yet, or document the async contract clearly. [fixable]

dimakis · 2026-06-04T23:01:15Z

+    .then((loaded) => {
+      const s = registry.get(clientId);
+      if (s) {
+        s.agentDefinition = loaded.definition as unknown as Record<string, unknown>;


🟡 unsafe_assumptions: Double type cast loaded.definition as unknown as Record<string, unknown> is a code smell indicating the ManagedSession.agentDefinition field should use the AgentDefinition type from agent-loader.ts instead of Record<string, unknown>. This erases all type safety — consumers will need their own casts to use the definition. Consider typing ManagedSession.agentDefinition as AgentDefinition | null and re-exporting the type from @mitzo/harness. [fixable]

dimakis · 2026-06-04T23:01:15Z

+  // Fire-and-forget: load agent definition and store in session registry.
+  loadAgentDef(agentName, cwd)
+    .then((loaded) => {
+      const s = registry.get(clientId);


🟡 unsafe_assumptions: When registry.get(clientId) returns undefined (session already torn down during the async load), the definition is silently discarded with no log. The log.info at line 922 still runs and reports success. Move the log inside the if (s) block or add a warn in the else branch so operators can see when the race is lost. [fixable]

dimakis commented May 30, 2026

View reviewed changes

dimakis force-pushed the session/2026-05-30-e90bfb59aa9c branch from 8066d4d to cbca1e5 Compare May 30, 2026 11:27

dimakis commented May 30, 2026

View reviewed changes

dimakis and others added 2 commits June 4, 2026 22:48

dimakis force-pushed the session/2026-05-30-e90bfb59aa9c branch from cbca1e5 to 3437039 Compare June 4, 2026 21:55

dimakis commented Jun 4, 2026

View reviewed changes


		if (!res.ok) return null;

		const data = (await res.json()) as Record<string, unknown>;

Conversation

dimakis commented May 30, 2026

Summary

Test plan

Uh oh!

dimakis left a comment

Choose a reason for hiding this comment

Centaur Review

server/agent-loader.ts

server/chat.ts

server/__tests__/agent-loader.test.ts

packages/harness/src/session-registry.ts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimakis left a comment

Choose a reason for hiding this comment

Centaur Review

server/agent-loader.ts

server/chat.ts

packages/harness/src/session-registry.ts

server/__tests__/agent-loader.test.ts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimakis left a comment

Choose a reason for hiding this comment

Centaur Review

server/agent-loader.ts

server/chat.ts

server/__tests__/agent-loader.test.ts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

`server/agent-loader.ts`

`server/chat.ts`

`server/tests/agent-loader.test.ts`

`packages/harness/src/session-registry.ts`

`server/agent-loader.ts`

`server/chat.ts`

`packages/harness/src/session-registry.ts`

`server/tests/agent-loader.test.ts`

`server/agent-loader.ts`

`server/chat.ts`

`server/tests/agent-loader.test.ts`

`server/agent-loader.ts`

`packages/harness/src/session-registry.ts`

`server/chat.ts`

`server/tests/agent-loader.test.ts`