From b1d6e49b9085eafd11df1186cc30b56a7e7948a2 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 10:31:05 +0200 Subject: [PATCH 01/12] fix(skills): one share gate, actionable refusals, and a louder stub deploy Follow-ups from the review of #699: - The Stop-hook reminder and `teamai skill get share` ask one gate (`shareGate`, through `contributeHintAllowed`). The hook skipped the unreadable-project-config check, and the legacy `teamai contribute-check` command, still called by hooks written before the dispatcher, checked nothing, so both nudged towards a command that refused. - The gate reads only a config load failure as "cannot be loaded"; any other fault propagates (the hook withholds the reminder and logs it at debug). - A `config` refusal says what failed (the file and position for a parse error) instead of pointing at `teamai doctor`, which cannot see a broken config. `skill show` now refuses through the same helper, so its hint moves from stdout to stderr like `skill get` and `skill path`. - `pull` warns when the discovery stub cannot be deployed (it was an empty catch on the fast path and a debug line on a full sync), and so does the legacy prune. - Error text no longer claims a reason was logged when none was: an empty config is named as empty, and `init` points at ~/.teamai/debug.log, where every path that deploys nothing now records why. - `core` routes a bare `/teamai` right after a friction reminder to `share`, as the stub already said. - The command drift guard rejects an unknown subcommand inside a group (`teamai skill gett core` passed before). - The contribute-check e2e asserts the reminder's real text again; the usage guides (EN, zh-CN) and the design doc cover the config refusal, the gate and the reminder routing. --- docs/designs/skill-serving.md | 26 +-- docs/usage-guide.md | 6 +- docs/usage-guide.zh-CN.md | 5 +- skill-data/core/SKILL.md | 6 +- src/__tests__/config-not-initialized.test.ts | 11 ++ src/__tests__/contribute-check-e2e.test.ts | 16 +- src/__tests__/hook-handlers.test.ts | 35 ++++ src/__tests__/init.test.ts | 46 ++++-- src/__tests__/pull-skip-sync.test.ts | 33 ++++ src/__tests__/skill-commands-exist.test.ts | 20 ++- .../skill-list-uninitialized.test.ts | 11 +- src/__tests__/skill-recall-gate.test.ts | 45 ++++- src/builtin-skills.ts | 10 +- src/config.ts | 28 +++- src/contribute-check.ts | 5 + src/hook-handlers.ts | 28 +--- src/init.ts | 2 +- src/pull.ts | 10 +- src/skill-cmd.ts | 32 ++-- src/skill-content.ts | 155 +++++++++++++----- 20 files changed, 395 insertions(+), 135 deletions(-) diff --git a/docs/designs/skill-serving.md b/docs/designs/skill-serving.md index 7888ca16..62d9fc6e 100644 --- a/docs/designs/skill-serving.md +++ b/docs/designs/skill-serving.md @@ -94,12 +94,14 @@ path (measured here from a 77-character one). blocks instead (`blockedBy: "config"`), since recall and the source are then unknown and the workflow would fail at `teamai contribute` — a project config too, which detection alone would skip in favour of the user config - (`findUnreadableProjectConfig`). The Stop-hook reminder follows the same rule. The Stop-hook share - reminder is gated the same way (`contributeHintAllowed`, `src/hook-handlers.ts`), - because it points at this command. The gate lives in one place: - `resolveServableSkill` (`src/skill-content.ts`) is the only way to obtain a - packaged skill outside that module, and it returns `blocked` instead of the - skill, so a command cannot print a directory it never received. + (`findUnreadableProjectConfig`). The refusal then says what failed (for a file + that does not parse, which file and where), since nothing else reports it. The + Stop-hook share reminder asks the same gate (`contributeHintAllowed`, called + by the hook dispatcher and by the legacy `teamai contribute-check`), because it + points at this command. The gate lives in one place: `shareGate` + (`src/skill-content.ts`) decides it, and `resolveServableSkill` is the only + way to obtain a packaged skill outside that module; it returns `blocked` + instead of the skill, so a command cannot print a directory it never received. - **`skill path` takes a name, always,** and a blocked name gets the same refusal as `skill get`. The gate routes the agent away from a workflow that cannot finish; it is not access control, since the files ship in the package. @@ -130,8 +132,9 @@ Two tests, both in the unit suite: `npx vitest run commands-reference -u`. - `skill-commands-exist.test.ts` resolves every `teamai …` string written anywhere under `skill-data/` against that same table, and fails on an unknown - command or flag. It carries a case proving it catches `teamai extract graph`, - the command the wiki skill advertised for four releases. + command, subcommand or flag. It carries cases proving it catches + `teamai extract graph`, the command the wiki skill advertised for four + releases, and a misspelled subcommand inside a group (`teamai skill gett`). A third, in `skill-content.test.ts`, asserts through `npm pack` that both `skills/` and `skill-data/` are in the published tarball. Without it, a missing @@ -235,7 +238,8 @@ and can be dropped on the same schedule. `/teamai-share-learnings` was never a deployed slash command in its own right — it existed because the directory was installed. The Stop-hook nudge now names `/teamai share what this session taught me`, an invocation the core skill routes -to `share` (bare `/teamai` prints the menu and stops), and carries +to `share` (so does a bare `/teamai` typed right after the reminder; otherwise +bare `/teamai` prints the menu and stops), and carries `teamai skill get share` literally, so an agent can act on it even without -inferring the intent. It is withheld while recall is off, because that command -refuses then. +inferring the intent. It is withheld wherever that command refuses (recall off, +a read-only HTTP source, a config that cannot be loaded). diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 1a1762d4..74838df8 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -481,7 +481,9 @@ yours and stays. A directory that also holds a file of your own is kept, with on packaged files removed, and named in the pull output. `share` is served only while recall is on (off by default; `sharing.recall.enabled: true` in `teamai.yaml` for the team, or `teamai recall enable` for one machine): until then `teamai skill get share` refuses and says so. -It also refuses on a read-only HTTP source, where `teamai contribute` cannot write. The legacy names still +It also refuses on a read-only HTTP source, where `teamai contribute` cannot write, and when a +teamai config exists but cannot be loaded (the refusal says what failed, and for a file that does not parse, which file and where), +since recall and the source are then unknown. The legacy names still resolve: `teamai skill get team-wiki-codebase` serves `wiki`. --- @@ -947,7 +949,7 @@ Teams that route knowledge sharing through their own review flow (for example, a Only the nudge is affected: friction scoring, `teamai contribute --file`, and `/teamai` keep working when invoked manually. -The reminder is also withheld while recall is off (the default until `sharing.recall.enabled: true` in `teamai.yaml`, or `teamai recall enable` on one machine): it points at the `share` workflow, and `teamai skill get share` refuses until recall is on. It never appears on a read-only HTTP source, where `share` refuses too. +The reminder is also withheld while recall is off (the default until `sharing.recall.enabled: true` in `teamai.yaml`, or `teamai recall enable` on one machine): it points at the `share` workflow, and `teamai skill get share` refuses until recall is on. It never appears on a read-only HTTP source, or while a teamai config exists but cannot be loaded, where `share` refuses too. ### Searching knowledge diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 013e756d..38143ab7 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -453,7 +453,8 @@ teamai skill path wiki # 打印打包目录，用于运行 skill 只删除其中的打包文件，保留该目录和你的文件，并在 pull 输出中点名。`share` 只在开启 recall 后才会提供（默认关闭； 团队在 `teamai.yaml` 设置 `sharing.recall.enabled: true`，或单台机器运行 `teamai recall enable`）：在此之前， `teamai skill get share` 会拒绝并说明原因。 -只读 HTTP 源上它同样会拒绝，因为 `teamai contribute` 无法写入。旧名字仍然可用： +只读 HTTP 源上它同样会拒绝，因为 `teamai contribute` 无法写入；teamai 配置文件存在但无法加载时也会拒绝 +（提示会说明失败原因；若是文件无法解析，还会指出是哪个文件、哪一行），因为此时无法确定 recall 与来源。旧名字仍然可用： `teamai skill get team-wiki-codebase` 等价于 `wiki`。 --- @@ -914,7 +915,7 @@ teamai contribute --file /tmp/session.md --scope project 只影响提醒本身：摩擦评分、`teamai contribute --file` 和手动调用 `/teamai` 不受影响。 -未开启 recall 时（默认关闭；团队在 `teamai.yaml` 设置 `sharing.recall.enabled: true`，或单台机器运行 `teamai recall enable`）也不会显示这条提醒：提醒指向 `share` 工作流，而 recall 关闭时 `teamai skill get share` 会拒绝执行。只读 HTTP 源上这条提醒也从不出现，因为 `share` 同样会拒绝。 +未开启 recall 时（默认关闭；团队在 `teamai.yaml` 设置 `sharing.recall.enabled: true`，或单台机器运行 `teamai recall enable`）也不会显示这条提醒：提醒指向 `share` 工作流，而 recall 关闭时 `teamai skill get share` 会拒绝执行。只读 HTTP 源上，或 teamai 配置文件存在但无法加载时，这条提醒也从不出现，因为 `share` 同样会拒绝。 ### 搜索知识 diff --git a/skill-data/core/SKILL.md b/skill-data/core/SKILL.md index b9a02ca6..4cee29b8 100644 --- a/skill-data/core/SKILL.md +++ b/skill-data/core/SKILL.md @@ -15,7 +15,11 @@ do not skip, reorder, or invent commands. Look at what the user typed after `/teamai`. -**If they gave NO scenario** (bare `/teamai`, or only greetings/no task): +**If they gave NO scenario right after a TeamAI friction reminder** (the +`[teamai]` line that suggests `/teamai share what this session taught me`), +that reminder is the scenario: load `teamai skill get share` and follow it. + +**If they gave NO scenario otherwise** (bare `/teamai`, or only greetings/no task): print the menu below **exactly**, then **STOP and wait**. Take no other action — do not run any command, do not load another skill yet. diff --git a/src/__tests__/config-not-initialized.test.ts b/src/__tests__/config-not-initialized.test.ts index 6de26fb9..4e147ea6 100644 --- a/src/__tests__/config-not-initialized.test.ts +++ b/src/__tests__/config-not-initialized.test.ts @@ -46,6 +46,17 @@ describe('requireInit: missing config versus unreadable config', () => { expect(String(error)).toContain(configPath); }); + it('says an empty config is empty, since no loader logged anything for it', async () => { + const configPath = path.join(home, '.teamai', 'config.yaml'); + fs.mkdirSync(path.dirname(configPath), { recursive: true }); + fs.writeFileSync(configPath, ''); + + const error = await requireInit().catch((e: unknown) => e); + expect(error).not.toBeInstanceOf(NotInitializedError); + expect(String(error)).toContain(`${configPath} could not be read: it is empty`); + expect(String(error)).not.toContain('above'); + }); + it('is not NotInitializedError when the config parses but fails validation', async () => { const configPath = path.join(home, '.teamai', 'config.yaml'); fs.mkdirSync(path.dirname(configPath), { recursive: true }); diff --git a/src/__tests__/contribute-check-e2e.test.ts b/src/__tests__/contribute-check-e2e.test.ts index 9b1261ba..46c65231 100644 --- a/src/__tests__/contribute-check-e2e.test.ts +++ b/src/__tests__/contribute-check-e2e.test.ts @@ -64,6 +64,8 @@ function runContributeCheck( 'node', [CLI_PATH, 'contribute-check', '--stdin', '--tool', tool], { + // cwd too: the share gate reads a project config under it. + cwd: homeDir, env: { ...process.env, HOME: homeDir, TEAMAI_LOG_LEVEL: 'silent' }, timeout: 10000, }, @@ -194,7 +196,8 @@ describe('contribute-check E2E', () => { expect(parsed.hookSpecificOutput.additionalContext).not.toContain(RAW_GITHUB_TOKEN); expect(parsed.hookSpecificOutput.additionalContext).not.toContain('50 tool calls'); expect(parsed.hookSpecificOutput.additionalContext).not.toContain('7 different tools'); - expect(parsed.hookSpecificOutput.additionalContext).toContain('/teamai'); + expect(parsed.hookSpecificOutput.additionalContext).toContain('/teamai share what this session taught me'); + expect(parsed.hookSpecificOutput.additionalContext).toContain('teamai skill get share'); expect(parsed.stopReason).toBeUndefined(); // The real CLI persists hinted=true, so a repeated Stop hook is silent. @@ -203,6 +206,17 @@ describe('contribute-check E2E', () => { expect(repeated.stdout).toBe(''); }); + it('withholds the reminder where `teamai skill get share` refuses: a config that cannot be loaded', async () => { + // Hooks written before the dispatcher still call this command directly; it + // must ask the same gate, or it nudges towards a command that says no. + writeEventsFile(tmpHome, buildRichSessionEvents(SESSION_ID)); + fs.writeFileSync(path.join(tmpHome, '.teamai', 'config.yaml'), ''); + + const result = await runContributeCheck(tmpHome, makeStdinPayload(SESSION_ID)); + + expect(result.stdout).toBe(''); + }); + it('produces no output for a trivial session below threshold', async () => { writeEventsFile(tmpHome, buildTrivialSessionEvents(SESSION_ID)); diff --git a/src/__tests__/hook-handlers.test.ts b/src/__tests__/hook-handlers.test.ts index a0f12ab8..81705061 100644 --- a/src/__tests__/hook-handlers.test.ts +++ b/src/__tests__/hook-handlers.test.ts @@ -82,13 +82,17 @@ const mockAutoDetectInit = vi.fn().mockResolvedValue({ teamConfig: { team: 'test', repo: '', toolPaths: {}, sharing: { recall: { enabled: true } } }, }); +const mockFindUnreadableProjectConfig = vi.fn().mockResolvedValue(null); + vi.mock('../config.js', async (importOriginal) => ({ ...(await importOriginal()), autoDetectInit: mockAutoDetectInit, + findUnreadableProjectConfig: mockFindUnreadableProjectConfig, })); vi.mock('../utils/logger.js', () => ({ log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + setStderrOnly: vi.fn().mockReturnValue(false), })); vi.mock('../local-agent.js', () => ({ @@ -424,6 +428,37 @@ describe('hook-handlers registry', () => { expect(result).toContain('do share'); }); + it('contribute-check handler stays silent when the project config is unreadable, even if the user config loads', async () => { + const registry = buildHandlerRegistry(); + const handler = registry.find( + (r) => r.event === 'stop' && r.handler.name === 'contribute-check', + )!.handler; + // Detection skips the broken project file and loads the user config (recall + // on), but `teamai skill get share` refuses here, so the nudge would lead nowhere. + mockFindUnreadableProjectConfig.mockResolvedValueOnce('/x/.teamai/config.yaml: bad indentation'); + mockContributeCheckForSession.mockClear(); + + const result = await handler.execute({ session_id: 's5c', cwd: '/x' }, 'claude'); + expect(result).toBeNull(); + expect(mockContributeCheckForSession).not.toHaveBeenCalled(); + }); + + it('contribute-check handler withholds the reminder, without failing the turn, when the gate itself faults', async () => { + const registry = buildHandlerRegistry(); + const handler = registry.find( + (r) => r.event === 'stop' && r.handler.name === 'contribute-check', + )!.handler; + mockAutoDetectInit.mockResolvedValueOnce({ + localConfig: { get repo(): never { throw new TypeError('a bug past the config load'); } }, + teamConfig: { team: 'test', repo: '', toolPaths: {}, sharing: { recall: { enabled: true } } }, + }); + mockContributeCheckForSession.mockClear(); + + const result = await handler.execute({ session_id: 's5d', cwd: '/x' }, 'claude'); + expect(result).toBeNull(); + expect(mockContributeCheckForSession).not.toHaveBeenCalled(); + }); + it('contribute-check handler stays silent when a config exists but cannot be loaded', async () => { const registry = buildHandlerRegistry(); const handler = registry.find( diff --git a/src/__tests__/init.test.ts b/src/__tests__/init.test.ts index 77719dc7..a1c1b985 100644 --- a/src/__tests__/init.test.ts +++ b/src/__tests__/init.test.ts @@ -561,19 +561,14 @@ describe('init', () => { }); describe('deploys built-in skills after init', () => { - it('calls deployBuiltinSkills with teamConfig when loadTeamConfig returns non-null', async () => { + /** Init against a clone whose teamai.yaml loads, so the stub deploy runs. */ + async function initWithTeamConfig(): Promise { let cloneDone = false; - pathExistsFn = (p: string) => { - if (p === localPath) return cloneDone; - return false; - }; - + pathExistsFn = (p: string) => (p === localPath ? cloneDone : false); mockGfRepoClone.mockImplementation(() => { cloneDone = true; }); - - const mockedLoadTeamConfig = vi.mocked(await import('../config.js')).loadTeamConfig; - mockedLoadTeamConfig.mockResolvedValue({ + vi.mocked(await import('../config.js')).loadTeamConfig.mockResolvedValue({ team: 'my-team', repo: 'https://git.woa.com/HyperAI/teamai-test.git', provider: 'tgit', @@ -586,10 +581,12 @@ describe('init', () => { }, toolPaths: {}, } as never); - questionAnswers = ['n', '1']; - await init({ repo: 'https://git.woa.com/HyperAI/teamai-test.git', scope: 'user' }); + } + + it('calls deployBuiltinSkills with teamConfig when loadTeamConfig returns non-null', async () => { + await initWithTeamConfig(); expect(mockDeployBuiltinSkills).toHaveBeenCalled(); // No recall option: one stub deploys for everyone, and `teamai skill get @@ -599,6 +596,33 @@ describe('init', () => { expect.anything(), ); }); + + it('announces the stub as ready only when it landed', async () => { + const { log } = await import('../utils/logger.js'); + mockDeployBuiltinSkills.mockResolvedValueOnce(1); + + await initWithTeamConfig(); + + expect(log.info).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill is ready in your IDE')); + expect(log.warn).not.toHaveBeenCalledWith(expect.stringContaining('not deployed')); + }); + + it('says the stub reached no tool without pointing at output that may not exist', async () => { + // No installed tool logs at debug only, so "see the lines above" alone + // would be false, and `teamai doctor` has no check for the stub. + const { log } = await import('../utils/logger.js'); + mockDeployBuiltinSkills.mockResolvedValueOnce(0); + + await initWithTeamConfig(); + + const warned = vi.mocked(log.warn).mock.calls.map((call) => String(call[0])).join('\n'); + expect(warned).toContain('The built-in teamai skill was not deployed to any AI tool'); + expect(warned).toContain('teamai pull'); + expect(warned).toContain('~/.teamai/debug.log'); + expect(warned).not.toContain('see the lines above'); + expect(warned).not.toContain('teamai doctor'); + expect(log.info).not.toHaveBeenCalledWith(expect.stringContaining('is ready in your IDE')); + }); }); describe('scope path display', () => { diff --git a/src/__tests__/pull-skip-sync.test.ts b/src/__tests__/pull-skip-sync.test.ts index bbb7e235..eadbc4be 100644 --- a/src/__tests__/pull-skip-sync.test.ts +++ b/src/__tests__/pull-skip-sync.test.ts @@ -81,6 +81,12 @@ vi.mock('../update.js', () => ({ releaseLock: vi.fn().mockResolvedValue(undefined), })); +// The real deploy by default; a test makes it fail once to see what pull reports. +vi.mock('../builtin-skills.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, deployBuiltinSkills: vi.fn(actual.deployBuiltinSkills) }; +}); + import { pull, compileRecallRulesBlock, cleanupInactiveNamespaceSkills } from '../pull.js'; import { loadLocalConfigForScope, loadTeamConfig, detectProjectConfig, loadStateForScope, saveStateForScope } from '../config.js'; import { getHeadRev, createGit, pullRepo } from '../utils/git.js'; @@ -300,6 +306,33 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(vi.mocked(saveStateForScope).mock.calls[0][0].lastPullTargets).toEqual(['claude']); }); + it('warns when the built-in stub cannot be deployed on the revision fast path', async () => { + // The stub is the agent's only way into TeamAI: a failure to write it must + // reach the member, not vanish after "Already synced" has printed. + const { deployBuiltinSkills } = await import('../builtin-skills.js'); + vi.mocked(deployBuiltinSkills).mockRejectedValueOnce(new Error('EACCES: permission denied')); + vi.mocked(getHeadRev).mockResolvedValue('abc1234'); + vi.mocked(loadStateForScope).mockResolvedValue(emptyState({ lastPullRev: 'abc1234', lastPullTargets: ['claude'] })); + + await pull({}); + + expect(log.success).toHaveBeenCalledWith(expect.stringContaining('Already synced at abc1234, skipping')); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('EACCES: permission denied')); + }); + + it('warns when the built-in stub cannot be deployed on a full sync', async () => { + const { deployBuiltinSkills } = await import('../builtin-skills.js'); + vi.mocked(deployBuiltinSkills).mockRejectedValueOnce(new Error('EACCES: permission denied')); + vi.mocked(getHeadRev).mockResolvedValue('def5678'); + vi.mocked(loadStateForScope).mockResolvedValue(emptyState({ lastPullRev: 'abc1234' })); + + await pull({}); + + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('EACCES: permission denied')); + }); + it('should do full sync when HEAD rev differs from lastPullRev', async () => { await fse.writeFile(path.join(repoPath, 'rules', 'my-rule.md'), '# rule'); diff --git a/src/__tests__/skill-commands-exist.test.ts b/src/__tests__/skill-commands-exist.test.ts index f4180f57..62a2d2da 100644 --- a/src/__tests__/skill-commands-exist.test.ts +++ b/src/__tests__/skill-commands-exist.test.ts @@ -92,6 +92,18 @@ function validate(program: Command, invocations: Invocation[]): string[] { continue; } + // A group with subcommands and no argument of its own can only be followed + // by one of them (or a flag), so any other word is a subcommand that does + // not exist. Placeholders such as `` stand for one and are skipped. + const next = rest[0]; + if ( + next !== undefined && command.commands.length > 0 && command.registeredArguments.length === 0 + && !next.startsWith('-') && !/[<>[\]{}"'…]/.test(next) + ) { + problems.push(`${where} → unknown subcommand "${next}" for \`teamai ${command.name()}\``); + continue; + } + const flags = knownFlags(command, program); for (const token of rest) { if (!token.startsWith('-') || token === '-') continue; @@ -125,10 +137,16 @@ describe('commands named by the served skill content', () => { const problems = validate(program, [ { file: 'synthetic.md', line: 1, text: 'teamai extract graph' }, { file: 'synthetic.md', line: 2, text: 'teamai codebase --no-such-flag' }, + // A misspelled or renamed subcommand inside a group: the group matches, so + // without its own check the typo is read as an argument. + { file: 'synthetic.md', line: 3, text: 'teamai skill gett core' }, + // A group that takes an argument of its own is left alone: `enabel` is a query. + { file: 'synthetic.md', line: 4, text: 'teamai recall enabel' }, ]); - expect(problems).toHaveLength(2); + expect(problems).toHaveLength(3); expect(problems[0]).toContain('unknown command "extract"'); expect(problems[1]).toContain('unknown flag "--no-such-flag"'); + expect(problems[2]).toContain('unknown subcommand "gett" for `teamai skill`'); }); }); diff --git a/src/__tests__/skill-list-uninitialized.test.ts b/src/__tests__/skill-list-uninitialized.test.ts index d6b4384f..15adbcf5 100644 --- a/src/__tests__/skill-list-uninitialized.test.ts +++ b/src/__tests__/skill-list-uninitialized.test.ts @@ -1,11 +1,18 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -const { autoDetectInit, logDim, NotInitializedError } = vi.hoisted(() => ({ +const { autoDetectInit, findUnreadableProjectConfig, logDim, 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), logDim: vi.fn(), NotInitializedError: class NotInitializedError extends Error {}, })); -vi.mock('../config.js', () => ({ autoDetectInit, NotInitializedError })); +vi.mock('../config.js', () => ({ + autoDetectInit, + findUnreadableProjectConfig, + NotInitializedError, + 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 }, setStderrOnly: vi.fn(() => false), diff --git a/src/__tests__/skill-recall-gate.test.ts b/src/__tests__/skill-recall-gate.test.ts index 3fa9223c..602298fc 100644 --- a/src/__tests__/skill-recall-gate.test.ts +++ b/src/__tests__/skill-recall-gate.test.ts @@ -168,11 +168,17 @@ describe('recall gate on served skills', () => { it('blocks share when a config exists but cannot be loaded, since recall and the source are then unknown', async () => { autoDetectInit.mockRejectedValue(new Error('Team config (teamai.yaml) not found. Check your repo path.')); - expect(await resolveServableSkill('share')).toEqual({ kind: 'blocked', name: 'share', reason: 'config' }); + expect(await resolveServableSkill('share')).toEqual({ + kind: 'blocked', name: 'share', reason: 'config', + detail: 'Team config (teamai.yaml) not found. Check your repo path.', + }); await skillGet(['share']); expect(process.exitCode).toBe(1); expect(stdout).toBe(''); expect(stderr).toContain('config on this machine could not be loaded'); + // The refusal names what failed: `teamai doctor` cannot see a broken config. + expect(stderr).toContain('Team config (teamai.yaml) not found. Check your repo path.'); + expect(stderr).not.toContain('teamai doctor'); expect((await skillCatalog()).find((entry) => entry.name === 'share')).toMatchObject({ blockedBy: 'config', path: null }); // Only share depends on the config; the rest is still served. expect((await resolveServableSkill('core')).kind).toBe('found'); @@ -184,7 +190,42 @@ describe('recall gate on served skills', () => { findUnreadableProjectConfig.mockResolvedValue('/work/proj/.teamai/config.yaml: bad indentation'); withRecall(true); - expect(await resolveServableSkill('share')).toEqual({ kind: 'blocked', name: 'share', reason: 'config' }); + expect(await resolveServableSkill('share')).toEqual({ + kind: 'blocked', name: 'share', reason: 'config', + detail: '/work/proj/.teamai/config.yaml: bad indentation. ' + + 'Fix the file, or move it aside and run `teamai init` to write a new one.', + }); expect(autoDetectInit).not.toHaveBeenCalled(); + + await skillGet(['share']); + expect(process.exitCode).toBe(1); + expect(stderr).toContain('/work/proj/.teamai/config.yaml: bad indentation'); + expect(stderr).toContain('move it aside'); + }); + + it('keeps only the first line of a multi-line parse error, which names the file and the position', async () => { + // YAML errors end in a code frame and a newline; appended as-is, the + // advice would start a line of its own with a stray ". ". + findUnreadableProjectConfig.mockResolvedValue( + '/work/proj/.teamai/config.yaml: Unexpected flow-seq-end at line 1, column 7:\n\nrepo: [unclosed\n ^\n', + ); + + expect(await resolveServableSkill('share')).toMatchObject({ + reason: 'config', + detail: '/work/proj/.teamai/config.yaml: Unexpected flow-seq-end at line 1, column 7. ' + + 'Fix the file, or move it aside and run `teamai init` to write a new one.', + }); + }); + + it('lets a fault in the gate itself propagate instead of reporting it as a broken config', async () => { + // Only loading the config means "cannot be loaded"; anything else would + // print "the teamai config could not be loaded" over an unrelated bug. + const fault = new TypeError('a bug past the config load'); + autoDetectInit.mockResolvedValue({ + localConfig: { get repo(): never { throw fault; } }, + teamConfig: { sharing: { recall: { enabled: true } } }, + }); + + await expect(resolveServableSkill('share')).rejects.toBe(fault); }); }); diff --git a/src/builtin-skills.ts b/src/builtin-skills.ts index 99dcdbec..485cfcd8 100644 --- a/src/builtin-skills.ts +++ b/src/builtin-skills.ts @@ -445,7 +445,7 @@ export async function pruneLegacyBuiltinSkills( log.warn(`Kept "${legacyName}" (${tool}): ${dir} holds files TeamAI did not put there. The packaged files were removed${saved}; delete the rest yourself once you have saved what you need.`); } } catch (e) { - log.debug(`Could not remove legacy built-in skill ${legacyName} from ${tool}: ${(e as Error).message}`); + log.warn(`Could not finish removing "${legacyName}" (${tool}) from ${dir}: ${e instanceof Error ? e.message : String(e)}. Whatever is left there stays until the next pull, which tries again.`); } } } @@ -517,7 +517,8 @@ export async function deployBuiltinSkills(teamConfig: TeamaiConfig, localConfig? let entries: string[]; try { entries = await fs.promises.readdir(builtinDir); - } catch { + } catch (e) { + log.debug(`Could not list the built-in skills in ${builtinDir}, skipping deployment: ${e instanceof Error ? e.message : String(e)}`); return 0; } @@ -530,7 +531,10 @@ export async function deployBuiltinSkills(teamConfig: TeamaiConfig, localConfig? } } - if (skillNames.length === 0) return 0; + if (skillNames.length === 0) { + log.debug(`No built-in skill in ${builtinDir} has a SKILL.md, skipping deployment`); + return 0; + } let deployed = 0; diff --git a/src/config.ts b/src/config.ts index ad03deef..1a1081ea 100644 --- a/src/config.ts +++ b/src/config.ts @@ -129,10 +129,16 @@ export class NotInitializedError extends Error { readonly name = 'NotInitializedError'; } +/** The local config and the team config it points at, as the init checks return them. */ +export type TeamaiInit = { localConfig: LocalConfig; teamConfig: TeamaiConfig }; + +/** What to do about a config file that exists but cannot be used. */ +export const BROKEN_CONFIG_ADVICE = 'Fix the file, or move it aside and run `teamai init` to write a new one.'; + /** * Require that teamai is initialized (local config exists) */ -export async function requireInit(): Promise<{ localConfig: LocalConfig; teamConfig: TeamaiConfig }> { +export async function requireInit(): Promise { const localConfig = await loadLocalConfig(); if (!localConfig) return throwMissingOrInvalid(expandHome(getUserConfigPath())); const teamConfig = await loadTeamConfig(localConfig.repo.localPath); @@ -144,14 +150,20 @@ export async function requireInit(): Promise<{ localConfig: LocalConfig; teamCon /** * The loaders return null both when the config file is absent and when it could - * not be used (they log the reason). Only the first is "not initialized"; - * telling a member with a broken config to re-init sends them over a real setup. + * not be used. Only the first is "not initialized"; telling a member with a + * broken config to re-init sends them over a real setup. The loaders log a + * parse or validation error, but not a file that is empty or cannot be opened, + * so those two are named here. */ -async function throwMissingOrInvalid(configPath: string, notInitializedMessage = 'teamai is not initialized. Run `teamai init` first.'): Promise { - if (await pathExists(configPath)) { - throw new Error(`The teamai config at ${configPath} could not be read (the reason is logged above). Fix the file, or move it aside and run \`teamai init\` to write a new one.`); +async function throwMissingOrInvalid(configPath: string): Promise { + if (!(await pathExists(configPath))) { + throw new NotInitializedError('teamai is not initialized. Run `teamai init` first.'); } - throw new NotInitializedError(notInitializedMessage); + const content = await readFileSafe(configPath); + const why = content === null ? 'the file could not be opened' + : content.trim() === '' ? 'it is empty' + : 'it is not a valid teamai config (the error is printed above)'; + throw new Error(`The teamai config at ${configPath} could not be read: ${why}. ${BROKEN_CONFIG_ADVICE}`); } // ─── Scope-aware config loading ───────────────────────── @@ -475,7 +487,7 @@ export async function requireInitForScope( * If cwd has a project-scope config, uses that; otherwise falls back to user scope. * This is the recommended entry point for commands that support both scopes. */ -export async function autoDetectInit(): Promise<{ localConfig: LocalConfig; teamConfig: TeamaiConfig }> { +export async function autoDetectInit(): Promise { const projectConfig = await detectProjectConfig(); if (projectConfig) { const teamConfig = await loadTeamConfig(projectConfig.repo.localPath); diff --git a/src/contribute-check.ts b/src/contribute-check.ts index 1af35b69..5e3355c4 100644 --- a/src/contribute-check.ts +++ b/src/contribute-check.ts @@ -710,6 +710,11 @@ export async function contributeCheck(toolArg?: string): Promise { return; } + // The same gate as the dispatcher's handler: hooks written before it still + // call this command, and must not nudge towards a `share` that refuses. + const { contributeHintAllowed } = await import('./skill-content.js'); + if (!(await contributeHintAllowed())) return; + const { stopStdoutUnsupported } = await import('./utils/tool-names.js'); const tool = toolArg?.toLowerCase() ?? 'claude'; const { hint } = await contributeCheckForSession( diff --git a/src/hook-handlers.ts b/src/hook-handlers.ts index f28ee95c..52657d09 100644 --- a/src/hook-handlers.ts +++ b/src/hook-handlers.ts @@ -230,32 +230,6 @@ const trackSlashHandler: HookHandler = { }, }; -/** - * Whether the share-learnings hint may be emitted at all. Resolved lazily per - * hook run so a team can switch it off via teamai.yaml (or a member via local - * config) without re-injecting hooks. Falls back to enabled when there is no - * config at all, preserving pre-toggle behavior for half-initialized installs, - * where `teamai skill get share` serves too. A config that exists but cannot be - * loaded withholds it: `share` refuses there, so the nudge would lead nowhere. - * - * Recall and a writable source gate it too: the hint routes to the `share` - * workflow, and `teamai skill get share` refuses while recall is off or the - * team source is read-only HTTP, so a nudge towards it would send the agent to - * a command that says no. The dispatcher already drops this `gitOnly` handler - * for HTTP teams; the check here keeps the gate the same wherever it is called. - */ -async function contributeHintAllowed(): Promise { - const { isContributeHintEnabled, isRecallEnabled } = await import('./types.js'); - const { autoDetectInit, NotInitializedError } = await import('./config.js'); - try { - const { localConfig, teamConfig } = await autoDetectInit(); - return localConfig.repo?.kind !== 'http' - && isContributeHintEnabled(localConfig, teamConfig) - && isRecallEnabled(localConfig, teamConfig); - } catch (e) { - return e instanceof NotInitializedError ? isContributeHintEnabled({}, {}) : false; - } -} /** * Ask the model to declare which recalled documents it actually used. @@ -276,6 +250,7 @@ export function buildVotesNudge(recalledDocIds: readonly string[]): string { const contributeCheckHandler: HookHandler = { name: 'contribute-check', async execute(stdin, tool) { + const { contributeHintAllowed } = await import('./skill-content.js'); if (!(await contributeHintAllowed())) return null; const { contributeCheckForSession } = await import('./contribute-check.js'); @@ -318,6 +293,7 @@ const pendingHintHandler: HookHandler = { // Always consume the stash so a hint stashed before the team turned the // feature off is not delivered later when it is turned back on. const stashed = await pending.takePendingHint(sessionId); + const { contributeHintAllowed } = await import('./skill-content.js'); const hint = (await contributeHintAllowed()) ? stashed : null; const votesHint = await pending.takePendingVotesHint(sessionId); diff --git a/src/init.ts b/src/init.ts index e2e0efe1..6f3df190 100644 --- a/src/init.ts +++ b/src/init.ts @@ -1637,7 +1637,7 @@ export async function init(options: GlobalOptions & { if (stubDeployed > 0) { log.info('The built-in teamai skill is ready in your IDE; it loads its workflows with `teamai skill get`.'); } else { - log.warn('The built-in teamai skill was not deployed to any AI tool (see the lines above); run `teamai pull` once the cause is fixed, or `teamai doctor` to see it.'); + log.warn('The built-in teamai skill was not deployed to any AI tool, so agents cannot find TeamAI yet. The reason is printed above or recorded in ~/.teamai/debug.log; the usual one is that none of the selected tools is installed. Run `teamai pull` once it is fixed.'); } log.info('Skills, rules, env and docs auto-sync on each session start when the selected agent has active TeamAI hooks.'); log.info('Run `teamai status` to check current config.'); diff --git a/src/pull.ts b/src/pull.ts index eabdd31e..90af492a 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1015,7 +1015,13 @@ async function pullForScope( const skipRecall = !isRecallEnabled(localConfig, freshConfig); try { const { deployBuiltinAgents } = await import('./builtin-agents.js'); await deployBuiltinAgents(freshConfig, localConfig, { skipRecall }); } catch {} try { const { deployBuiltinRules } = await import('./builtin-rules.js'); await deployBuiltinRules(freshConfig, localConfig, { skipRecall }); } catch {} - try { const { deployBuiltinSkills } = await import('./builtin-skills.js'); await deployBuiltinSkills(freshConfig, localConfig); } catch {} + try { + const { deployBuiltinSkills } = await import('./builtin-skills.js'); + await deployBuiltinSkills(freshConfig, localConfig); + } catch (e) { + // The stub is the agent's only way into TeamAI, so this one is not silent. + log.warn(`[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`); + } // Refresh managed culture/shared-instruction blocks as well. A CLI // upgrade may add a new target file while the team repo SHA and tool // target set remain unchanged. @@ -1294,7 +1300,7 @@ async function pullForScope( log.debug(`[${scopeLabel}] Deployed ${deployed} built-in skill(s)`); } } catch (e) { - log.debug(`[${scopeLabel}] Built-in skills deployment skipped: ${(e as Error).message}`); + log.warn(`[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`); } } diff --git a/src/skill-cmd.ts b/src/skill-cmd.ts index fe6786dd..08f40ceb 100644 --- a/src/skill-cmd.ts +++ b/src/skill-cmd.ts @@ -14,7 +14,13 @@ import { } from './agent-skills.js'; import { detectInstalledAgents, type ResolvedAgent } from './known-agents.js'; import { LEGACY_BUILTIN_SKILL_NAMES } from './builtin-skills.js'; -import { blockMessage, resolveServableSkill, skillCatalog, type SkillBlockReason } from './skill-content.js'; +import { + BLOCK_NOTES, + refuseBlocked, + resolveServableSkill, + skillCatalog, + type BlockedSkill, +} from './skill-content.js'; import type { GlobalOptions, LocalConfig, TeamaiConfig } from './types.js'; const DESCRIPTION_MAX = 160; @@ -30,13 +36,6 @@ interface ResolvedSkill { namespace?: string; } -/** A packaged skill the serving gate withholds; there is no path to print. */ -interface BlockedSkill { - kind: 'blocked'; - name: string; - reason: SkillBlockReason; -} - type LocatedSkill = ResolvedSkill | BlockedSkill; /** @@ -59,10 +58,7 @@ export async function skillShow(name: string, options: GlobalOptions): Promise> = { */ const RECALL_DEPENDENT_SKILLS = new Set(['share']); -/** Why a served skill is withheld right now. */ -export type SkillBlockReason = 'recall' | 'read-only' | 'config'; +/** + * Why a served skill is withheld right now. A config that cannot be loaded + * carries what failed: nothing else reports it, because detection skips a + * broken project file and `teamai doctor` reads the one it falls back to. + */ +export type SkillBlock = + | { reason: 'recall' } + | { reason: 'read-only' } + | { reason: 'config'; detail: string }; + +export type SkillBlockReason = SkillBlock['reason']; + +/** + * The share gate's answer. A blocked answer carries no config: nothing past the + * gate may act on it. An open one carries the config it was decided on, or + * null when there is none on the machine. + */ +export type ShareGate = + | { block: SkillBlock; config: null } + | { block: null; config: TeamaiInit | null }; /** - * What makes this skill unusable right now, or null. + * 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 + * on/off switch from the config returned instead of loading it a second time. * * Fails open only where there is no config at all: a fresh install reading * 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. */ -async function blockReason(name: string): Promise { - if (!RECALL_DEPENDENT_SKILLS.has(name)) return null; - const { NotInitializedError } = await import('./config.js'); +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 { - const [{ autoDetectInit, findUnreadableProjectConfig }, { 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 or the JSON these commands print on - // stdout, so config loading reports on stderr for this one call. - const previous = setStderrOnly(true); - let loaded: Awaited>; - try { - // A broken project config is skipped by detection, which would then - // answer with the user config: another team's recall and source. - if (await findUnreadableProjectConfig()) return 'config'; - loaded = await autoDetectInit(); - } finally { - setStderrOnly(previous); + // 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 }; } - const { localConfig, teamConfig } = loaded; - // `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 'read-only'; - return isRecallEnabled(localConfig, teamConfig) ? null : 'recall'; + loaded = await autoDetectInit(); } catch (e) { - return e instanceof NotInitializedError ? null : 'config'; + 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; + // `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 }; +} + +/** The first line of an error, without the colon that introduces its code frame. */ +function firstLine(text: string): string { + return text.trim().split('\n')[0].trim().replace(/:$/, ''); +} + +/** + * Whether the Stop-hook share reminder may be shown. Resolved per hook run so + * a team can switch it off in teamai.yaml (or a member in local config) without + * re-injecting hooks; on by default, and with no config at all, where + * `teamai skill get share` serves too. + * + * The reminder routes to `share`, so it is withheld wherever `shareGate` + * blocks it: a nudge there would send the agent to a command that says no. + * Both the hook dispatcher and the legacy `teamai contribute-check` ask this. + */ +export async function contributeHintAllowed(): Promise { + const { isContributeHintEnabled } = await import('./types.js'); + let gate: ShareGate; + try { + gate = await shareGate(); + } catch (e) { + // A fault in the gate itself, not a config it could not load (the gate + // answers that): a Stop hook must not fail the turn over a reminder. + log.debug(`share gate failed, reminder withheld: ${e instanceof Error ? e.message : String(e)}`); + return false; + } + if (gate.block) return false; + return gate.config + ? isContributeHintEnabled(gate.config.localConfig, gate.config.teamConfig) + : isContributeHintEnabled({}, {}); +} + +/** What makes this skill unusable right now, or null. */ +async function blockReason(name: string): Promise { + if (!RECALL_DEPENDENT_SKILLS.has(name)) return null; + return (await shareGate()).block; } /** A skill directory that ships inside the npm package. */ @@ -196,9 +259,12 @@ async function resolvePackagedSkill( */ export type ServableSkillResolution = | { kind: 'found'; skill: PackagedSkill } - | { kind: 'blocked'; name: string; reason: SkillBlockReason } + | BlockedSkill | { kind: 'not-found'; name: string }; +/** A packaged skill the gate withholds: its canonical name and why. */ +export type BlockedSkill = { kind: 'blocked'; name: string } & SkillBlock; + /** * The only way to obtain a packaged skill outside this module. * @@ -212,14 +278,21 @@ export async function resolveServableSkill( ): Promise { const skill = await resolvePackagedSkill(name, roots); if (!skill) return { kind: 'not-found', name }; - const reason = await blockReason(skill.name); - if (reason) return { kind: 'blocked', name: skill.name, reason }; + const block = await blockReason(skill.name); + if (block) return { kind: 'blocked', name: skill.name, ...block }; return { kind: 'found', skill }; } +/** The short note `skill list` prints beside a blocked skill, per reason. */ +export const BLOCK_NOTES: Record = { + recall: 'needs recall — teamai recall enable', + 'read-only': 'not available on a read-only HTTP source', + config: 'not available: the teamai config could not be loaded', +}; + /** The two lines every command prints for a blocked skill. */ -export function blockMessage(name: string, reason: SkillBlockReason): { headline: string; hint: string } { - switch (reason) { +export function blockMessage(name: string, block: SkillBlock): { headline: string; hint: string } { + switch (block.reason) { case 'recall': return { headline: `${name} needs recall, which is disabled for this team.`, @@ -233,10 +306,10 @@ export function blockMessage(name: string, reason: SkillBlockReason): { headline case 'config': return { headline: `${name} is not available: the teamai config on this machine could not be loaded, so whether it can contribute is unknown.`, - hint: 'Run `teamai doctor` to see what is wrong with it, then try again.', + hint: block.detail, }; default: { - const exhaustive: never = reason; + const exhaustive: never = block; throw new Error(`Unhandled block reason ${String(exhaustive)}`); } } @@ -313,8 +386,8 @@ function rootsMissing(): void { * cannot finish whichever way it asks. A routing aid, not access control: the * files ship in the npm package either way. */ -function refuseBlocked(name: string, reason: SkillBlockReason): void { - const { headline, hint } = blockMessage(name, reason); +export function refuseBlocked(blocked: BlockedSkill): void { + const { headline, hint } = blockMessage(blocked.name, blocked); diagnostic(`${chalk.red('✖')} ${headline}`); diagnostic(` ${hint}`); process.exitCode = 1; @@ -360,7 +433,7 @@ export async function skillGet(names: string[], options: SkillGetOptions = {}): // either found or blocked. if (resolved.kind !== 'found') { if (resolved.kind === 'blocked') { - const { headline, hint } = blockMessage(listed.name, resolved.reason); + const { headline, hint } = blockMessage(listed.name, resolved); diagnostic(`${chalk.yellow('⚠')} Skipped ${listed.name}. ${headline} ${hint}`); } continue; @@ -375,7 +448,7 @@ export async function skillGet(names: string[], options: SkillGetOptions = {}): return; } if (resolved.kind === 'blocked') { - refuseBlocked(resolved.name, resolved.reason); + refuseBlocked(resolved); return; } targets.push(resolved.skill); @@ -412,7 +485,7 @@ export async function skillPath(name: string): Promise { notFound(name, await listServableSkills(roots)); return; case 'blocked': - refuseBlocked(resolved.name, resolved.reason); + refuseBlocked(resolved); return; case 'found': console.log(resolved.skill.dir); From 7b7c7bf50a6a6ffe3ddc41a5aca8d79ce2a7b0ea Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 11:07:13 +0200 Subject: [PATCH 02/12] test(learnings): retry temp-dir cleanup that races a detached git gc A push into the bare origin can leave `git gc --auto` writing to objects/pack after the test returns; the single rmdir in afterEach then fails with ENOTEMPTY (seen on CI, Node 22 ubuntu, #747). --- src/__tests__/git-kind-learnings.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/__tests__/git-kind-learnings.test.ts b/src/__tests__/git-kind-learnings.test.ts index 8f44ecdc..068aa54b 100644 --- a/src/__tests__/git-kind-learnings.test.ts +++ b/src/__tests__/git-kind-learnings.test.ts @@ -26,7 +26,9 @@ beforeEach(() => { afterEach(() => { process.env.HOME = originalHome; - fs.rmSync(tmp, { recursive: true, force: true }); + // A push into the bare origin can leave a detached `git gc --auto` writing to + // objects/pack after the test returns, so a single rmdir races it (ENOTEMPTY). + fs.rmSync(tmp, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); }); async function configureGit(dir: string): Promise { From 88829dd5556d8e1a6c480c41236f004ebd428c15 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 13:05:23 +0200 Subject: [PATCH 03/12] fix(skills): gate skill show before its lookups, name the failing field Review of #747: - `skill show share` under a broken project config searched the user config's team repo and agents, which detection falls back to, and printed a `share` found there. It now asks the gate first and refuses on a config block before any lookup. With an empty user config it refuses instead of ending in a stack trace. - A config that parses but fails validation reported the Zod JSON dump, whose first line is `[`, so the refusal said `config.yaml: [.`. Every config loader now reports each issue as `field: reason` on one line. - The docs and skills that describe the share reminder or the refusal say it is withheld on a read-only source and while the config cannot be loaded, and that a validation failure names the field: product-overview and usage-guide (EN, zh-CN), designs/skill-serving.md, core/SKILL.md, contribute-member, setup-admin, join-member and manage-admin. --- docs/designs/skill-serving.md | 7 +++-- docs/product-overview.md | 2 +- docs/product-overview.zh-CN.md | 2 +- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- skill-data/core/SKILL.md | 3 +- .../core/references/contribute-member.md | 2 +- skill-data/setup/references/join-member.md | 3 +- skill-data/setup/references/manage-admin.md | 6 ++-- skill-data/setup/references/setup-admin.md | 3 +- src/__tests__/config-not-initialized.test.ts | 27 ++++++++++++++++++ src/__tests__/skill-show.test.ts | 28 ++++++++++++++++++- src/config.ts | 20 ++++++++++--- src/skill-cmd.ts | 26 +++++++++++------ 14 files changed, 107 insertions(+), 26 deletions(-) diff --git a/docs/designs/skill-serving.md b/docs/designs/skill-serving.md index 62d9fc6e..da898fb1 100644 --- a/docs/designs/skill-serving.md +++ b/docs/designs/skill-serving.md @@ -95,7 +95,8 @@ path (measured here from a 77-character one). unknown and the workflow would fail at `teamai contribute` — a project config too, which detection alone would skip in favour of the user config (`findUnreadableProjectConfig`). The refusal then says what failed (for a file - that does not parse, which file and where), since nothing else reports it. The + that does not parse, which file and where; for one that fails validation, + which field and why), since nothing else reports it. The Stop-hook share reminder asks the same gate (`contributeHintAllowed`, called by the hook dispatcher and by the legacy `teamai contribute-check`), because it points at this command. The gate lives in one place: `shareGate` @@ -109,7 +110,9 @@ 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. The two legacy directory names are the exception, by design: + 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: `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 diff --git a/docs/product-overview.md b/docs/product-overview.md index baadea9e..bf690908 100644 --- a/docs/product-overview.md +++ b/docs/product-overview.md @@ -121,7 +121,7 @@ Task: Fix duplicate project-level Hook injection Consider running `/teamai share what this session taught me` to summarize what you learned and share it with your team (or run `teamai skill get share`). ``` -The hint names the non-zero friction signals that triggered it and, when available, includes a redacted, single-line summary of the first task. The `share` workflow (`teamai skill get share`) summarizes the session and pushes a learning document directly to the team repo. Each session is prompted at most once. Teams can switch the hint off with `sharing.contributeHint.enabled: false` in `teamai.yaml` (members: `contributeHintEnabled` in local config) while keeping the rest of the Stop hook. The hint also needs recall to be on (it is off by default), because the workflow it points at is served only then. +The hint names the non-zero friction signals that triggered it and, when available, includes a redacted, single-line summary of the first task. The `share` workflow (`teamai skill get share`) summarizes the session and pushes a learning document directly to the team repo. Each session is prompted at most once. Teams can switch the hint off with `sharing.contributeHint.enabled: false` in `teamai.yaml` (members: `contributeHintEnabled` in local config) while keeping the rest of the Stop hook. The hint also needs recall to be on (it is off by default), because the workflow it points at is served only then. For the same reason it never appears on a read-only HTTP source, or while a teamai config exists but cannot be loaded. ### Team Knowledge Recall diff --git a/docs/product-overview.zh-CN.md b/docs/product-overview.zh-CN.md index 18c7370f..d1005c7a 100644 --- a/docs/product-overview.zh-CN.md +++ b/docs/product-overview.zh-CN.md @@ -121,7 +121,7 @@ Task: Fix duplicate project-level Hook injection Consider running `/teamai share what this session taught me` to summarize what you learned and share it with your team (or run `teamai skill get share`). ``` -提示会列出实际触发它的非零摩擦信号；如果能取得首个任务摘要，还会在脱敏、单行化后附上任务上下文。`share` 工作流（`teamai skill get share`）自动总结 session 经验并推送到团队仓库。每个 session 最多提示一次。团队可在 `teamai.yaml` 设置 `sharing.contributeHint.enabled: false` 关闭该提示（成员可用本地配置 `contributeHintEnabled` 覆盖），Stop hook 的其余功能不受影响。该提示还需要开启 recall（默认关闭），因为它指向的工作流只在 recall 开启时提供。 +提示会列出实际触发它的非零摩擦信号；如果能取得首个任务摘要，还会在脱敏、单行化后附上任务上下文。`share` 工作流（`teamai skill get share`）自动总结 session 经验并推送到团队仓库。每个 session 最多提示一次。团队可在 `teamai.yaml` 设置 `sharing.contributeHint.enabled: false` 关闭该提示（成员可用本地配置 `contributeHintEnabled` 覆盖），Stop hook 的其余功能不受影响。该提示还需要开启 recall（默认关闭），因为它指向的工作流只在 recall 开启时提供。同理，只读 HTTP 源上，或 teamai 配置文件存在但无法加载时，该提示从不出现。 ### 团队知识检索 diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 74838df8..f5abc47a 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -482,7 +482,7 @@ packaged files removed, and named in the pull output. `share` is served only whi on (off by default; `sharing.recall.enabled: true` in `teamai.yaml` for the team, or `teamai recall enable` for one machine): until then `teamai skill get share` refuses and says so. It also refuses on a read-only HTTP source, where `teamai contribute` cannot write, and when a -teamai config exists but cannot be loaded (the refusal says what failed, and for a file that does not parse, which file and where), +teamai config exists but cannot be loaded (the refusal says what failed: for a file that does not parse, which file and where; for one that fails validation, which field and why), since recall and the source are then unknown. The legacy names still resolve: `teamai skill get team-wiki-codebase` serves `wiki`. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 38143ab7..6592e310 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -454,7 +454,7 @@ teamai skill path wiki # 打印打包目录，用于运行 skill 团队在 `teamai.yaml` 设置 `sharing.recall.enabled: true`，或单台机器运行 `teamai recall enable`）：在此之前， `teamai skill get share` 会拒绝并说明原因。 只读 HTTP 源上它同样会拒绝，因为 `teamai contribute` 无法写入；teamai 配置文件存在但无法加载时也会拒绝 -（提示会说明失败原因；若是文件无法解析，还会指出是哪个文件、哪一行），因为此时无法确定 recall 与来源。旧名字仍然可用： +（提示会说明失败原因；若是文件无法解析，还会指出是哪个文件、哪一行；若是校验失败，还会指出是哪个字段、为何不合法），因为此时无法确定 recall 与来源。旧名字仍然可用： `teamai skill get team-wiki-codebase` 等价于 `wiki`。 --- diff --git a/skill-data/core/SKILL.md b/skill-data/core/SKILL.md index 4cee29b8..c81bcf58 100644 --- a/skill-data/core/SKILL.md +++ b/skill-data/core/SKILL.md @@ -66,7 +66,8 @@ Sharing a session's learnings needs no menu choice: TeamAI prompts on its own at the end of a session that produced something worth sharing, and that prompt means `teamai skill get share`. (Only when recall is on; it is off by default. The team turns it on with `sharing.recall.enabled: true` in `teamai.yaml`, a member with `teamai recall enable`; -while it is off, `teamai skill get share` says so.) +while it is off, or while the teamai config cannot be loaded, `teamai skill get share` +says so and why.) ## Global rules diff --git a/skill-data/core/references/contribute-member.md b/skill-data/core/references/contribute-member.md index 73590fde..5b7f8481 100644 --- a/skill-data/core/references/contribute-member.md +++ b/skill-data/core/references/contribute-member.md @@ -8,7 +8,7 @@ team"*, in whatever language they work in — then you run the publish for them. ## Which kind of contribution? - **A learning** (a lesson, a gotcha, how you solved something) → this is - **automatic** once recall is on (off by default): TeamAI prompts at the end of a session worth sharing and the + **automatic** once recall is on (off by default) and the teamai config loads: TeamAI prompts at the end of a session worth sharing and the dedicated `share` workflow (`teamai skill get share`) takes over (it summarizes the session and runs `teamai contribute`). The user does not come through this flow for it. (Step A below is only a manual fallback for while recall is off.) diff --git a/skill-data/setup/references/join-member.md b/skill-data/setup/references/join-member.md index 564b2a86..0f5f5489 100644 --- a/skill-data/setup/references/join-member.md +++ b/skill-data/setup/references/join-member.md @@ -143,7 +143,8 @@ Summarize the outcome **in the user's own language** (global rule 1). Cover: summarize and contribute it. They do **not** invoke `/teamai` for this. (This prompt only appears when recall is on — it is off by default; the admin turns it on in `teamai.yaml` (`sharing.recall.enabled`), a member with `teamai recall enable` — and the admin has not switched the - reminder off in `teamai.yaml`.) + reminder off in `teamai.yaml`. It also stays silent while their teamai config + cannot be loaded; `teamai skill get share` then names the file and the error.) 3. **They can also contribute a skill — just ask in plain language.** A member does not need to be an admin to publish a skill. They tell TeamAI something like *"share this xxx skill with my team"*, in their own language, and you diff --git a/skill-data/setup/references/manage-admin.md b/skill-data/setup/references/manage-admin.md index c8fe0ae3..56f44fe6 100644 --- a/skill-data/setup/references/manage-admin.md +++ b/skill-data/setup/references/manage-admin.md @@ -124,13 +124,15 @@ the team (`sharing.recall.enabled: true` in `teamai.yaml`, then `teamai push`; i off by default, and `teamai recall enable` turns it on for one machine only): at the end of a session worth sharing, TeamAI prompts the member and the dedicated `share` workflow (`teamai skill get share`) summarizes the session and runs -`teamai contribute`. Nobody has to invoke it by hand. +`teamai contribute`. Nobody has to invoke it by hand. (A member whose teamai config +cannot be loaded gets no prompt; `teamai skill get share` names the file and the error.) (Publishing a **reusable skill** someone authored is a different task — any member can do it, see `"$(teamai skill path core)/references/contribute-member.md"`.) ### Turn the sharing prompt on or off (admin) -The auto-share prompt is **on by default once recall is on**. To disable it team-wide, set this in +The auto-share prompt is **on by default once recall is on** (never on a read-only +HTTP source, or while a member's teamai config cannot be loaded). To disable it team-wide, set this in `teamai.yaml` and `teamai push`: ```yaml diff --git a/skill-data/setup/references/setup-admin.md b/skill-data/setup/references/setup-admin.md index acabd7b9..17698f6f 100644 --- a/skill-data/setup/references/setup-admin.md +++ b/skill-data/setup/references/setup-admin.md @@ -275,7 +275,8 @@ day-to-day work — they can keep letting the AI run things for them: its own at the end of a session worth sharing, and the `share` workflow (`teamai skill get share`) takes over. (Only once recall is on — off by default; turn it on team-wide with `sharing.recall.enabled: true` in `teamai.yaml`, then - `teamai push`.) + `teamai push`. Never on a read-only HTTP source, or while a member's teamai + config cannot be loaded.) Mention the underlying commands (`teamai push`, `teamai roles`, …) only as a note for users who *do* want them — the primary path is re-invoking `/teamai`. diff --git a/src/__tests__/config-not-initialized.test.ts b/src/__tests__/config-not-initialized.test.ts index 4e147ea6..b67884d1 100644 --- a/src/__tests__/config-not-initialized.test.ts +++ b/src/__tests__/config-not-initialized.test.ts @@ -11,6 +11,7 @@ vi.mock('../utils/logger.js', () => ({ import { NotInitializedError, detectProjectConfig, findUnreadableProjectConfig, requireInit } from '../config.js'; import { projectDataHome } from '../utils/partition.js'; +import { log } from '../utils/logger.js'; /** * `loadLocalConfig` returns null both for a missing file and for one it could @@ -64,6 +65,19 @@ describe('requireInit: missing config versus unreadable config', () => { await expect(requireInit()).rejects.not.toBeInstanceOf(NotInitializedError); }); + + 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 `[`. + const configPath = path.join(home, '.teamai', 'config.yaml'); + fs.mkdirSync(path.dirname(configPath), { recursive: true }); + fs.writeFileSync(configPath, 'username: 42\n'); + vi.mocked(log.error).mockClear(); + + await expect(requireInit()).rejects.toThrow('the error is printed above'); + expect(vi.mocked(log.error)).toHaveBeenCalledWith(expect.stringMatching(/username: Expected string, received number/)); + expect(vi.mocked(log.error).mock.calls.flat().join('')).not.toContain('\n'); + }); }); describe('findUnreadableProjectConfig', () => { @@ -97,6 +111,19 @@ describe('findUnreadableProjectConfig', () => { expect(await findUnreadableProjectConfig(dir)).toContain(configPath); }); + it('names the failing field of a project config that parses but fails validation, on one line', async () => { + // A Zod message is a JSON dump whose first line is `[`; the refusal keeps + // only the first line, so the field and the reason must lead. + const configPath = path.join(dir, '.teamai', 'config.yaml'); + fs.mkdirSync(path.dirname(configPath), { recursive: true }); + fs.writeFileSync(configPath, 'scope: project\nrepo: 42\n'); + + const problem = await findUnreadableProjectConfig(dir); + expect(problem).toContain(configPath); + expect(problem).toMatch(/repo: Expected object, received number/); + expect(problem).not.toContain('\n'); + }); + it('names a broken partition config even when the legacy .teamai/ config behind it loads', async () => { // The partition is authoritative; detection skips it when broken and lands // on the legacy config, which may belong to another team. diff --git a/src/__tests__/skill-show.test.ts b/src/__tests__/skill-show.test.ts index 616c6cfd..93317563 100644 --- a/src/__tests__/skill-show.test.ts +++ b/src/__tests__/skill-show.test.ts @@ -86,10 +86,11 @@ function captureLogs() { }; } -async function runSkillShow(name: string, fx: Fixture): Promise { +async function runSkillShow(name: string, fx: Fixture, unreadableProjectConfig: string | null = null): Promise { vi.doMock('../config.js', async (importOriginal) => ({ ...(await importOriginal()), autoDetectInit: async () => ({ localConfig: fx.localConfig, teamConfig: fx.teamConfig }), + findUnreadableProjectConfig: async () => unreadableProjectConfig, })); const { skillShow } = await import('../skill-cmd.js'); const cap = captureLogs(); @@ -198,6 +199,31 @@ describe('skillShow locator', () => { process.exitCode = 0; }); + it('refuses share under a broken project config before searching the user config it falls back to', async () => { + // Detection skips the broken project file and answers with the user config: + // another team's repo and agents, where a `share` directory is not the one + // this project would mean. + fx.localConfig.recallEnabled = true; + await makeSkill(path.join(fx.repoPath, 'skills'), 'share', 'the other team share skill'); + await makeSkill(path.join(fx.homeDir, '.claude', 'skills'), 'share', 'a user-scope share skill'); + + const stderr: string[] = []; + const errorSpy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + stderr.push(args.join(' ')); + }); + let text: string; + try { + text = (await runSkillShow('share', fx, '/work/proj/.teamai/config.yaml: bad indentation')).join('\n'); + } finally { + errorSpy.mockRestore(); + } + expect(process.exitCode).toBe(1); + expect(text).not.toContain('skill: share'); + expect(text).not.toContain(fx.repoPath); + expect(stderr.join('\n')).toContain('/work/proj/.teamai/config.yaml: bad indentation'); + 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 1a1081ea..2866095a 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,4 +1,5 @@ import YAML from 'yaml'; +import { ZodError } from 'zod'; import path from 'node:path'; import { TeamaiConfigSchema, @@ -63,7 +64,7 @@ export async function loadTeamConfig(repoPath: string): Promise { const parsed = LocalConfigSchema.parse(raw); return await migrateLegacyRoleConfig(parsed, configPath); } catch (e) { - log.error(`Invalid local config: ${(e as Error).message}`); + log.error(`Invalid local config: ${describeConfigError(e)}`); return null; } } @@ -195,7 +196,7 @@ export async function loadLocalConfigForScope( const parsed = LocalConfigSchema.parse(raw); return await migrateLegacyRoleConfig(parsed, configPath); } catch (e) { - log.error(`Invalid ${scope} config at ${configPath}: ${(e as Error).message}`); + log.error(`Invalid ${scope} config at ${configPath}: ${describeConfigError(e)}`); return null; } } @@ -440,11 +441,22 @@ export async function readConfigFrom( } return resolved; } catch (e) { - onUnreadable?.(configPath, (e as Error).message); + onUnreadable?.(configPath, describeConfigError(e)); return null; } } +/** + * One line naming what is wrong. A Zod message is a JSON dump of its issues, + * whose first line is `[`; each issue's field and reason is what a member fixes. + */ +function describeConfigError(e: unknown): string { + if (e instanceof ZodError) { + return e.issues.map((issue) => `${issue.path.join('.') || '(root)'}: ${issue.message}`).join('; '); + } + return e instanceof Error ? e.message : String(e); +} + /** * The project-scope config under `cwd` that exists but cannot be read, parsed * or validated, with the reason, or null. Detection skips such a file and falls diff --git a/src/skill-cmd.ts b/src/skill-cmd.ts index 08f40ceb..f68eb460 100644 --- a/src/skill-cmd.ts +++ b/src/skill-cmd.ts @@ -20,6 +20,7 @@ import { resolveServableSkill, skillCatalog, type BlockedSkill, + type ServableSkillResolution, } from './skill-content.js'; import type { GlobalOptions, LocalConfig, TeamaiConfig } from './types.js'; @@ -48,6 +49,14 @@ type LocatedSkill = ResolvedSkill | BlockedSkill; * we print under "Repo path" or "Installed in". */ 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(); @@ -56,24 +65,23 @@ export async function skillShow(name: string, options: GlobalOptions): Promise { const teamSkillsDir = path.join(localConfig.repo.localPath, 'skills'); @@ -217,7 +226,6 @@ async function locateSkill( // 4. Built-in skill served by the CLI, including legacy-name aliases. Last, // so it answers for the names nothing on this machine claims: `core` and // `wiki` live in the package, and the agent directory holds only the stub. - const served = await resolveServableSkill(name); if (served.kind === 'blocked') return served; if (served.kind === 'found') { return { kind: 'found', name: served.skill.name, primaryPath: served.skill.dir, primaryOrigin: 'builtin' }; From 9586fb6f8a7fba1e85e42f295050aca51537cc4a Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 13:28:10 +0200 Subject: [PATCH 04/12] fix(skills): no share reminder where teamai is not set up `contributeHintAllowed` fell open with no config at all, so a caller other than the dispatcher (the legacy `teamai contribute-check`) still nudged in projects that never set up teamai, which have no team to share with (#748). It now returns false there. Serving the skill stays fail-open. --- docs/designs/skill-serving.md | 4 +++- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- src/__tests__/codex-stop-hint.test.ts | 8 ++++++++ src/__tests__/contribute-check-e2e.test.ts | 21 ++++++++++++++++++++- src/__tests__/hook-handlers.test.ts | 8 +++++--- src/skill-content.ts | 7 ++++--- 7 files changed, 42 insertions(+), 10 deletions(-) diff --git a/docs/designs/skill-serving.md b/docs/designs/skill-serving.md index da898fb1..b3078108 100644 --- a/docs/designs/skill-serving.md +++ b/docs/designs/skill-serving.md @@ -99,7 +99,9 @@ path (measured here from a 77-character one). which field and why), since nothing else reports it. The Stop-hook share reminder asks the same gate (`contributeHintAllowed`, called by the hook dispatcher and by the legacy `teamai contribute-check`), because it - points at this command. The gate lives in one place: `shareGate` + points at this command, with one difference: with no config at all it stays + silent. The hook fires in every project on the machine, and a directory without + teamai has no team to share with (#748). The gate lives in one place: `shareGate` (`src/skill-content.ts`) decides it, and `resolveServableSkill` is the only way to obtain a packaged skill outside that module; it returns `blocked` instead of the skill, so a command cannot print a directory it never received. diff --git a/docs/usage-guide.md b/docs/usage-guide.md index f5abc47a..5d31f5fe 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -949,7 +949,7 @@ Teams that route knowledge sharing through their own review flow (for example, a Only the nudge is affected: friction scoring, `teamai contribute --file`, and `/teamai` keep working when invoked manually. -The reminder is also withheld while recall is off (the default until `sharing.recall.enabled: true` in `teamai.yaml`, or `teamai recall enable` on one machine): it points at the `share` workflow, and `teamai skill get share` refuses until recall is on. It never appears on a read-only HTTP source, or while a teamai config exists but cannot be loaded, where `share` refuses too. +The reminder is also withheld while recall is off (the default until `sharing.recall.enabled: true` in `teamai.yaml`, or `teamai recall enable` on one machine): it points at the `share` workflow, and `teamai skill get share` refuses until recall is on. It never appears on a read-only HTTP source, or while a teamai config exists but cannot be loaded, where `share` refuses too, nor in a directory where teamai is not set up, although `teamai skill get share` still serves there. ### Searching knowledge diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 6592e310..f2e24d54 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -915,7 +915,7 @@ teamai contribute --file /tmp/session.md --scope project 只影响提醒本身：摩擦评分、`teamai contribute --file` 和手动调用 `/teamai` 不受影响。 -未开启 recall 时（默认关闭；团队在 `teamai.yaml` 设置 `sharing.recall.enabled: true`，或单台机器运行 `teamai recall enable`）也不会显示这条提醒：提醒指向 `share` 工作流，而 recall 关闭时 `teamai skill get share` 会拒绝执行。只读 HTTP 源上，或 teamai 配置文件存在但无法加载时，这条提醒也从不出现，因为 `share` 同样会拒绝。 +未开启 recall 时（默认关闭；团队在 `teamai.yaml` 设置 `sharing.recall.enabled: true`，或单台机器运行 `teamai recall enable`）也不会显示这条提醒：提醒指向 `share` 工作流，而 recall 关闭时 `teamai skill get share` 会拒绝执行。只读 HTTP 源上，或 teamai 配置文件存在但无法加载时，这条提醒也从不出现，因为 `share` 同样会拒绝；在未配置 teamai 的目录中也不会出现，尽管 `teamai skill get share` 在那里仍会提供。 ### 搜索知识 diff --git a/src/__tests__/codex-stop-hint.test.ts b/src/__tests__/codex-stop-hint.test.ts index da14ca64..55f9e8f6 100644 --- a/src/__tests__/codex-stop-hint.test.ts +++ b/src/__tests__/codex-stop-hint.test.ts @@ -17,6 +17,14 @@ describe('Codex Stop hint handoff with persisted session state', () => { beforeEach(() => { tmpHome = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-codex-stop-')); process.env.HOME = tmpHome; + // The nudge needs a team with recall on; without any config it stays silent (#748). + const teamRepo = path.join(tmpHome, '.teamai', 'team-repo'); + fs.mkdirSync(teamRepo, { recursive: true }); + fs.writeFileSync(path.join(teamRepo, 'teamai.yaml'), 'team: acme\nrepo: https://example.test/acme/team.git\nsharing:\n recall:\n enabled: true\n'); + fs.writeFileSync( + path.join(tmpHome, '.teamai', 'config.yaml'), + `repo:\n localPath: ${teamRepo}\n remote: https://example.test/acme/team.git\nusername: tester\nscope: user\n`, + ); }); afterEach(() => { if (originalHome === undefined) delete process.env.HOME; diff --git a/src/__tests__/contribute-check-e2e.test.ts b/src/__tests__/contribute-check-e2e.test.ts index 46c65231..c04bf6f8 100644 --- a/src/__tests__/contribute-check-e2e.test.ts +++ b/src/__tests__/contribute-check-e2e.test.ts @@ -18,8 +18,17 @@ const SESSION_ID = 'e2e-test-session-001'; const RAW_GITHUB_TOKEN = 'ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789'; const FIRST_TASK = `Fix auth retry for ${RAW_GITHUB_TOKEN}\nthen add regression coverage`; +/** A temp HOME with a user-scope install and recall on: the nudge only runs where `share` is served (#748). */ function makeTmpHome(): string { - return fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-contribute-e2e-')); + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-contribute-e2e-')); + const teamRepo = path.join(home, '.teamai', 'team-repo'); + fs.mkdirSync(teamRepo, { recursive: true }); + fs.writeFileSync(path.join(teamRepo, 'teamai.yaml'), 'team: acme\nrepo: https://example.test/acme/team.git\nsharing:\n recall:\n enabled: true\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`, + ); + return home; } /** Build a hook STDIN JSON payload with a session_id. */ @@ -157,6 +166,16 @@ describe('contribute-check E2E', () => { fs.rmSync(tmpHome, { recursive: true, force: true }); }); + it('stays silent where teamai is not set up (#748)', async () => { + fs.rmSync(path.join(tmpHome, '.teamai', 'config.yaml')); + writeEventsFile(tmpHome, buildRichSessionEvents(SESSION_ID)); + + const result = await runContributeCheck(tmpHome, makeStdinPayload(SESSION_ID)); + expect(result.code).toBe(0); + expect(result.stdout).toBe(''); + expect(readSessionState(tmpHome, SESSION_ID)).toBeNull(); + }); + it('standalone Codex Stop queues the hint without emitting incompatible JSON', async () => { writeEventsFile(tmpHome, buildRichSessionEvents(SESSION_ID)); const result = await runContributeCheck(tmpHome, makeStdinPayload(SESSION_ID), 'codex'); diff --git a/src/__tests__/hook-handlers.test.ts b/src/__tests__/hook-handlers.test.ts index 81705061..05194204 100644 --- a/src/__tests__/hook-handlers.test.ts +++ b/src/__tests__/hook-handlers.test.ts @@ -415,17 +415,19 @@ describe('hook-handlers registry', () => { expect(mockContributeCheckForSession).not.toHaveBeenCalled(); }); - it('contribute-check handler keeps hinting when there is no config at all', async () => { + it('contribute-check handler stays silent when there is no config at all', async () => { + // A project that never set up teamai has no team to share with (#748). const { NotInitializedError } = await import('../config.js'); const registry = buildHandlerRegistry(); const handler = registry.find( (r) => r.event === 'stop' && r.handler.name === 'contribute-check', )!.handler; mockAutoDetectInit.mockRejectedValueOnce(new NotInitializedError('teamai is not initialized. Run `teamai init` first.')); - mockContributeCheckForSession.mockResolvedValueOnce({ hint: '[teamai] do share' }); + mockContributeCheckForSession.mockClear(); const result = await handler.execute({ session_id: 's5', cwd: '/x' }, 'claude'); - expect(result).toContain('do share'); + expect(result).toBeNull(); + expect(mockContributeCheckForSession).not.toHaveBeenCalled(); }); it('contribute-check handler stays silent when the project config is unreadable, even if the user config loads', async () => { diff --git a/src/skill-content.ts b/src/skill-content.ts index ab05685a..9dbb5248 100644 --- a/src/skill-content.ts +++ b/src/skill-content.ts @@ -137,8 +137,9 @@ function firstLine(text: string): string { /** * Whether the Stop-hook share reminder may be shown. Resolved per hook run so * a team can switch it off in teamai.yaml (or a member in local config) without - * re-injecting hooks; on by default, and with no config at all, where - * `teamai skill get share` serves too. + * re-injecting hooks; on by default. Never with no config at all: a project + * that never set up teamai has no team to share with, although + * `teamai skill get share` still serves there. * * The reminder routes to `share`, so it is withheld wherever `shareGate` * blocks it: a nudge there would send the agent to a command that says no. @@ -158,7 +159,7 @@ export async function contributeHintAllowed(): Promise { if (gate.block) return false; return gate.config ? isContributeHintEnabled(gate.config.localConfig, gate.config.teamConfig) - : isContributeHintEnabled({}, {}); + : false; } /** What makes this skill unusable right now, or null. */ From d43c4e736c304f0d1884cca6acb000fe53ced52f Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 13:35:53 +0200 Subject: [PATCH 05/12] fix(contribute-check): gate the legacy reminder on the session's cwd `teamai contribute-check --stdin` asked the share gate about the directory the hook process started in, while the session analysis used the payload cwd. Started outside the project, it could read the user config and nudge where `teamai skill get share` refuses (a project config that does not load). It now moves to the payload cwd first, as hook-dispatch does. --- src/__tests__/contribute-check-e2e.test.ts | 18 ++++++++++++++++-- src/contribute-check.ts | 10 +++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/__tests__/contribute-check-e2e.test.ts b/src/__tests__/contribute-check-e2e.test.ts index c04bf6f8..0050b37b 100644 --- a/src/__tests__/contribute-check-e2e.test.ts +++ b/src/__tests__/contribute-check-e2e.test.ts @@ -32,11 +32,11 @@ function makeTmpHome(): string { } /** Build a hook STDIN JSON payload with a session_id. */ -function makeStdinPayload(sessionId: string): string { +function makeStdinPayload(sessionId: string, cwd = '/tmp/fake-project'): string { return JSON.stringify({ session_id: sessionId, hook_event_name: 'Stop', - cwd: '/tmp/fake-project', + cwd, }); } @@ -236,6 +236,20 @@ describe('contribute-check E2E', () => { expect(result.stdout).toBe(''); }); + it('asks the gate about the payload cwd, not the directory the hook process started in', async () => { + // The process starts in HOME, where the user config (recall on) would allow + // the nudge; the session ran in a project whose config does not parse, where + // `teamai skill get share` refuses. + const project = path.join(tmpHome, 'project'); + fs.mkdirSync(path.join(project, '.teamai'), { recursive: true }); + fs.writeFileSync(path.join(project, '.teamai', 'config.yaml'), 'repo: [unclosed\n'); + writeEventsFile(tmpHome, buildRichSessionEvents(SESSION_ID)); + + const result = await runContributeCheck(tmpHome, makeStdinPayload(SESSION_ID, project)); + expect(result.code).toBe(0); + expect(result.stdout).toBe(''); + }); + it('produces no output for a trivial session below threshold', async () => { writeEventsFile(tmpHome, buildTrivialSessionEvents(SESSION_ID)); diff --git a/src/contribute-check.ts b/src/contribute-check.ts index 5e3355c4..94fbd2de 100644 --- a/src/contribute-check.ts +++ b/src/contribute-check.ts @@ -711,7 +711,15 @@ export async function contributeCheck(toolArg?: string): Promise { } // The same gate as the dispatcher's handler: hooks written before it still - // call this command, and must not nudge towards a `share` that refuses. + // call this command, and must not nudge towards a `share` that refuses. The + // gate reads the cwd, so move to the session's, as hook-dispatch does. + if (stdinData.cwd) { + try { + process.chdir(stdinData.cwd); + } catch (e) { + log.debug(`contribute-check: chdir to ${stdinData.cwd} failed: ${e instanceof Error ? e.message : String(e)}`); + } + } const { contributeHintAllowed } = await import('./skill-content.js'); if (!(await contributeHintAllowed())) return; From aa3b0cd047637b448f51cb5968842a684272ab5a Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 13:36:34 +0200 Subject: [PATCH 06/12] fix(pull): keep a debug.log record when the stub cannot be deployed The previous commit turned both deploy catches into `log.warn`, which is muted in silent mode and never reaches debug.log, and a SessionStart pull runs detached with its output discarded. So the automatic pull, the one that deploys the stub for most members, lost the only persistent record it had. Both catches now warn and write the same line to debug.log. --- src/__tests__/pull-skip-sync.test.ts | 4 ++++ src/pull.ts | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/__tests__/pull-skip-sync.test.ts b/src/__tests__/pull-skip-sync.test.ts index eadbc4be..bceff37e 100644 --- a/src/__tests__/pull-skip-sync.test.ts +++ b/src/__tests__/pull-skip-sync.test.ts @@ -319,6 +319,8 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(log.success).toHaveBeenCalledWith(expect.stringContaining('Already synced at abc1234, skipping')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('EACCES: permission denied')); + // A SessionStart pull runs detached and silent; debug.log is its only record. + expect(log.debug).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed: EACCES: permission denied')); }); it('warns when the built-in stub cannot be deployed on a full sync', async () => { @@ -331,6 +333,8 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('EACCES: permission denied')); + // A SessionStart pull runs detached and silent; debug.log is its only record. + expect(log.debug).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed: EACCES: permission denied')); }); it('should do full sync when HEAD rev differs from lastPullRev', async () => { diff --git a/src/pull.ts b/src/pull.ts index 90af492a..378db2e2 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -730,6 +730,17 @@ async function reconcileEnvForUnchangedRepo( } } +/** + * The stub is the agent's only way into TeamAI, so a failure to deploy it is + * not silent. A SessionStart pull runs detached with its output discarded, and + * `log.warn` is muted there, so debug.log keeps the record. + */ +function warnStubNotDeployed(scopeLabel: string, e: unknown): void { + const message = `[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`; + log.warn(message); + log.debug(message); +} + async function pullForScope( localConfig: LocalConfig, options: GlobalOptions, @@ -1019,8 +1030,7 @@ async function pullForScope( const { deployBuiltinSkills } = await import('./builtin-skills.js'); await deployBuiltinSkills(freshConfig, localConfig); } catch (e) { - // The stub is the agent's only way into TeamAI, so this one is not silent. - log.warn(`[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`); + warnStubNotDeployed(scopeLabel, e); } // Refresh managed culture/shared-instruction blocks as well. A CLI // upgrade may add a new target file while the team repo SHA and tool @@ -1300,7 +1310,7 @@ async function pullForScope( log.debug(`[${scopeLabel}] Deployed ${deployed} built-in skill(s)`); } } catch (e) { - log.warn(`[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`); + warnStubNotDeployed(scopeLabel, e); } } From 5793758a9e3666dcedc5d348f3c9f286cd132c67 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 13:39:02 +0200 Subject: [PATCH 07/12] fix(skills): skill show and list never answer for the fallback team Known issues left by #747: - `skill show ` and `skill list` on a config that exists but does not load ended in a Node stack trace, and under a broken project config they searched the user config detection falls back to: another team's repo and agents. Both now ask `detectTeam`, the one place that tells "this team", "no team" and "cannot tell, and why" apart (`shareGate` is built on it). Without a usable team, `show` answers from the package alone and `list` prints only the packaged catalog; both say what failed on stderr and exit 1. - A teamai.yaml that exists but fails validation was reported as "not found. Check your repo path". It is now named as invalid, empty or unreadable, like the local config. --- docs/designs/skill-serving.md | 9 ++- src/__tests__/config-not-initialized.test.ts | 26 ++++++++ src/__tests__/e2e/skill-serving-cli.test.ts | 3 +- .../skill-list-uninitialized.test.ts | 32 ++++++++-- src/__tests__/skill-show.test.ts | 37 +++++++++-- src/config.ts | 30 ++++++--- src/skill-cmd.ts | 57 +++++++++-------- src/skill-content.ts | 61 ++++++++++++------- 8 files changed, 181 insertions(+), 74 deletions(-) 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 }, { isRecallEnabled }] = - await Promise.all([import('./config.js'), import('./types.js')]); +export type TeamDetection = + | { kind: 'team'; init: TeamaiInit } + | { kind: 'none' } + | { kind: 'unusable'; detail: string }; + +export async function detectTeam(): 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); - 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 }; + return { kind: 'unusable', detail: `${firstLine(unreadable)}. ${BROKEN_CONFIG_ADVICE}` }; } - loaded = await autoDetectInit(); + return { kind: 'team', init: 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 }; + if (e instanceof NotInitializedError) return { kind: 'none' }; + return { kind: 'unusable', detail: firstLine(e instanceof Error ? e.message : String(e)) }; } finally { setStderrOnly(previous); } - const { localConfig, teamConfig } = loaded; +} + +/** + * 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 + * on/off switch from the config returned instead of loading it a second time. + * + * Fails open only where there is no config at all: a fresh install reading + * 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`. + * Any failure past loading the config is a fault here and propagates. + */ +export async function shareGate(): Promise { + 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. */ From 79e4432bcfc9741782f14b52a13d3f537e244caf Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 14:03:59 +0200 Subject: [PATCH 08/12] fix(skills): the gate reads the session's directory, and no project config is skipped Codex review of 5793758: - A project-location config that is not `scope: project` (or omits `scope`, which defaults to user) was skipped without a word, so the gate read past it to the user config. It is now reported as unusable, unless it is the user config itself, as when running from HOME. - The legacy `contribute-check` changed into the payload cwd and, if that failed, asked the gate about the directory the process started in. It now passes the payload cwd to the gate (`detectTeam(cwd)`), and a cwd that no longer exists holds no project config, so only the user config is asked, as #753 does. --- docs/designs/skill-serving.md | 2 +- src/__tests__/config-not-initialized.test.ts | 24 +++++++++++++++++ src/__tests__/contribute-check-e2e.test.ts | 19 +++++++++++-- .../skill-list-uninitialized.test.ts | 1 + src/config.ts | 27 ++++++++++++++++--- src/contribute-check.ts | 13 +++------ src/skill-content.ts | 23 +++++++++------- 7 files changed, 84 insertions(+), 25 deletions(-) diff --git a/docs/designs/skill-serving.md b/docs/designs/skill-serving.md index 1cb239ab..a20dd5f4 100644 --- a/docs/designs/skill-serving.md +++ b/docs/designs/skill-serving.md @@ -94,7 +94,7 @@ path (measured here from a 77-character one). blocks instead (`blockedBy: "config"`), since recall and the source are then unknown and the workflow would fail at `teamai contribute` — a project config too, which detection alone would skip in favour of the user config - (`findUnreadableProjectConfig`). The refusal then says what failed (for a file + (`findUnreadableProjectConfig`), including one that is not `scope: project`. The refusal then says what failed (for a file that does not parse, which file and where; for one that fails validation, which field and why), since nothing else reports it. The Stop-hook share reminder asks the same gate (`contributeHintAllowed`, called diff --git a/src/__tests__/config-not-initialized.test.ts b/src/__tests__/config-not-initialized.test.ts index 645dfc8d..864d6803 100644 --- a/src/__tests__/config-not-initialized.test.ts +++ b/src/__tests__/config-not-initialized.test.ts @@ -150,6 +150,30 @@ describe('findUnreadableProjectConfig', () => { expect(problem).not.toContain('\n'); }); + it('names a project config that is not scope: project, which detection alone skips', async () => { + // `scope` omitted defaults to user: detection would read past the file to + // the user config, another team's. + const configPath = path.join(dir, '.teamai', 'config.yaml'); + fs.mkdirSync(path.dirname(configPath), { recursive: true }); + fs.writeFileSync(configPath, 'repo:\n localPath: /x\n remote: https://example.test/a.git\nusername: t\n'); + + const problem = await findUnreadableProjectConfig(dir); + expect(problem).toContain(configPath); + expect(problem).toContain('scope: project'); + }); + + it('does not name the user config when the directory is HOME itself', async () => { + // Run from HOME, `/.teamai/config.yaml` is the user config, not a project's. + vi.stubEnv('HOME', dir); + vi.stubEnv('USERPROFILE', dir); + const configPath = path.join(dir, '.teamai', 'config.yaml'); + fs.mkdirSync(path.dirname(configPath), { recursive: true }); + fs.writeFileSync(configPath, 'repo:\n localPath: /x\n remote: https://example.test/a.git\nusername: t\nscope: user\n'); + + expect(await findUnreadableProjectConfig(dir)).toBeNull(); + vi.unstubAllEnvs(); + }); + it('names a broken partition config even when the legacy .teamai/ config behind it loads', async () => { // The partition is authoritative; detection skips it when broken and lands // on the legacy config, which may belong to another team. diff --git a/src/__tests__/contribute-check-e2e.test.ts b/src/__tests__/contribute-check-e2e.test.ts index 0050b37b..3eb51c89 100644 --- a/src/__tests__/contribute-check-e2e.test.ts +++ b/src/__tests__/contribute-check-e2e.test.ts @@ -67,14 +67,15 @@ function runContributeCheck( homeDir: string, stdinPayload: string, tool = 'claude', + processCwd = homeDir, ): Promise<{ stdout: string; stderr: string; code: number }> { return new Promise((resolve) => { const child = execFile( 'node', [CLI_PATH, 'contribute-check', '--stdin', '--tool', tool], { - // cwd too: the share gate reads a project config under it. - cwd: homeDir, + // The directory the hook process starts in, which the gate must not read. + cwd: processCwd, env: { ...process.env, HOME: homeDir, TEAMAI_LOG_LEVEL: 'silent' }, timeout: 10000, }, @@ -250,6 +251,20 @@ describe('contribute-check E2E', () => { expect(result.stdout).toBe(''); }); + it('never asks the gate about the launcher directory, even when the payload cwd no longer exists', async () => { + // The hook process starts in a project whose config does not parse; the + // session ran in a worktree since deleted, which holds no project config, + // so the user config (recall on) decides. + const launcher = path.join(tmpHome, 'launcher'); + fs.mkdirSync(path.join(launcher, '.teamai'), { recursive: true }); + fs.writeFileSync(path.join(launcher, '.teamai', 'config.yaml'), 'repo: [unclosed\n'); + writeEventsFile(tmpHome, buildRichSessionEvents(SESSION_ID)); + + const result = await runContributeCheck(tmpHome, makeStdinPayload(SESSION_ID, path.join(tmpHome, 'deleted-worktree')), 'claude', launcher); + expect(result.code).toBe(0); + expect(result.stdout).not.toBe(''); + }); + it('produces no output for a trivial session below threshold', async () => { writeEventsFile(tmpHome, buildTrivialSessionEvents(SESSION_ID)); diff --git a/src/__tests__/skill-list-uninitialized.test.ts b/src/__tests__/skill-list-uninitialized.test.ts index 37ccfa7f..08bc2d4e 100644 --- a/src/__tests__/skill-list-uninitialized.test.ts +++ b/src/__tests__/skill-list-uninitialized.test.ts @@ -10,6 +10,7 @@ const { autoDetectInit, findUnreadableProjectConfig, logDim, logError, NotInitia vi.mock('../config.js', () => ({ autoDetectInit, findUnreadableProjectConfig, + requireInit: vi.fn(), NotInitializedError, BROKEN_CONFIG_ADVICE: 'Fix the file, or move it aside and run `teamai init` to write a new one.', })); diff --git a/src/config.ts b/src/config.ts index 0ae44634..ed8f075d 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,5 +1,6 @@ import YAML from 'yaml'; import { ZodError } from 'zod'; +import fs from 'node:fs'; import path from 'node:path'; import { TeamaiConfigSchema, @@ -432,7 +433,15 @@ export async function readConfigFrom( try { const raw = YAML.parse(content); const config = LocalConfigSchema.parse(raw); - if (config.scope !== 'project') return null; + if (config.scope !== 'project') { + // Run from HOME, `/.teamai/config.yaml` is the user config itself. + // Anywhere else a config here that is not scope: project cannot say which + // project it serves, and detection would read past it to the user config. + if (!isUserConfigFile(configPath)) { + onUnreadable?.(configPath, `it is scope: ${config.scope}, but a config inside a project must be scope: project`); + } + return null; + } // Anchor projectRoot to the workspace root (resource landing) and dataHome to // the directory this config lives in (machine-data location). A persisted // projectRoot can be wrong (e.g. a `.teamai/` copied from the main checkout @@ -462,6 +471,18 @@ export async function readConfigFrom( } } +/** Whether `configPath` is the user config, comparing real paths (a tmp HOME and the cwd can differ by a symlink). */ +function isUserConfigFile(configPath: string): boolean { + const real = (p: string): string => { + try { + return fs.realpathSync(p); + } catch { + return path.resolve(p); + } + }; + return real(configPath) === real(expandHome(getUserConfigPath())); +} + /** * One line naming what is wrong. A Zod message is a JSON dump of its issues, * whose first line is `[`; each issue's field and reason is what a member fixes. @@ -513,8 +534,8 @@ export async function requireInitForScope( * If cwd has a project-scope config, uses that; otherwise falls back to user scope. * This is the recommended entry point for commands that support both scopes. */ -export async function autoDetectInit(): Promise { - const projectConfig = await detectProjectConfig(); +export async function autoDetectInit(cwd?: string): Promise { + const projectConfig = await detectProjectConfig(cwd); if (projectConfig) { const teamConfig = await loadTeamConfig(projectConfig.repo.localPath); if (!teamConfig) return throwTeamConfigMissingOrInvalid(projectConfig.repo.localPath); diff --git a/src/contribute-check.ts b/src/contribute-check.ts index 94fbd2de..7de03229 100644 --- a/src/contribute-check.ts +++ b/src/contribute-check.ts @@ -711,17 +711,10 @@ export async function contributeCheck(toolArg?: string): Promise { } // The same gate as the dispatcher's handler: hooks written before it still - // call this command, and must not nudge towards a `share` that refuses. The - // gate reads the cwd, so move to the session's, as hook-dispatch does. - if (stdinData.cwd) { - try { - process.chdir(stdinData.cwd); - } catch (e) { - log.debug(`contribute-check: chdir to ${stdinData.cwd} failed: ${e instanceof Error ? e.message : String(e)}`); - } - } + // call this command, and must not nudge towards a `share` that refuses. It + // is asked about the session's cwd, never the one this process started in. const { contributeHintAllowed } = await import('./skill-content.js'); - if (!(await contributeHintAllowed())) return; + if (!(await contributeHintAllowed(stdinData.cwd))) return; const { stopStdoutUnsupported } = await import('./utils/tool-names.js'); const tool = toolArg?.toLowerCase() ?? 'claude'; diff --git a/src/skill-content.ts b/src/skill-content.ts index 94fea847..9542885d 100644 --- a/src/skill-content.ts +++ b/src/skill-content.ts @@ -97,21 +97,24 @@ export type TeamDetection = | { kind: 'none' } | { kind: 'unusable'; detail: string }; -export async function detectTeam(): Promise { - const { autoDetectInit, findUnreadableProjectConfig, NotInitializedError, BROKEN_CONFIG_ADVICE } = +export async function detectTeam(cwd?: string): Promise { + const { autoDetectInit, findUnreadableProjectConfig, requireInit, 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(); + // A directory that no longer exists (a hook payload naming a deleted + // worktree) holds no project config, and git refuses to open it. + if (cwd !== undefined && !(await pathExists(cwd))) return { kind: 'team', init: await requireInit() }; + const unreadable = await findUnreadableProjectConfig(cwd); 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() }; + return { kind: 'team', init: await autoDetectInit(cwd) }; } catch (e) { if (e instanceof NotInitializedError) return { kind: 'none' }; return { kind: 'unusable', detail: firstLine(e instanceof Error ? e.message : String(e)) }; @@ -131,9 +134,9 @@ export async function detectTeam(): Promise { * writable, is then unknown, and the workflow would fail at `teamai contribute`. * Any failure past loading the config is a fault here and propagates. */ -export async function shareGate(): Promise { +export async function shareGate(cwd?: string): Promise { const { isRecallEnabled } = await import('./types.js'); - const team = await detectTeam(); + const team = await detectTeam(cwd); 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; @@ -158,13 +161,15 @@ function firstLine(text: string): string { * * The reminder routes to `share`, so it is withheld wherever `shareGate` * blocks it: a nudge there would send the agent to a command that says no. - * Both the hook dispatcher and the legacy `teamai contribute-check` ask this. + * Both the hook dispatcher and the legacy `teamai contribute-check` ask this: + * the dispatcher has changed into the session's cwd already, the legacy + * command passes it, since its process may start anywhere. */ -export async function contributeHintAllowed(): Promise { +export async function contributeHintAllowed(cwd?: string): Promise { const { isContributeHintEnabled } = await import('./types.js'); let gate: ShareGate; try { - gate = await shareGate(); + gate = await shareGate(cwd); } catch (e) { // A fault in the gate itself, not a config it could not load (the gate // answers that): a Stop hook must not fail the turn over a reminder. From b0583f56814a3bbfb5875c517ef2836a76750d80 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 14:03:59 +0200 Subject: [PATCH 09/12] fix(logger): record warnings in debug.log `log.warn` wrote to the console only and was muted in silent mode, so a detached SessionStart pull, whose output is discarded, lost every warning: the stub deploy failure and the legacy prune among them. Warnings now reach debug.log like debug and error lines. `warnStubNotDeployed` drops the second `log.debug` call, which printed the line twice under --verbose. --- src/__tests__/logger.test.ts | 8 ++++++++ src/__tests__/pull-skip-sync.test.ts | 8 ++++---- src/pull.ts | 8 +++----- src/utils/logger.ts | 3 ++- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/__tests__/logger.test.ts b/src/__tests__/logger.test.ts index ed57c00c..5c92d1e6 100644 --- a/src/__tests__/logger.test.ts +++ b/src/__tests__/logger.test.ts @@ -45,6 +45,14 @@ describe('file transport', () => { expect(fs.readFileSync(logFile, 'utf-8')).toContain('[ERROR] something broke'); }); + it('writes warn to file, even when silent', () => { + // A detached SessionStart pull runs silent with its output discarded; its + // warnings must still leave a record. + setSilent(true); + log.warn('stub not deployed'); + expect(fs.readFileSync(logFile, 'utf-8')).toContain('[WARN] stub not deployed'); + }); + it('includes timestamp', () => { log.debug('ts'); expect(fs.readFileSync(logFile, 'utf-8').trim()).toMatch(/^\d{4}-\d{2}-\d{2}T/); diff --git a/src/__tests__/pull-skip-sync.test.ts b/src/__tests__/pull-skip-sync.test.ts index bceff37e..ab3397bc 100644 --- a/src/__tests__/pull-skip-sync.test.ts +++ b/src/__tests__/pull-skip-sync.test.ts @@ -319,8 +319,8 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(log.success).toHaveBeenCalledWith(expect.stringContaining('Already synced at abc1234, skipping')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('EACCES: permission denied')); - // A SessionStart pull runs detached and silent; debug.log is its only record. - expect(log.debug).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed: EACCES: permission denied')); + // `log.warn` also records to debug.log; a second `log.debug` would print it twice under --verbose. + expect(log.debug).not.toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); }); it('warns when the built-in stub cannot be deployed on a full sync', async () => { @@ -333,8 +333,8 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('EACCES: permission denied')); - // A SessionStart pull runs detached and silent; debug.log is its only record. - expect(log.debug).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed: EACCES: permission denied')); + // `log.warn` also records to debug.log; a second `log.debug` would print it twice under --verbose. + expect(log.debug).not.toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); }); it('should do full sync when HEAD rev differs from lastPullRev', async () => { diff --git a/src/pull.ts b/src/pull.ts index 378db2e2..11da3ae5 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -732,13 +732,11 @@ async function reconcileEnvForUnchangedRepo( /** * The stub is the agent's only way into TeamAI, so a failure to deploy it is - * not silent. A SessionStart pull runs detached with its output discarded, and - * `log.warn` is muted there, so debug.log keeps the record. + * not silent. A SessionStart pull runs detached with its output discarded; + * `log.warn` also records to debug.log, which keeps the trace there. */ function warnStubNotDeployed(scopeLabel: string, e: unknown): void { - const message = `[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`; - log.warn(message); - log.debug(message); + log.warn(`[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`); } async function pullForScope( diff --git a/src/utils/logger.ts b/src/utils/logger.ts index aba89b09..1455478f 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -10,7 +10,7 @@ let stderrMode = false; // ─── File transport ───────────────────────────────────── // -// All log.debug() and log.error() calls are persisted to +// All log.debug(), log.warn() and log.error() calls are persisted to // ~/.teamai/debug.log via synchronous append. This ensures // hook processes (short-lived, stdout swallowed by Claude Code) // leave a durable trace for troubleshooting. @@ -143,6 +143,7 @@ export const log = { writeInfoLine(`${chalk.green('✔')} ${msg}`); }, warn(msg: string): void { + writeToFile('WARN', msg); if (silentMode) return; writeInfoLine(`${chalk.yellow('⚠')} ${msg}`); }, From 15a5b5d54595d569260c5debc3570603c74a4022 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 14:21:44 +0200 Subject: [PATCH 10/12] fix(skills): the dispatcher gate reads the payload cwd; a symlink is not HOME Codex review of b0583f5: - The dispatcher's `contribute-check` and `pending-hint` handlers asked the gate about the process's directory, trusting hook-dispatch's `chdir`; when that failed, the launcher's config decided. They now pass `resolveHookCwd(stdin)`, as the legacy command does. - The HOME exception for a non-project scope compared the config file's real path, so a project config symlinked to ~/.teamai/config.yaml passed for the user config. It is now decided by the project's location: its root is HOME. --- src/__tests__/config-not-initialized.test.ts | 20 ++++++++++++++++ src/__tests__/hook-handlers.test.ts | 25 +++++++++++++++++++- src/config.ts | 15 ++++++++---- src/hook-handlers.ts | 9 +++---- src/skill-content.ts | 6 ++--- 5 files changed, 63 insertions(+), 12 deletions(-) diff --git a/src/__tests__/config-not-initialized.test.ts b/src/__tests__/config-not-initialized.test.ts index 864d6803..e23eb4c1 100644 --- a/src/__tests__/config-not-initialized.test.ts +++ b/src/__tests__/config-not-initialized.test.ts @@ -174,6 +174,26 @@ describe('findUnreadableProjectConfig', () => { vi.unstubAllEnvs(); }); + it('names a project config that is a symlink to the user config', async () => { + // The HOME exception is about where the project is, not where the file points. + const home = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-symlink-home-')); + vi.stubEnv('HOME', home); + vi.stubEnv('USERPROFILE', home); + const userConfig = path.join(home, '.teamai', 'config.yaml'); + fs.mkdirSync(path.dirname(userConfig), { recursive: true }); + fs.writeFileSync(userConfig, 'repo:\n localPath: /x\n remote: https://example.test/a.git\nusername: t\nscope: user\n'); + const configPath = path.join(dir, '.teamai', 'config.yaml'); + fs.mkdirSync(path.dirname(configPath), { recursive: true }); + fs.symlinkSync(userConfig, configPath); + + try { + expect(await findUnreadableProjectConfig(dir)).toContain(configPath); + } finally { + vi.unstubAllEnvs(); + fs.rmSync(home, { recursive: true, force: true }); + } + }); + it('names a broken partition config even when the legacy .teamai/ config behind it loads', async () => { // The partition is authoritative; detection skips it when broken and lands // on the legacy config, which may belong to another team. diff --git a/src/__tests__/hook-handlers.test.ts b/src/__tests__/hook-handlers.test.ts index 05194204..49cf98c0 100644 --- a/src/__tests__/hook-handlers.test.ts +++ b/src/__tests__/hook-handlers.test.ts @@ -87,6 +87,9 @@ const mockFindUnreadableProjectConfig = vi.fn().mockResolvedValue(null); vi.mock('../config.js', async (importOriginal) => ({ ...(await importOriginal()), autoDetectInit: mockAutoDetectInit, + // A payload cwd that no longer exists (these tests use '/x') holds no + // project config, so the gate asks the user config: the same mocked one. + requireInit: mockAutoDetectInit, findUnreadableProjectConfig: mockFindUnreadableProjectConfig, })); @@ -440,11 +443,31 @@ describe('hook-handlers registry', () => { mockFindUnreadableProjectConfig.mockResolvedValueOnce('/x/.teamai/config.yaml: bad indentation'); mockContributeCheckForSession.mockClear(); - const result = await handler.execute({ session_id: 's5c', cwd: '/x' }, 'claude'); + // An existing directory: a deleted one holds no project config to be unreadable. + const result = await handler.execute({ session_id: 's5c', cwd: process.cwd() }, 'claude'); expect(result).toBeNull(); expect(mockContributeCheckForSession).not.toHaveBeenCalled(); }); + it('contribute-check handler asks the gate about the payload cwd, not the directory the process is in', async () => { + // hook-dispatch changes into the payload cwd, but that can fail; the gate + // must not then read wherever the process started. + const registry = buildHandlerRegistry(); + const handler = registry.find( + (r) => r.event === 'stop' && r.handler.name === 'contribute-check', + )!.handler; + mockFindUnreadableProjectConfig.mockImplementation(async (cwd?: string) => + cwd === undefined ? '/launcher/.teamai/config.yaml: bad indentation' : null); + mockContributeCheckForSession.mockResolvedValueOnce({ hint: '[teamai] do share' }); + try { + const result = await handler.execute({ session_id: 's5e', cwd: process.cwd() }, 'claude'); + expect(result).toContain('do share'); + } finally { + mockFindUnreadableProjectConfig.mockReset(); + mockFindUnreadableProjectConfig.mockResolvedValue(null); + } + }); + it('contribute-check handler withholds the reminder, without failing the turn, when the gate itself faults', async () => { const registry = buildHandlerRegistry(); const handler = registry.find( diff --git a/src/config.ts b/src/config.ts index ed8f075d..5fb25ba7 100644 --- a/src/config.ts +++ b/src/config.ts @@ -19,6 +19,7 @@ import { } from './types.js'; import { readFileSafe, readJson, writeFile, writeJson, expandHome, pathExists } from './utils/fs.js'; import { resolveAnchors } from './utils/git.js'; +import { getUserHome } from './utils/home.js'; import { resolvePartitionDir, writeAnchorFile } from './utils/partition.js'; import { log } from './utils/logger.js'; import { loadRolesManifest } from './roles.js'; @@ -437,7 +438,7 @@ export async function readConfigFrom( // Run from HOME, `/.teamai/config.yaml` is the user config itself. // Anywhere else a config here that is not scope: project cannot say which // project it serves, and detection would read past it to the user config. - if (!isUserConfigFile(configPath)) { + if (!isUserTeamaiDir(dataHomeDir, projectRoot)) { onUnreadable?.(configPath, `it is scope: ${config.scope}, but a config inside a project must be scope: project`); } return null; @@ -471,8 +472,13 @@ export async function readConfigFrom( } } -/** Whether `configPath` is the user config, comparing real paths (a tmp HOME and the cwd can differ by a symlink). */ -function isUserConfigFile(configPath: string): boolean { +/** + * Whether `dataHomeDir` is `/.teamai`, decided by where the project is: + * its root is HOME. Real paths, since a tmp HOME and the cwd can differ by a + * symlink; but never the file's target, or a project config symlinked to the + * user config would pass for it. + */ +function isUserTeamaiDir(dataHomeDir: string, projectRoot: string): boolean { const real = (p: string): string => { try { return fs.realpathSync(p); @@ -480,7 +486,8 @@ function isUserConfigFile(configPath: string): boolean { return path.resolve(p); } }; - return real(configPath) === real(expandHome(getUserConfigPath())); + return path.resolve(dataHomeDir) === path.join(path.resolve(projectRoot), '.teamai') + && real(projectRoot) === real(getUserHome()); } /** diff --git a/src/hook-handlers.ts b/src/hook-handlers.ts index 52657d09..1ef03bb9 100644 --- a/src/hook-handlers.ts +++ b/src/hook-handlers.ts @@ -129,8 +129,7 @@ const updateHandler: HookHandler = { /** * Team course-correction keywords for the current project. The dispatcher has * already chdir'd to the hook payload's cwd (hook-dispatch-cli), so - * autoDetectInit() resolves the right project, as it does for - * contributeHintAllowed. Only prompt hooks pay for the config read; an + * autoDetectInit() resolves the right project. Only prompt hooks pay for the config read; an * unreadable config means "built-in keywords only". */ async function teamCorrectionKeywords(stdin: Record): Promise { @@ -250,8 +249,10 @@ export function buildVotesNudge(recalledDocIds: readonly string[]): string { const contributeCheckHandler: HookHandler = { name: 'contribute-check', async execute(stdin, tool) { + // The payload's cwd, not the process's: hook-dispatch changes into it, but + // that can fail, and the gate must not then read the launcher's directory. const { contributeHintAllowed } = await import('./skill-content.js'); - if (!(await contributeHintAllowed())) return null; + if (!(await contributeHintAllowed(resolveHookCwd(stdin)))) return null; const { contributeCheckForSession } = await import('./contribute-check.js'); const { formatStopHookOutput, relayWhenHidden } = await import('./utils/hook-output.js'); @@ -294,7 +295,7 @@ const pendingHintHandler: HookHandler = { // feature off is not delivered later when it is turned back on. const stashed = await pending.takePendingHint(sessionId); const { contributeHintAllowed } = await import('./skill-content.js'); - const hint = (await contributeHintAllowed()) ? stashed : null; + const hint = (await contributeHintAllowed(resolveHookCwd(stdin))) ? stashed : null; const votesHint = await pending.takePendingVotesHint(sessionId); // The votes nudge instructs the model; the contribute hint asks it to relay diff --git a/src/skill-content.ts b/src/skill-content.ts index 9542885d..bc17fe20 100644 --- a/src/skill-content.ts +++ b/src/skill-content.ts @@ -161,9 +161,9 @@ function firstLine(text: string): string { * * The reminder routes to `share`, so it is withheld wherever `shareGate` * blocks it: a nudge there would send the agent to a command that says no. - * Both the hook dispatcher and the legacy `teamai contribute-check` ask this: - * the dispatcher has changed into the session's cwd already, the legacy - * command passes it, since its process may start anywhere. + * Both the hook dispatcher and the legacy `teamai contribute-check` ask this, + * passing the session's cwd: the process may run anywhere, and a `chdir` into + * the cwd can fail. */ export async function contributeHintAllowed(cwd?: string): Promise { const { isContributeHintEnabled } = await import('./types.js'); From aaaf0ac172f4a7d585339bba15c5bcaa107c2730 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 14:47:59 +0200 Subject: [PATCH 11/12] fix(skills): only a missing cwd falls back to the user config; load it once Codex review of 15a5b5d: - `detectTeam` read any failure to see the payload cwd as "deleted", so a cwd it could not open (no permission, a path through a file) fell back to the user config and could allow the reminder. Only ENOENT does now; anything else is `unusable` and withholds it. - `skill show share` and `skill list` loaded the config twice, through the gate and then the team lookup, and reported a broken one twice. Both detect the team once and hand it to the gate. --- src/__tests__/hook-handlers.test.ts | 23 ++++++++++++ .../skill-list-uninitialized.test.ts | 8 ++++ src/__tests__/skill-show.test.ts | 10 ++++- src/skill-cmd.ts | 10 +++-- src/skill-content.ts | 37 ++++++++++++++----- 5 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/__tests__/hook-handlers.test.ts b/src/__tests__/hook-handlers.test.ts index f42e2c2e..e2acb0d4 100644 --- a/src/__tests__/hook-handlers.test.ts +++ b/src/__tests__/hook-handlers.test.ts @@ -1,4 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; // ── Mocks ──────────────────────────────────────────────── // Mock the underlying modules so handlers don't do real I/O @@ -472,6 +475,26 @@ describe('hook-handlers registry', () => { } }); + it('contribute-check handler withholds the reminder when the payload cwd exists but cannot be checked', async () => { + // Only a cwd that is gone (ENOENT) falls back to the user config; one that + // cannot be opened, here a path through a file (ENOTDIR), is unknown. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-hook-cwd-')); + const file = path.join(dir, 'a-file'); + fs.writeFileSync(file, ''); + const registry = buildHandlerRegistry(); + const handler = registry.find( + (r) => r.event === 'stop' && r.handler.name === 'contribute-check', + )!.handler; + mockContributeCheckForSession.mockClear(); + try { + const result = await handler.execute({ session_id: 's5f', cwd: path.join(file, 'sub') }, 'claude'); + expect(result).toBeNull(); + expect(mockContributeCheckForSession).not.toHaveBeenCalled(); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + it('contribute-check handler withholds the reminder, without failing the turn, when the gate itself faults', async () => { const registry = buildHandlerRegistry(); const handler = registry.find( diff --git a/src/__tests__/skill-list-uninitialized.test.ts b/src/__tests__/skill-list-uninitialized.test.ts index 08bc2d4e..a68ab4e8 100644 --- a/src/__tests__/skill-list-uninitialized.test.ts +++ b/src/__tests__/skill-list-uninitialized.test.ts @@ -75,6 +75,14 @@ describe('teamai skill list before init', () => { expect(stdout).toContain('teamai skill get core'); }); + it('loads the config once, so a broken one is reported once', async () => { + autoDetectInit.mockRejectedValue(new Error('The teamai config at /h/.teamai/config.yaml could not be read: it is empty.')); + + await skillList({}); + + expect(autoDetectInit).toHaveBeenCalledTimes(1); + }); + 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. diff --git a/src/__tests__/skill-show.test.ts b/src/__tests__/skill-show.test.ts index 16d2dd9d..0263114a 100644 --- a/src/__tests__/skill-show.test.ts +++ b/src/__tests__/skill-show.test.ts @@ -89,11 +89,12 @@ function captureLogs() { async function runSkillShow( name: string, fx: Fixture, - config: { unreadableProjectConfig?: string; loadError?: Error } = {}, + config: { unreadableProjectConfig?: string; loadError?: Error; loads?: { count: number } } = {}, ): Promise { vi.doMock('../config.js', async (importOriginal) => ({ ...(await importOriginal()), autoDetectInit: async () => { + if (config.loads) config.loads.count += 1; if (config.loadError) throw config.loadError; return { localConfig: fx.localConfig, teamConfig: fx.teamConfig }; }, @@ -253,6 +254,13 @@ describe('skillShow locator', () => { process.exitCode = 0; }); + it('loads the config once for share, so a broken one is reported once', async () => { + fx.localConfig.recallEnabled = true; + const loads = { count: 0 }; + await runSkillShow('share', fx, { loads }); + expect(loads.count).toBe(1); + }); + it('shows share once recall is enabled', async () => { fx.localConfig.recallEnabled = true; const lines = await runSkillShow('share', fx); diff --git a/src/skill-cmd.ts b/src/skill-cmd.ts index 385439db..e8c69ace 100644 --- a/src/skill-cmd.ts +++ b/src/skill-cmd.ts @@ -49,8 +49,10 @@ type LocatedSkill = ResolvedSkill | BlockedSkill; * we print under "Repo path" or "Installed in". */ export async function skillShow(name: string, options: GlobalOptions): Promise { - const served = await resolveServableSkill(name); + // One config load for both the gate and the lookup, so a broken config is + // reported once. const team = await detectTeam(); + const served = await resolveServableSkill(name, undefined, team); 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 @@ -144,7 +146,10 @@ export async function skillShow(name: string, options: GlobalOptions): Promise { - const catalog = await skillCatalog(); + // One config load for the catalog's gate and the team listing, so a broken + // config is reported once. + const team = await detectTeam(); + const catalog = await skillCatalog(undefined, team); if (options.json) { console.log(JSON.stringify({ skills: catalog }, null, 2)); @@ -155,7 +160,6 @@ export async function skillList(options: GlobalOptions & { json?: boolean }): Pr // still gets to discover what the installed CLI serves, like `skill get` does. // A config that cannot be loaded lists no team either, rather than the one // detection would fall back to, and says what failed. - const team = await detectTeam(); if (team.kind === 'team') { const { list } = await import('./status.js'); await list('skills', { ...options, source: 'all' }); diff --git a/src/skill-content.ts b/src/skill-content.ts index bc17fe20..31e4c242 100644 --- a/src/skill-content.ts +++ b/src/skill-content.ts @@ -105,9 +105,20 @@ export async function detectTeam(cwd?: string): Promise { // stdout, or a hook's reply, so config loading reports on stderr here. const previous = setStderrOnly(true); try { - // A directory that no longer exists (a hook payload naming a deleted - // worktree) holds no project config, and git refuses to open it. - if (cwd !== undefined && !(await pathExists(cwd))) return { kind: 'team', init: await requireInit() }; + if (cwd !== undefined) { + try { + await fs.promises.stat(cwd); + } catch (e) { + // A directory that no longer exists (a hook payload naming a deleted + // worktree) holds no project config, and git refuses to open it. Any + // other failure (no permission, a path through a file) leaves the + // project unknown, not absent. + if (typeof e === 'object' && e !== null && 'code' in e && e.code === 'ENOENT') { + return { kind: 'team', init: await requireInit() }; + } + return { kind: 'unusable', detail: `${cwd} cannot be checked: ${e instanceof Error ? e.message : String(e)}` }; + } + } const unreadable = await findUnreadableProjectConfig(cwd); if (unreadable) { // A parse error spans several lines (a code frame); its first names the @@ -135,8 +146,12 @@ export async function detectTeam(cwd?: string): Promise { * Any failure past loading the config is a fault here and propagates. */ export async function shareGate(cwd?: string): Promise { + return gateFor(await detectTeam(cwd)); +} + +/** The share gate on a team already detected, for a command that needs the team too. */ +async function gateFor(team: TeamDetection): Promise { const { isRecallEnabled } = await import('./types.js'); - const team = await detectTeam(cwd); 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; @@ -183,9 +198,9 @@ export async function contributeHintAllowed(cwd?: string): Promise { } /** What makes this skill unusable right now, or null. */ -async function blockReason(name: string): Promise { +async function blockReason(name: string, team?: TeamDetection): Promise { if (!RECALL_DEPENDENT_SKILLS.has(name)) return null; - return (await shareGate()).block; + return (team ? await gateFor(team) : await shareGate()).block; } /** A skill directory that ships inside the npm package. */ @@ -296,10 +311,11 @@ export type BlockedSkill = { kind: 'blocked'; name: string } & SkillBlock; export async function resolveServableSkill( name: string, roots: PackagedSkillRoots = packagedSkillRoots(), + team?: TeamDetection, ): Promise { const skill = await resolvePackagedSkill(name, roots); if (!skill) return { kind: 'not-found', name }; - const block = await blockReason(skill.name); + const block = await blockReason(skill.name, team); if (block) return { kind: 'blocked', name: skill.name, ...block }; return { kind: 'found', skill }; } @@ -539,11 +555,14 @@ export type SkillCatalogEntry = | (SkillCatalogEntryFields & { blockedBy: null; path: string }) | (SkillCatalogEntryFields & { blockedBy: SkillBlockReason; path: null }); -export async function skillCatalog(roots: PackagedSkillRoots = packagedSkillRoots()): Promise { +export async function skillCatalog( + roots: PackagedSkillRoots = packagedSkillRoots(), + team?: TeamDetection, +): Promise { const skills = await listServableSkills(roots); const entries: SkillCatalogEntry[] = []; for (const skill of skills) { - const resolved = await resolveServableSkill(skill.name, roots); + const resolved = await resolveServableSkill(skill.name, roots, team); const fields: SkillCatalogEntryFields = { name: skill.name, description: await readSkillDescription(path.join(skill.dir, SKILL_MD)), From 84a461d3f5763b96a5652d2454ee7385191cdf62 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 14:47:59 +0200 Subject: [PATCH 12/12] fix(logger): a file-only record instead of persisting every warning b0583f5 made every `log.warn` append to debug.log, wider than the two failures it was for, and it wrote unrelated subprocess errors to disk. `log.warn` is console-only again; `log.persist` writes one line to debug.log and never to the console. The stub deploy catches and the legacy prune catch use both, so a detached SessionStart pull keeps the record and --verbose prints it once. --- src/__tests__/logger.test.ts | 22 ++++++++++++++++++---- src/__tests__/pull-skip-sync.test.ts | 9 +++++---- src/builtin-skills.ts | 5 ++++- src/pull.ts | 8 +++++--- src/utils/logger.ts | 11 +++++++++-- 5 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/__tests__/logger.test.ts b/src/__tests__/logger.test.ts index 5c92d1e6..7884d038 100644 --- a/src/__tests__/logger.test.ts +++ b/src/__tests__/logger.test.ts @@ -45,14 +45,28 @@ describe('file transport', () => { expect(fs.readFileSync(logFile, 'utf-8')).toContain('[ERROR] something broke'); }); - it('writes warn to file, even when silent', () => { - // A detached SessionStart pull runs silent with its output discarded; its - // warnings must still leave a record. + it('persists to file only, never the console, even when silent', () => { + // A detached SessionStart pull runs silent with its output discarded; the + // failures it cannot show still leave a record, printed nowhere twice. + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + setVerbose(true); setSilent(true); - log.warn('stub not deployed'); + log.persist('stub not deployed'); + expect(logSpy).not.toHaveBeenCalled(); + expect(errorSpy).not.toHaveBeenCalled(); + logSpy.mockRestore(); + errorSpy.mockRestore(); expect(fs.readFileSync(logFile, 'utf-8')).toContain('[WARN] stub not deployed'); }); + it('keeps warn on the console only', () => { + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + log.warn('shown, not stored'); + logSpy.mockRestore(); + expect(fs.existsSync(logFile) ? fs.readFileSync(logFile, 'utf-8') : '').not.toContain('shown, not stored'); + }); + it('includes timestamp', () => { log.debug('ts'); expect(fs.readFileSync(logFile, 'utf-8').trim()).toMatch(/^\d{4}-\d{2}-\d{2}T/); diff --git a/src/__tests__/pull-skip-sync.test.ts b/src/__tests__/pull-skip-sync.test.ts index ab3397bc..f2ae089d 100644 --- a/src/__tests__/pull-skip-sync.test.ts +++ b/src/__tests__/pull-skip-sync.test.ts @@ -24,6 +24,7 @@ vi.mock('../utils/git.js', () => ({ vi.mock('../utils/logger.js', () => ({ log: { + persist: vi.fn(), info: vi.fn(), success: vi.fn(), warn: vi.fn(), @@ -319,8 +320,8 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(log.success).toHaveBeenCalledWith(expect.stringContaining('Already synced at abc1234, skipping')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('EACCES: permission denied')); - // `log.warn` also records to debug.log; a second `log.debug` would print it twice under --verbose. - expect(log.debug).not.toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); + // A detached SessionStart pull discards its output: debug.log keeps the record. + expect(log.persist).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed: EACCES')); }); it('warns when the built-in stub cannot be deployed on a full sync', async () => { @@ -333,8 +334,8 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('EACCES: permission denied')); - // `log.warn` also records to debug.log; a second `log.debug` would print it twice under --verbose. - expect(log.debug).not.toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed')); + // A detached SessionStart pull discards its output: debug.log keeps the record. + expect(log.persist).toHaveBeenCalledWith(expect.stringContaining('The built-in teamai skill was not deployed: EACCES')); }); it('should do full sync when HEAD rev differs from lastPullRev', async () => { diff --git a/src/builtin-skills.ts b/src/builtin-skills.ts index 485cfcd8..c942d18b 100644 --- a/src/builtin-skills.ts +++ b/src/builtin-skills.ts @@ -445,7 +445,10 @@ export async function pruneLegacyBuiltinSkills( log.warn(`Kept "${legacyName}" (${tool}): ${dir} holds files TeamAI did not put there. The packaged files were removed${saved}; delete the rest yourself once you have saved what you need.`); } } catch (e) { - log.warn(`Could not finish removing "${legacyName}" (${tool}) from ${dir}: ${e instanceof Error ? e.message : String(e)}. Whatever is left there stays until the next pull, which tries again.`); + const message = `Could not finish removing "${legacyName}" (${tool}) from ${dir}: ${e instanceof Error ? e.message : String(e)}. Whatever is left there stays until the next pull, which tries again.`; + log.warn(message); + // A detached SessionStart pull discards its output; debug.log keeps the record. + log.persist(message); } } } diff --git a/src/pull.ts b/src/pull.ts index 9f4e513c..3b2d0325 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -732,11 +732,13 @@ async function reconcileEnvForUnchangedRepo( /** * The stub is the agent's only way into TeamAI, so a failure to deploy it is - * not silent. A SessionStart pull runs detached with its output discarded; - * `log.warn` also records to debug.log, which keeps the trace there. + * not silent. A SessionStart pull runs detached with its output discarded, so + * debug.log keeps the record. */ function warnStubNotDeployed(scopeLabel: string, e: unknown): void { - log.warn(`[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`); + const message = `[${scopeLabel}] The built-in teamai skill was not deployed: ${e instanceof Error ? e.message : String(e)}`; + log.warn(message); + log.persist(message); } async function pullForScope( diff --git a/src/utils/logger.ts b/src/utils/logger.ts index 1455478f..38cc4a3c 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -10,7 +10,7 @@ let stderrMode = false; // ─── File transport ───────────────────────────────────── // -// All log.debug(), log.warn() and log.error() calls are persisted to +// All log.debug(), log.error() and log.persist() calls are persisted to // ~/.teamai/debug.log via synchronous append. This ensures // hook processes (short-lived, stdout swallowed by Claude Code) // leave a durable trace for troubleshooting. @@ -143,10 +143,17 @@ export const log = { writeInfoLine(`${chalk.green('✔')} ${msg}`); }, warn(msg: string): void { - writeToFile('WARN', msg); if (silentMode) return; writeInfoLine(`${chalk.yellow('⚠')} ${msg}`); }, + /** + * Record a failure in debug.log only, never on the console: for a warning + * already shown where a detached hook process (stdout and stderr discarded, + * silent) would lose it, without printing it twice under --verbose. + */ + persist(msg: string): void { + writeToFile('WARN', msg); + }, error(msg: string): void { console.error(chalk.red('✖'), msg); writeToFile('ERROR', msg);