Verification — Skills root resolution + diagnostic surface

Setup

  • Branch: feature/fix-skills-root-env
  • Worktree: E:/AMS/.worktrees/claude/fix-skills-root-env
  • Implementation commit: 2872b70c
  • Audit commit: 642f76bf · Contract: 44f56ab3 · Packet: 24eda617
  • Origin base: 86e430fb (origin/main)

Gate 1 — npm run build 

> colibri@0.0.1 build
> tsc

(no output, exit 0)

tsc accepts the new COLIBRI_SKILLS_ROOT field on Config, the new skill_count field on HealthPayload, the new countSkills export, and all six modified test files. Schema/types are sound.

Gate 2 — npm run lint 

> colibri@0.0.1 lint
> eslint src

(no output, exit 0)

Initially failed with two @typescript-eslint/consistent-type-imports errors on the dynamic import() type annotations in src/__tests__/startup.test.ts. Resolved by hoisting the type via a top-level import type * as StartupModuleType from '../startup.js' + type StartupModule = typeof StartupModuleType declaration. Lint now clean.

Gate 3 — npm test 

Test Suites: 30 passed, 30 total
Tests:       1384 passed, 1384 total
Snapshots:   0 total
Time:        23.116 s

Pre-change baseline at 86e430fb was 1357 tests across 30 suites (per memory R83 close). Post-change is 1384 = baseline + 27 new tests across:

  • config.test.ts +5 cases (absolute path, relative path, undefined, empty rejection, freeze).
  • startup.test.ts +12 cases (2 resolution-chain, 2 mocked-config resolution, 6 WARN-gating, plus the eager-import deletion was a non-test refactor not a count change).
  • tools/health.test.ts +10 cases (7 countSkills unit, 3 payload, plus 2 E2E for skill_count; minus 0 from the 6→7 field assertion which is an in-place edit not a count change).

Coverage on the touched files:

File % stmts % branch % funcs % lines
src/config.ts 100% 80% 100% 100%
src/tools/health.ts 100% 100% 100% 100%
src/startup.ts 93.65% 92.18% 85.71% 93.54%

(startup.ts uncovered lines are pre-existing branches in the shutdown

  • subprocess paths, untouched by this change.)

Acceptance evidence vs. task spec

Criterion How verified
1. COLIBRI_SKILLS_ROOT optional in config schema with TSDoc src/config.ts shows the field at L48-71 with TSDoc explaining override semantics + AMS_* rejection + accepted values. loadConfig parses it as optional via z.string().min(1).optional(). Three positive cases + one rejection case in config.test.ts.
2. Resolution chain options.skillsRoot ?? config.COLIBRI_SKILLS_ROOT ?? cwd default src/startup.ts:299-302 shows the new ?? chain. The options.skillsRoot wins… test in startup.test.ts confirms the test-seam priority. The passes config.COLIBRI_SKILLS_ROOT… test (mocked config) confirms the env-var path. The uses cwd default… test confirms the fallback.
3. skill_count on server_health payload src/tools/health.ts:88-95 schema field. src/tools/health.ts:135-149 countSkills helper. src/tools/health.ts:163 payload assembly. E2E test returns skill_count reflecting a populated skills table over the SDK envelope confirms the field surfaces through the full SDK envelope.
4. WARN log on loaded=0 AND total_on_disk=0 AND mode=FULL src/startup.ts:316-326 shows the conditional. Six tests in startup — empty-skills WARN diagnostic exercise every gating branch (FULL fires; READONLY/TEST/MINIMAL silent; non-zero total_on_disk silent; non-zero loaded silent).
5. New test surfaces for config/startup/health Counted above (+5/+12/+10).
6. npm run build && npm run lint && npm test all green Gates above.

Constraint compliance

  • 14-tool surface unchanged. skill_count is a payload field on the existing server_health tool. No new MCP tool registered. The _registeredToolNames.has('server_health') assertion in the existing tests (which counts unique MCP tool names) is unaffected.
  • Worktree only. Every edit landed in E:/AMS/.worktrees/claude/fix-skills-root-env. Main checkout (E:/AMS) untouched.
  • No push, no merge. Local commits on feature/fix-skills-root-env only. git push not invoked.
  • No new migration. The skills table already exists at src/db/migrations/004_skills.sql:18-29. countSkills reads from it via SELECT COUNT(*) AS c FROM skills.

Design decisions

  1. Path resolution: COLIBRI_SKILLS_ROOT is consumed verbatim. We do NOT call path.resolve() to absolutize it — loadSkillsFromDisk already calls path.resolve(skillsRoot) internally (see src/domains/skills/repository.ts:238), so a relative value gets resolved against process.cwd() at the call site, identical to how the bare .agents/skills default works today. This keeps debugging predictable: the value the operator sets is the value fs sees.

  2. Empty-string rejection: z.string().min(1).optional() rejects an empty string at module-load time. An operator who exports COLIBRI_SKILLS_ROOT="" gets a validation error immediately, not a silent zero-skills boot.

  3. WARN gating: restricted-mode deploys (READONLY/TEST/MINIMAL) may legitimately ship without skills. A populated disk inventory with loaded=0 is a parser-error scenario already logged inside the loader. The diagnostic targets only the total_on_disk === 0 && loaded === 0 && mode === 'FULL' shape — that’s the misconfiguration the code review surfaced.

  4. WARN line text frozen: the line [Startup] WARNING: 0 skills loaded — check COLIBRI_SKILLS_ROOT or process.cwd() is intentionally stable. Operators may grep for it. Future changes to the gating conditions must keep this exact substring.

  5. Test mocking strategy: the env-var-to-startup chain is normally tested via subprocess (mirroring the existing subprocess smoke pattern), but on Windows + tsx cold-start, Phase 2 (DB integrity check + 6 migrations + skills load) routinely exceeds 15 s — well beyond Jest’s per-test budget. Switched to jest.unstable_mockModule to stub the config import, then dynamically re-imported startup so it picks up the mock. Imported jest from @jest/globals (already used in db-init.test.ts); cast for the unstable API (not in @types/jest@29.5.14).

Test fixture impact

  • src/__tests__/tools/health.test.ts:299-306 — pre-existing 6-field key-set assertion in buildHealthPayload’s “complete payload shape” test. Updated to 7 fields including skill_count.
  • No other existing test fixtures needed updating; the existing no-op loadSkillsFn: () => ({ loaded: 0, skipped: 0, pruned: 0, total_on_disk: 0 }) stub is now a default that trips the new WARN under the default mode: 'FULL' fixture, but the WARN is benign noise — no existing test asserts log absence (verified via grep across startup.test.ts).

Residual risks

  • First subprocess run latency: the existing startup — subprocess smoke test (startup.test.ts:1041) uses timeout: 3000 and only asserts on Phase 1 logs. It still passes reliably; no regression.
  • No live ζ writeback: no MCP client attached to this session. The task task_update + thought_record writeback lives in the PR body
    • memory file per CLAUDE.md §4 fallback path.
  • Pre-existing @types/jest gap: unstable_mockModule is not typed; the cast pattern is documented inline.

Rollback plan

If the WARN line proves noisy in production for non-FULL operators with skills configured (we don’t believe so — the gating is strict), the single-line revert is:

// src/startup.ts — drop the if-block at L316-326

COLIBRI_SKILLS_ROOT itself is purely additive and safe to keep. The skill_count payload field is similarly additive.

Back to top

Colibri — documentation-first MCP runtime. Apache 2.0 + Commons Clause.

This site uses Just the Docs, a documentation theme for Jekyll.