diff --git a/docs/designs/skill-serving.md b/docs/designs/skill-serving.md index b3078108..1cb239ab 100644 --- a/docs/designs/skill-serving.md +++ b/docs/designs/skill-serving.md @@ -112,9 +112,10 @@ path (measured here from a 77-character one). team repo, then the installed agents, then the package. `codebase`, `default`, `learning` and `share` are ordinary names: a directory a member created under one of them is the skill they are asking about, and the recall gate does not - apply to it. A config the gate cannot load does apply: which team repo and - agents are meant is then unknown, so `skill show share` refuses before - searching them. The two legacy directory names are the exception, by design: + apply to it. A config that cannot be loaded does apply: which team repo and + agents are meant is then unknown (`detectTeam`), so `skill show` searches + neither, refuses `share`, and answers any other name from the package alone, + saying what failed. The two legacy directory names are the exception, by design: `team-wiki-codebase` and `teamai-share-learnings` classify as `[builtin]` and are skipped by the push scan by name alone (`isCliOwnedSkillName`), because a tree with that name is one a pre-stub release wrote until the first pull has @@ -126,6 +127,8 @@ path (measured here from a 77-character one). - **`skill list` needs no team.** The human-readable listing prints the packaged catalog even before `teamai init`, with a hint for the team half, so a fresh machine can discover what the installed CLI serves the way `skill get` lets it. + With a config that cannot be loaded it prints the catalog too, but no team + listing, says what failed on stderr, and exits 1. ## Drift guards diff --git a/src/__tests__/config-not-initialized.test.ts b/src/__tests__/config-not-initialized.test.ts index b67884d1..645dfc8d 100644 --- a/src/__tests__/config-not-initialized.test.ts +++ b/src/__tests__/config-not-initialized.test.ts @@ -66,6 +66,32 @@ describe('requireInit: missing config versus unreadable config', () => { await expect(requireInit()).rejects.not.toBeInstanceOf(NotInitializedError); }); + it('says a team config that exists but fails validation is invalid, not missing', async () => { + // "not found. Check your repo path" sends the member after a path that is right. + const teamRepo = path.join(home, '.teamai', 'team-repo'); + fs.mkdirSync(teamRepo, { recursive: true }); + fs.writeFileSync(path.join(teamRepo, 'teamai.yaml'), 'team: 42\n'); + fs.writeFileSync( + path.join(home, '.teamai', 'config.yaml'), + `repo:\n localPath: ${teamRepo}\n remote: https://example.test/acme/team.git\nusername: tester\nscope: user\n`, + ); + + const error = String(await requireInit().catch((e: unknown) => e)); + expect(error).toContain(`${path.join(teamRepo, 'teamai.yaml')} could not be read: it is not a valid team config`); + expect(error).not.toContain('not found'); + }); + + it('still says a missing team config is not found', async () => { + const teamRepo = path.join(home, '.teamai', 'team-repo'); + fs.mkdirSync(teamRepo, { recursive: true }); + fs.writeFileSync( + path.join(home, '.teamai', 'config.yaml'), + `repo:\n localPath: ${teamRepo}\n remote: https://example.test/acme/team.git\nusername: tester\nscope: user\n`, + ); + + await expect(requireInit()).rejects.toThrow('Team config (teamai.yaml) not found'); + }); + it('logs the failing field of a config that fails validation, which the refusal points at', async () => { // The refusal says "the error is printed above"; a Zod JSON dump there // would bury the field under a line of `[`. diff --git a/src/__tests__/e2e/skill-serving-cli.test.ts b/src/__tests__/e2e/skill-serving-cli.test.ts index 2b6688e6..c9dd43e8 100644 --- a/src/__tests__/e2e/skill-serving-cli.test.ts +++ b/src/__tests__/e2e/skill-serving-cli.test.ts @@ -130,7 +130,8 @@ describe('teamai skill get / path CLI (e2e)', () => { encoding: 'utf8', }); expect(result.status, args.join(' ')).not.toBe(0); - expect(result.stderr, args.join(' ')).toContain('could not be read'); + // It names the file and where it breaks, which is what the member fixes. + expect(result.stderr, args.join(' ')).toMatch(/\.teamai[\\/]config\.yaml: .* at line \d+, column \d+/); expect(result.stdout + result.stderr, args.join(' ')).not.toContain('Not initialized'); expect(result.stdout + result.stderr, args.join(' ')).not.toContain('No team is set up'); } diff --git a/src/__tests__/skill-list-uninitialized.test.ts b/src/__tests__/skill-list-uninitialized.test.ts index 15adbcf5..37ccfa7f 100644 --- a/src/__tests__/skill-list-uninitialized.test.ts +++ b/src/__tests__/skill-list-uninitialized.test.ts @@ -1,10 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -const { autoDetectInit, findUnreadableProjectConfig, logDim, NotInitializedError } = vi.hoisted(() => ({ +const { autoDetectInit, findUnreadableProjectConfig, logDim, logError, NotInitializedError } = vi.hoisted(() => ({ autoDetectInit: vi.fn(), - // No project config under the test's cwd: the share gate goes on to autoDetectInit. - findUnreadableProjectConfig: vi.fn(async () => null), + findUnreadableProjectConfig: vi.fn(), logDim: vi.fn(), + logError: vi.fn(), NotInitializedError: class NotInitializedError extends Error {}, })); vi.mock('../config.js', () => ({ @@ -14,7 +14,7 @@ vi.mock('../config.js', () => ({ BROKEN_CONFIG_ADVICE: 'Fix the file, or move it aside and run `teamai init` to write a new one.', })); vi.mock('../utils/logger.js', () => ({ - log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), dim: logDim }, + log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: logError, debug: vi.fn(), dim: logDim }, setStderrOnly: vi.fn(() => false), })); @@ -32,7 +32,11 @@ describe('teamai skill list before init', () => { beforeEach(() => { stdout = ''; autoDetectInit.mockReset(); + // No project config under the test's cwd: detection goes on to autoDetectInit. + findUnreadableProjectConfig.mockReset(); + findUnreadableProjectConfig.mockResolvedValue(null); logDim.mockReset(); + logError.mockReset(); logSpy = vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { stdout += args.join(' ') + '\n'; }); @@ -61,7 +65,25 @@ describe('teamai skill list before init', () => { // member to run `teamai init` would send them to re-init over a real setup. autoDetectInit.mockRejectedValue(new Error('Team config (teamai.yaml) not found. Check your repo path.')); - await expect(skillList({})).rejects.toThrow('Team config (teamai.yaml) not found'); + await skillList({}); + + expect(process.exitCode).toBe(1); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('Team config (teamai.yaml) not found')); expect(logDim).not.toHaveBeenCalledWith(expect.stringContaining('Not initialized')); + // The packaged catalog needs no team, so it is still listed. + expect(stdout).toContain('teamai skill get core'); + }); + + it('does not list the team the user config names while the project config is unreadable', async () => { + // Detection skips the broken project file and would answer with the user + // config: another team's repo. + findUnreadableProjectConfig.mockResolvedValue('/work/proj/.teamai/config.yaml: bad indentation'); + + await skillList({}); + + expect(autoDetectInit).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + expect(logError).toHaveBeenCalledWith(expect.stringContaining('/work/proj/.teamai/config.yaml: bad indentation')); + expect(stdout).toContain('teamai skill get core'); }); }); diff --git a/src/__tests__/skill-show.test.ts b/src/__tests__/skill-show.test.ts index 93317563..16d2dd9d 100644 --- a/src/__tests__/skill-show.test.ts +++ b/src/__tests__/skill-show.test.ts @@ -86,11 +86,18 @@ function captureLogs() { }; } -async function runSkillShow(name: string, fx: Fixture, unreadableProjectConfig: string | null = null): Promise { +async function runSkillShow( + name: string, + fx: Fixture, + config: { unreadableProjectConfig?: string; loadError?: Error } = {}, +): Promise { vi.doMock('../config.js', async (importOriginal) => ({ ...(await importOriginal()), - autoDetectInit: async () => ({ localConfig: fx.localConfig, teamConfig: fx.teamConfig }), - findUnreadableProjectConfig: async () => unreadableProjectConfig, + autoDetectInit: async () => { + if (config.loadError) throw config.loadError; + return { localConfig: fx.localConfig, teamConfig: fx.teamConfig }; + }, + findUnreadableProjectConfig: async () => config.unreadableProjectConfig ?? null, })); const { skillShow } = await import('../skill-cmd.js'); const cap = captureLogs(); @@ -213,7 +220,7 @@ describe('skillShow locator', () => { }); let text: string; try { - text = (await runSkillShow('share', fx, '/work/proj/.teamai/config.yaml: bad indentation')).join('\n'); + text = (await runSkillShow('share', fx, { unreadableProjectConfig: '/work/proj/.teamai/config.yaml: bad indentation' })).join('\n'); } finally { errorSpy.mockRestore(); } @@ -224,6 +231,28 @@ describe('skillShow locator', () => { process.exitCode = 0; }); + it('does not search the team the user config names while the project config is unreadable', async () => { + await makeSkill(path.join(fx.repoPath, 'skills'), 'other-team-skill', 'belongs to the user-scope team'); + + const text = (await runSkillShow('other-team-skill', fx, { unreadableProjectConfig: '/work/proj/.teamai/config.yaml: bad indentation' })).join('\n'); + const { log } = await import('../utils/logger.js'); + expect(process.exitCode).toBe(1); + expect(text).not.toContain(fx.repoPath); + expect(vi.mocked(log.dim)).toHaveBeenCalledWith(expect.stringContaining('/work/proj/.teamai/config.yaml: bad indentation')); + process.exitCode = 0; + }); + + it('shows a packaged skill when the config cannot be loaded, and says what failed instead of throwing', async () => { + const loadError = new Error('The teamai config at /h/.teamai/config.yaml could not be read: it is empty.'); + + const text = (await runSkillShow('core', fx, { loadError })).join('\n'); + const { log } = await import('../utils/logger.js'); + expect(text).toContain('skill: core'); + expect(vi.mocked(log.error)).toHaveBeenCalledWith(expect.stringContaining('could not be read: it is empty')); + expect(process.exitCode).toBe(1); + process.exitCode = 0; + }); + it('shows share once recall is enabled', async () => { fx.localConfig.recallEnabled = true; const lines = await runSkillShow('share', fx); diff --git a/src/config.ts b/src/config.ts index 2866095a..0ae44634 100644 --- a/src/config.ts +++ b/src/config.ts @@ -143,9 +143,7 @@ export async function requireInit(): Promise { const localConfig = await loadLocalConfig(); if (!localConfig) return throwMissingOrInvalid(expandHome(getUserConfigPath())); const teamConfig = await loadTeamConfig(localConfig.repo.localPath); - if (!teamConfig) { - throw new Error('Team config (teamai.yaml) not found. Check your repo path.'); - } + if (!teamConfig) return throwTeamConfigMissingOrInvalid(localConfig.repo.localPath); return { localConfig, teamConfig }; } @@ -167,6 +165,24 @@ async function throwMissingOrInvalid(configPath: string): Promise { throw new Error(`The teamai config at ${configPath} could not be read: ${why}. ${BROKEN_CONFIG_ADVICE}`); } +/** + * `loadTeamConfig` returns null both when teamai.yaml is absent and when it + * could not be used (it logs a parse or validation error). Only the first is + * "not found": "check your repo path" sends the member after a path that is + * right. + */ +async function throwTeamConfigMissingOrInvalid(repoPath: string): Promise { + const teamConfigPath = path.join(repoPath, 'teamai.yaml'); + if (!(await pathExists(teamConfigPath))) { + throw new Error('Team config (teamai.yaml) not found. Check your repo path.'); + } + const content = await readFileSafe(teamConfigPath); + const why = content === null ? 'the file could not be opened' + : content.trim() === '' ? 'it is empty' + : 'it is not a valid team config (the error is printed above)'; + throw new Error(`The team config at ${teamConfigPath} could not be read: ${why}. Fix it in the team repo, or ask a team admin to.`); +} + // ─── Scope-aware config loading ───────────────────────── /** @@ -488,9 +504,7 @@ export async function requireInitForScope( return throwMissingOrInvalid(expandHome(getConfigPath(scope, projectRoot))); } const teamConfig = await loadTeamConfig(localConfig.repo.localPath); - if (!teamConfig) { - throw new Error('Team config (teamai.yaml) not found. Check your repo path.'); - } + if (!teamConfig) return throwTeamConfigMissingOrInvalid(localConfig.repo.localPath); return { localConfig, teamConfig }; } @@ -503,9 +517,7 @@ export async function autoDetectInit(): Promise { const projectConfig = await detectProjectConfig(); if (projectConfig) { const teamConfig = await loadTeamConfig(projectConfig.repo.localPath); - if (!teamConfig) { - throw new Error('Team config (teamai.yaml) not found. Check your repo path.'); - } + if (!teamConfig) return throwTeamConfigMissingOrInvalid(projectConfig.repo.localPath); return { localConfig: projectConfig, teamConfig }; } return requireInit(); diff --git a/src/skill-cmd.ts b/src/skill-cmd.ts index f68eb460..385439db 100644 --- a/src/skill-cmd.ts +++ b/src/skill-cmd.ts @@ -1,5 +1,4 @@ import path from 'node:path'; -import { autoDetectInit, NotInitializedError } from './config.js'; import { log } from './utils/logger.js'; import { listDirs, pathExists } from './utils/fs.js'; import { SkillsHandler } from './resources/skills.js'; @@ -16,13 +15,14 @@ import { detectInstalledAgents, type ResolvedAgent } from './known-agents.js'; import { LEGACY_BUILTIN_SKILL_NAMES } from './builtin-skills.js'; import { BLOCK_NOTES, + detectTeam, refuseBlocked, resolveServableSkill, skillCatalog, type BlockedSkill, type ServableSkillResolution, } from './skill-content.js'; -import type { GlobalOptions, LocalConfig, TeamaiConfig } from './types.js'; +import type { GlobalOptions, LocalConfig } from './types.js'; const DESCRIPTION_MAX = 160; @@ -50,28 +50,22 @@ type LocatedSkill = ResolvedSkill | BlockedSkill; */ export async function skillShow(name: string, options: GlobalOptions): Promise { const served = await resolveServableSkill(name); - // A config the gate cannot load leaves the team unknown: detection skips a - // broken project file and falls back to the user config, whose repo and - // agents belong to another team. Refuse before searching them. - if (served.kind === 'blocked' && served.reason === 'config') { - refuseBlocked(served); - return; - } - let init: { localConfig: LocalConfig; teamConfig: TeamaiConfig }; - try { - init = await autoDetectInit(); - } catch (e) { - // A packaged skill needs no team: it ships with the CLI, so `teamai skill - // show core` still works on a machine that has never run `teamai init`. - // Only that case: a broken config is reported, not read as "no team". - if (!(e instanceof NotInitializedError)) throw e; + const team = await detectTeam(); + if (team.kind !== 'team') { + // Only the package can answer without a team: it ships with the CLI, so + // `teamai skill show core` still works on a machine that has never run + // `teamai init`. A config that cannot be loaded leaves the team unknown + // too, so the repo and agents detection would fall back to (another + // team's) are not searched, and the member is told what failed. if (served.kind === 'blocked') { refuseBlocked(served); return; } if (served.kind !== 'found') { log.error(`Skill "${name}" not found among the skills the installed CLI serves.`); - log.dim('Run `teamai init` first to search the team repo and installed agents too.'); + log.dim(team.kind === 'none' + ? 'Run `teamai init` first to search the team repo and installed agents too.' + : `The team repo and installed agents were not searched: the teamai config could not be loaded. ${team.detail}`); process.exitCode = 1; return; } @@ -85,10 +79,15 @@ export async function skillShow(name: string, options: GlobalOptions): Promise { + const { autoDetectInit, findUnreadableProjectConfig, NotInitializedError, BROKEN_CONFIG_ADVICE } = + await import('./config.js'); + // Loading the config can migrate it and say so with `log.info`. That line + // must not land in the skill content, the JSON these commands print on + // stdout, or a hook's reply, so config loading reports on stderr here. + const previous = setStderrOnly(true); + try { + const unreadable = await findUnreadableProjectConfig(); + if (unreadable) { + // A parse error spans several lines (a code frame); its first names the + // file, the line and the column, which is what the member acts on. + return { kind: 'unusable', detail: `${firstLine(unreadable)}. ${BROKEN_CONFIG_ADVICE}` }; + } + return { kind: 'team', init: await autoDetectInit() }; + } catch (e) { + if (e instanceof NotInitializedError) return { kind: 'none' }; + return { kind: 'unusable', detail: firstLine(e instanceof Error ? e.message : String(e)) }; + } finally { + setStderrOnly(previous); + } +} + /** * Whether `share` can be served here. The Stop-hook reminder asks this too, so * the nudge and `teamai skill get share` cannot disagree, and it reads its own @@ -93,40 +129,19 @@ export type ShareGate = * the docs gets the content rather than a refusal it cannot act on. A config * that exists but cannot be loaded blocks: whether recall is on, or the source * writable, is then unknown, and the workflow would fail at `teamai contribute`. - * Only loading the config is read as "cannot be loaded"; any other failure is - * a fault here and propagates. + * Any failure past loading the config is a fault here and propagates. */ export async function shareGate(): Promise { - const [{ autoDetectInit, findUnreadableProjectConfig, NotInitializedError, BROKEN_CONFIG_ADVICE }, { isRecallEnabled }] = - await Promise.all([import('./config.js'), import('./types.js')]); - // Loading the config can migrate it and say so with `log.info`. That line - // must not land in the skill content, the JSON these commands print on - // stdout, or a hook's reply, so config loading reports on stderr here. - const previous = setStderrOnly(true); - let loaded: TeamaiInit; - try { - // A broken project config is skipped by detection, which would then - // answer with the user config: another team's recall and source. - const unreadable = await findUnreadableProjectConfig(); - if (unreadable) { - // A parse error spans several lines (a code frame); its first names the - // file, the line and the column, which is what the member acts on. - const detail = `${firstLine(unreadable)}. ${BROKEN_CONFIG_ADVICE}`; - return { block: { reason: 'config', detail }, config: null }; - } - loaded = await autoDetectInit(); - } catch (e) { - if (e instanceof NotInitializedError) return { block: null, config: null }; - return { block: { reason: 'config', detail: firstLine(e instanceof Error ? e.message : String(e)) }, config: null }; - } finally { - setStderrOnly(previous); - } - const { localConfig, teamConfig } = loaded; + const { isRecallEnabled } = await import('./types.js'); + const team = await detectTeam(); + if (team.kind === 'none') return { block: null, config: null }; + if (team.kind === 'unusable') return { block: { reason: 'config', detail: team.detail }, config: null }; + const { localConfig, teamConfig } = team.init; // `teamai contribute` refuses a read-only source (read-only.ts), so the // workflow would fail at its last step after the agent did all the work. if (localConfig.repo?.kind === 'http') return { block: { reason: 'read-only' }, config: null }; if (!isRecallEnabled(localConfig, teamConfig)) return { block: { reason: 'recall' }, config: null }; - return { block: null, config: loaded }; + return { block: null, config: team.init }; } /** The first line of an error, without the colon that introduces its code frame. */