From b7fab234d2cd80cbbe2e90519eab3a501944f3f5 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 09:47:42 +0530 Subject: [PATCH 1/9] fix(env): detect the right shell profile file on Windows (#682) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SHELL is never set on Windows, so detectShellProfile() always fell back to ~/.bashrc — but Git Bash starts as a login shell, which reads ~/.bash_profile, ~/.bash_login or ~/.profile, never ~/.bashrc. The env block was written correctly and looked correct on inspection, yet no shell ever sourced it. Give detectShellProfile a Windows branch that prefers an existing login-shell file, in that order, and falls back to .bashrc only when none exist — matching Git for Windows' own fallback in /etc/profile.d/bash_profile.sh, so both agree on what gets sourced. The logic previously lived twice (resources/env.ts and uninstall.ts); both now delegate to a single utils/shell-profile.ts so pull, doctor and uninstall can never resolve to different files. platform is an injectable parameter (default process.platform), same convention as resolveCliPath in utils/cli-path.ts, since CI only runs ubuntu/macos and a hardcoded process.platform would leave the win32 branch permanently uncovered — which is how this went unnoticed. Co-Authored-By: Claude Sonnet 5 --- src/__tests__/env-handler.test.ts | 5 ++ src/__tests__/shell-profile.test.ts | 77 +++++++++++++++++++++++++++++ src/__tests__/uninstall.test.ts | 6 +++ src/doctor-delivery.ts | 2 +- src/resources/env.ts | 18 +++---- src/uninstall.ts | 12 +---- src/utils/shell-profile.ts | 46 +++++++++++++++++ 7 files changed, 144 insertions(+), 22 deletions(-) create mode 100644 src/__tests__/shell-profile.test.ts create mode 100644 src/utils/shell-profile.ts diff --git a/src/__tests__/env-handler.test.ts b/src/__tests__/env-handler.test.ts index 2bd2a0ef..f7d5850b 100644 --- a/src/__tests__/env-handler.test.ts +++ b/src/__tests__/env-handler.test.ts @@ -54,6 +54,10 @@ describe('EnvHandler', () => { vi.stubEnv('HOME', homeDir); vi.stubEnv('SHELL', '/bin/bash'); + // These tests exercise the SHELL-based POSIX branch of detectShellProfile; + // pin the platform so they assert the same thing on a Windows dev machine + // as they do in CI (ubuntu/macos). The win32 branch has its own tests. + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); teamConfig = { team: 'test', @@ -81,6 +85,7 @@ scope: 'user', afterEach(async () => { vi.unstubAllEnvs(); + vi.restoreAllMocks(); await fse.remove(tmpDir); }); diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts new file mode 100644 index 00000000..4e283a88 --- /dev/null +++ b/src/__tests__/shell-profile.test.ts @@ -0,0 +1,77 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import path from 'node:path'; +import os from 'node:os'; +import fse from 'fs-extra'; +import { detectShellProfile } from '../utils/shell-profile.js'; + +/** + * `platform` is passed explicitly to every call below rather than relying on + * `process.platform` (same convention as `resolveCliPath` in cli-path.ts): + * CI only runs ubuntu/macos, so a test that trusted the host platform would + * never exercise the win32 branch — which is exactly how #682 went unnoticed. + */ +describe('detectShellProfile', () => { + let tmpDir: string; + let homeDir: string; + + beforeEach(async () => { + tmpDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-shell-profile-test-')); + homeDir = path.join(tmpDir, 'home'); + await fse.ensureDir(homeDir); + vi.stubEnv('HOME', homeDir); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + await fse.remove(tmpDir); + }); + + describe('POSIX (darwin/linux)', () => { + it('returns .zshrc when SHELL is zsh', async () => { + vi.stubEnv('SHELL', '/bin/zsh'); + expect(await detectShellProfile('linux')).toBe(path.join(homeDir, '.zshrc')); + }); + + it('returns .bashrc when SHELL is bash', async () => { + vi.stubEnv('SHELL', '/bin/bash'); + expect(await detectShellProfile('darwin')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('returns .bashrc when SHELL is unset', async () => { + vi.stubEnv('SHELL', ''); + expect(await detectShellProfile('linux')).toBe(path.join(homeDir, '.bashrc')); + }); + }); + + describe('Windows (win32)', () => { + it('falls back to .bashrc when none of the login-shell files exist', async () => { + // SHELL is never set on Windows; this also proves the branch ignores + // it even when something has set it. + vi.stubEnv('SHELL', '/bin/zsh'); + expect(await detectShellProfile('win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('prefers an existing .bash_profile over .bash_login, .profile and .bashrc', async () => { + await fse.writeFile(path.join(homeDir, '.bash_profile'), ''); + await fse.writeFile(path.join(homeDir, '.bash_login'), ''); + await fse.writeFile(path.join(homeDir, '.profile'), ''); + await fse.writeFile(path.join(homeDir, '.bashrc'), ''); + expect(await detectShellProfile('win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('prefers .bash_login over .profile and .bashrc when .bash_profile is absent', async () => { + await fse.writeFile(path.join(homeDir, '.bash_login'), ''); + await fse.writeFile(path.join(homeDir, '.profile'), ''); + await fse.writeFile(path.join(homeDir, '.bashrc'), ''); + expect(await detectShellProfile('win32')).toBe(path.join(homeDir, '.bash_login')); + }); + + it('falls back to .profile when only it exists — the case from #682', async () => { + // Reported setup: ~/.bashrc present, ~/.bash_profile absent, ~/.profile + // present. Git Bash starts as a login shell and never reads .bashrc. + await fse.writeFile(path.join(homeDir, '.bashrc'), ''); + await fse.writeFile(path.join(homeDir, '.profile'), ''); + expect(await detectShellProfile('win32')).toBe(path.join(homeDir, '.profile')); + }); + }); +}); diff --git a/src/__tests__/uninstall.test.ts b/src/__tests__/uninstall.test.ts index 1685ba77..08f594ee 100644 --- a/src/__tests__/uninstall.test.ts +++ b/src/__tests__/uninstall.test.ts @@ -191,10 +191,16 @@ describe('uninstall', () => { mockReconcileHooks.mockReset(); mockSaveLocalConfig.mockReset(); mockSaveLocalConfigForScope.mockReset(); + // These tests exercise the SHELL-based POSIX branch of detectShellProfile + // via stubbed SHELL values; pin the platform so they assert the same + // thing on a Windows dev machine as they do in CI (ubuntu/macos). The + // win32 branch has its own tests in shell-profile.test.ts. + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); }); afterEach(async () => { vi.unstubAllEnvs(); + vi.restoreAllMocks(); await fse.remove(tmpDir); }); diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index d2e4701c..555a6f5f 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -591,7 +591,7 @@ async function envDeliveryProblems(ctx: DoctorContext): Promise { } // Same resolution the injection runs, not a second copy of it. - const profilePath = teamConfig?.sharing?.env?.shellProfilePath ?? envHandler.detectShellProfile(); + const profilePath = teamConfig?.sharing?.env?.shellProfilePath ?? await envHandler.detectShellProfile(); const profile = await readFileSafe(profilePath); const block = profile === null ? null : envBlockIn(profile); diff --git a/src/resources/env.ts b/src/resources/env.ts index 55669d02..d66cb0ce 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -6,7 +6,7 @@ import type { ResourceItem, TeamaiConfig, LocalConfig } from '../types.js'; import { TEAMAI_ENV_START, TEAMAI_ENV_END, getDataHome, getEnvBackupPath, isSelfMode } from '../types.js'; import { pathExists, readFileSafe, writeFile, ensureDir, fileContentEqual } from '../utils/fs.js'; import { log } from '../utils/logger.js'; -import { getUserHome } from '../utils/home.js'; +import { detectShellProfile as resolveShellProfilePath } from '../utils/shell-profile.js'; // ─── Schema for env.yaml ──────────────────────────────── @@ -280,7 +280,7 @@ export class EnvHandler extends ResourceHandler { if (inject) { const profilePath = teamConfig.sharing.env.shellProfilePath ? teamConfig.sharing.env.shellProfilePath - : this.detectShellProfile(); + : await this.detectShellProfile(); const shellBlock = this.generateShellBlock(teamaiHome); await this.injectShellProfile(profilePath, shellBlock); @@ -430,16 +430,12 @@ export class EnvHandler extends ResourceHandler { * * Public because `doctor` has to check the same file the injection writes: * a second spelling of this choice would check `.bashrc` while the pull - * wrote `.zshrc`, and report a correct install as broken. + * wrote `.zshrc`, and report a correct install as broken. Delegates to the + * shared `utils/shell-profile.js` so `teamai uninstall` resolves the same + * file too (#682). */ - detectShellProfile(): string { - const home = getUserHome(); - const shell = process.env.SHELL ?? ''; - - if (shell.includes('zsh')) { - return path.join(home, '.zshrc'); - } - return path.join(home, '.bashrc'); + detectShellProfile(platform: NodeJS.Platform = process.platform): Promise { + return resolveShellProfilePath(platform); } /** diff --git a/src/uninstall.ts b/src/uninstall.ts index f27e79f4..ac38433c 100644 --- a/src/uninstall.ts +++ b/src/uninstall.ts @@ -53,6 +53,7 @@ import { import { log } from './utils/logger.js'; import { askConfirmation } from './utils/prompt.js'; import { getUserHome } from './utils/home.js'; +import { detectShellProfile } from './utils/shell-profile.js'; // ─── Types ───────────────────────────────────────────── @@ -131,15 +132,6 @@ const CLAUDEMD_MARKER_PAIRS: Array<[string, string]> = [ [TEAMAI_RECALL_RULES_START, TEAMAI_RECALL_RULES_END], ]; -function detectShellProfile(): string { - const home = getUserHome(); - const shell = process.env.SHELL ?? ''; - if (shell.includes('zsh')) { - return path.join(home, '.zshrc'); - } - return path.join(home, '.bashrc'); -} - /** * Collect team repo skill names, handling both flat and namespaced layouts. * A directory is a namespace if it does NOT contain SKILL.md. @@ -520,7 +512,7 @@ async function buildRemovalPlan( // (e) Shell profile env block const shellProfilePath = teamConfig.sharing.env.shellProfilePath ? expandHome(teamConfig.sharing.env.shellProfilePath) - : detectShellProfile(); + : await detectShellProfile(); if (shellProfilePath) { const profileContent = await readFileSafe(shellProfilePath); if (profileContent && profileContent.includes(TEAMAI_ENV_START)) { diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts new file mode 100644 index 00000000..ff4f23d9 --- /dev/null +++ b/src/utils/shell-profile.ts @@ -0,0 +1,46 @@ +import path from 'node:path'; +import { pathExists } from './fs.js'; +import { getUserHome } from './home.js'; + +/** + * Detect the shell profile file `teamai`'s env block should be injected into. + * + * Shared by `EnvHandler.detectShellProfile` (resources/env.ts) and + * `teamai uninstall` so both resolve the same file — a second, independent + * copy previously drifted (#682) and left `uninstall` unable to find the + * block `pull` had written. + * + * `platform` is injectable because CI only runs ubuntu/macos: hardcoding + * `process.platform` would leave the Windows branch permanently uncovered, + * which is how #682 went unnoticed. Same pattern as `resolveCliPath` in + * `utils/cli-path.ts`. + * + * On Windows, `SHELL` is never set, so the POSIX logic below always fell + * back to `~/.bashrc` — but Git Bash starts as a *login* shell, which reads + * `~/.bash_profile`, `~/.bash_login` or `~/.profile`, never `~/.bashrc`. The + * block was written correctly and looked correct on inspection, yet no shell + * ever sourced it. This mirrors Git for Windows' own fallback in + * `/etc/profile.d/bash_profile.sh`: it only generates a `.bash_profile` that + * sources `.bashrc` when none of the three files exist, so preferring an + * existing one of them — and falling back to `.bashrc` only when none exist — + * agrees with what Git for Windows itself will end up sourcing. + */ +export async function detectShellProfile( + platform: NodeJS.Platform = process.platform, +): Promise { + const home = getUserHome(); + + if (platform === 'win32') { + for (const name of ['.bash_profile', '.bash_login', '.profile']) { + const candidate = path.join(home, name); + if (await pathExists(candidate)) return candidate; + } + return path.join(home, '.bashrc'); + } + + const shell = process.env.SHELL ?? ''; + if (shell.includes('zsh')) { + return path.join(home, '.zshrc'); + } + return path.join(home, '.bashrc'); +} From 7c1659b1ba35b2469e055805e0afed0ac55c5818 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 11:33:31 +0530 Subject: [PATCH 2/9] fix(env): preserve zsh detection and clean up legacy profiles (review) Two P1s from the automated review on #693: - detectShellProfile ignored SHELL entirely on win32. A zsh installed via MSYS2/Cygwin sets SHELL just like it does on POSIX, while native Windows Node still reports platform === 'win32'; the Windows branch was taking over unconditionally and silently stopped loading .zshrc for that setup. SHELL is now checked before the platform branch, on every platform. - uninstall only looked at the one file detectShellProfile() resolves to today. The #682 fix changed which file `pull` prefers, so a machine last pulled with an older CLI can carry a stale block in a file the current resolution no longer points at, and a plain uninstall silently left it behind. buildRemovalPlan now scans every profile file teamai could ever have written to (.zshrc/.bashrc/.bash_profile/.bash_login/.profile, plus a configured override) and cleans every one that still carries a block. Verified both with a real CLI build against the exact scenarios described: zsh-on-Windows via SHELL, and a stale .bashrc block surviving alongside a freshly-written .profile block until `uninstall` now removes both. Co-Authored-By: Claude Sonnet 5 --- src/__tests__/shell-profile.test.ts | 14 +++++--- src/__tests__/uninstall.test.ts | 53 +++++++++++++++++++++++++++++ src/uninstall.ts | 52 ++++++++++++++++++---------- src/utils/shell-profile.ts | 28 +++++++++------ 4 files changed, 115 insertions(+), 32 deletions(-) diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 4e283a88..bcfc6d07 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -44,10 +44,16 @@ describe('detectShellProfile', () => { }); describe('Windows (win32)', () => { - it('falls back to .bashrc when none of the login-shell files exist', async () => { - // SHELL is never set on Windows; this also proves the branch ignores - // it even when something has set it. - vi.stubEnv('SHELL', '/bin/zsh'); + it('returns .zshrc when SHELL is zsh, even on win32 (MSYS2/Cygwin zsh)', async () => { + // A zsh installed via MSYS2/Cygwin sets SHELL just like it does on + // POSIX, while native Windows Node still reports platform === win32. + // SHELL-based detection must win here, or this setup regresses. + vi.stubEnv('SHELL', '/usr/bin/zsh'); + expect(await detectShellProfile('win32')).toBe(path.join(homeDir, '.zshrc')); + }); + + it('falls back to .bashrc when SHELL is unset and none of the login-shell files exist', async () => { + vi.stubEnv('SHELL', ''); expect(await detectShellProfile('win32')).toBe(path.join(homeDir, '.bashrc')); }); diff --git a/src/__tests__/uninstall.test.ts b/src/__tests__/uninstall.test.ts index 08f594ee..a9274dca 100644 --- a/src/__tests__/uninstall.test.ts +++ b/src/__tests__/uninstall.test.ts @@ -253,6 +253,59 @@ describe('uninstall', () => { expect(await fse.pathExists(teamaiHome)).toBe(false); }); + // Regression (#693 review): the Windows fix in #682 changed which profile + // file `pull` prefers, so a machine last pulled with an older CLI can carry + // a stale env block in a file the current detectShellProfile() resolution + // no longer points at (a fresh pull then adds a second block elsewhere). + // uninstall must find and clean every such file, not only the current one. + it('cleans a stale env block left in an old profile file alongside the current one', async () => { + const { homeDir, repoPath, teamaiHome } = await setupFixture(tmpDir); + vi.stubEnv('HOME', homeDir); + vi.stubEnv('SHELL', ''); + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + + // Stale block: what an older CLI wrote to .bashrc before #682. + const staleBashrc = [ + '# my bashrc', + TEAMAI_ENV_START, + '# DO NOT EDIT', + '[ -f ~/.teamai/env.sh ] && source ~/.teamai/env.sh', + TEAMAI_ENV_END, + ].join('\n'); + await fse.writeFile(path.join(homeDir, '.bashrc'), staleBashrc); + + // Current block: what the fixed CLI writes to .profile today. + const currentProfile = [ + '# my profile', + TEAMAI_ENV_START, + '# DO NOT EDIT', + '[ -f ~/.teamai/env.sh ] && source ~/.teamai/env.sh', + TEAMAI_ENV_END, + ].join('\n'); + await fse.writeFile(path.join(homeDir, '.profile'), currentProfile); + + const teamConfig = makeTeamConfig({ + sharing: { + skills: {}, + rules: { enforced: [] }, + docs: { localDir: `${teamaiHome}/docs` }, + env: { injectShellProfile: true }, + }, + }); + const localConfig = makeLocalConfig(homeDir, repoPath); + mockAutoDetectInit.mockResolvedValue({ localConfig, teamConfig }); + + await uninstall({ force: true }); + + const bashrc = await fse.readFile(path.join(homeDir, '.bashrc'), 'utf-8'); + expect(bashrc).toContain('# my bashrc'); + expect(bashrc).not.toContain(TEAMAI_ENV_START); + + const profile = await fse.readFile(path.join(homeDir, '.profile'), 'utf-8'); + expect(profile).toContain('# my profile'); + expect(profile).not.toContain(TEAMAI_ENV_START); + }); + // Regression: Cursor rules are `.mdc`; matching only `.md` left every team // rule on disk after uninstall, still injected into each Cursor session. it('removes cursor .mdc rules (and a legacy .md copy) on uninstall', async () => { diff --git a/src/uninstall.ts b/src/uninstall.ts index ac38433c..ffda6aa6 100644 --- a/src/uninstall.ts +++ b/src/uninstall.ts @@ -82,8 +82,8 @@ interface RemovalPlan { agentFiles: string[]; /** teamai-managed MCP servers from managed-mcp.json (`tool/server` or `tool:project/server`). */ mcpServers: string[]; - /** Shell profile path containing env block (null if none). */ - shellProfile: string | null; + /** Shell profile paths carrying a teamai env block (usually one, but see #682/#693). */ + shellProfiles: string[]; /** Docs directory (null if doesn't exist). */ docsDir: string | null; /** The .teamai home directory path. */ @@ -132,6 +132,9 @@ const CLAUDEMD_MARKER_PAIRS: Array<[string, string]> = [ [TEAMAI_RECALL_RULES_START, TEAMAI_RECALL_RULES_END], ]; +/** Every profile file `detectShellProfile()` could ever have resolved to, across platforms and CLI versions. */ +const SHELL_PROFILE_CANDIDATE_NAMES = ['.zshrc', '.bashrc', '.bash_profile', '.bash_login', '.profile']; + /** * Collect team repo skill names, handling both flat and namespaced layouts. * A directory is a namespace if it does NOT contain SKILL.md. @@ -468,7 +471,7 @@ async function buildRemovalPlan( ruleFiles: [], agentFiles: [], mcpServers: [], - shellProfile: null, + shellProfiles: [], docsDir: null, teamaiHome, teamaiHomeExists: includeShared && await pathExists(teamaiHome), @@ -509,14 +512,24 @@ async function buildRemovalPlan( } plan.mcpServers.sort(); - // (e) Shell profile env block - const shellProfilePath = teamConfig.sharing.env.shellProfilePath + // (e) Shell profile env block(s). Scan every profile file teamai could + // ever have written to, not just the one detectShellProfile() resolves to + // today: the Windows fix (#682) changed which file `pull` prefers, so a + // machine last pulled with an older CLI can carry a stale block in a file + // the current resolution no longer points at, and a plain uninstall would + // silently leave that managed block behind. + const configuredProfilePath = teamConfig.sharing.env.shellProfilePath ? expandHome(teamConfig.sharing.env.shellProfilePath) : await detectShellProfile(); - if (shellProfilePath) { - const profileContent = await readFileSafe(shellProfilePath); + const home = getUserHome(); + const candidateProfilePaths = Array.from(new Set([ + configuredProfilePath, + ...SHELL_PROFILE_CANDIDATE_NAMES.map((name) => path.join(home, name)), + ])); + for (const candidate of candidateProfilePaths) { + const profileContent = await readFileSafe(candidate); if (profileContent && profileContent.includes(TEAMAI_ENV_START)) { - plan.shellProfile = shellProfilePath; + plan.shellProfiles.push(candidate); } } @@ -543,7 +556,7 @@ function isPlanEmpty(plan: RemovalPlan): boolean { plan.ruleFiles.length === 0 && plan.agentFiles.length === 0 && plan.mcpServers.length === 0 && - plan.shellProfile === null && + plan.shellProfiles.length === 0 && plan.docsDir === null && !plan.teamaiHomeExists ); @@ -629,9 +642,11 @@ function printSummary(plan: RemovalPlan, agentFilter?: string): void { console.log(''); } - if (plan.shellProfile) { - console.log(' Shell profile env block:'); - console.log(` ${plan.shellProfile}`); + if (plan.shellProfiles.length > 0) { + console.log(` Shell profile env blocks (${plan.shellProfiles.length}):`); + for (const profilePath of plan.shellProfiles) { + console.log(` ${profilePath}`); + } console.log(''); } @@ -787,22 +802,23 @@ async function executeRemoval(plan: RemovalPlan): Promise { log.success(`Removed ${plan.agentFiles.length} agent files`); } - // (e) Clean shell profile env block - if (plan.shellProfile) { + // (e) Clean shell profile env block(s) — every file discovered in + // buildRemovalPlan, not just the one detectShellProfile() resolves to today. + for (const profilePath of plan.shellProfiles) { try { - const content = await readFileSafe(plan.shellProfile); + const content = await readFileSafe(profilePath); if (content) { const startIdx = content.indexOf(TEAMAI_ENV_START); const endIdx = content.indexOf(TEAMAI_ENV_END); if (startIdx !== -1 && endIdx !== -1) { const before = content.substring(0, startIdx).replace(/\n+$/, '\n'); const after = content.substring(endIdx + TEAMAI_ENV_END.length).replace(/^\n+/, '\n'); - await writeFile(plan.shellProfile, before + after); - log.success(`Cleaned shell profile: ${plan.shellProfile}`); + await writeFile(profilePath, before + after); + log.success(`Cleaned shell profile: ${profilePath}`); } } } catch (e) { - log.warn(`Failed to clean shell profile: ${(e as Error).message}`); + log.warn(`Failed to clean shell profile ${profilePath}: ${(e as Error).message}`); } } diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index ff4f23d9..f5885fd5 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -15,11 +15,19 @@ import { getUserHome } from './home.js'; * which is how #682 went unnoticed. Same pattern as `resolveCliPath` in * `utils/cli-path.ts`. * - * On Windows, `SHELL` is never set, so the POSIX logic below always fell - * back to `~/.bashrc` — but Git Bash starts as a *login* shell, which reads - * `~/.bash_profile`, `~/.bash_login` or `~/.profile`, never `~/.bashrc`. The - * block was written correctly and looked correct on inspection, yet no shell - * ever sourced it. This mirrors Git for Windows' own fallback in + * `SHELL` is checked before the platform branch, on every platform: a zsh + * installed via MSYS2/Cygwin on Windows sets `SHELL` just like it does on + * POSIX, and native Windows Node still reports `platform === 'win32'` in + * that case. Deferring to the Windows branch unconditionally would silently + * stop loading `.zshrc` for that setup, even though `SHELL`-based detection + * already got it right. + * + * On Windows, when `SHELL` does not indicate zsh, `SHELL` is otherwise never + * set, so the POSIX logic below always fell back to `~/.bashrc` — but Git + * Bash starts as a *login* shell, which reads `~/.bash_profile`, + * `~/.bash_login` or `~/.profile`, never `~/.bashrc`. The block was written + * correctly and looked correct on inspection, yet no shell ever sourced it. + * This mirrors Git for Windows' own fallback in * `/etc/profile.d/bash_profile.sh`: it only generates a `.bash_profile` that * sources `.bashrc` when none of the three files exist, so preferring an * existing one of them — and falling back to `.bashrc` only when none exist — @@ -29,18 +37,18 @@ export async function detectShellProfile( platform: NodeJS.Platform = process.platform, ): Promise { const home = getUserHome(); + const shell = process.env.SHELL ?? ''; + + if (shell.includes('zsh')) { + return path.join(home, '.zshrc'); + } if (platform === 'win32') { for (const name of ['.bash_profile', '.bash_login', '.profile']) { const candidate = path.join(home, name); if (await pathExists(candidate)) return candidate; } - return path.join(home, '.bashrc'); } - const shell = process.env.SHELL ?? ''; - if (shell.includes('zsh')) { - return path.join(home, '.zshrc'); - } return path.join(home, '.bashrc'); } From 4043f69832434ef8eed044b2f09a20c470167bd6 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 11:46:45 +0530 Subject: [PATCH 3/9] fix(uninstall): scope legacy shell-profile cleanup to this data home (review) Two more findings from the automated review on #693: - buildRemovalPlan scanned every candidate profile filename for the generic TEAMAI_ENV_START marker alone, so uninstalling one scope (e.g. a project) could delete a completely different scope's still-active block just because it happened to live in one of the same candidate files. A candidate now only counts when its block's source line actually points at THIS scope's own env.sh (getDataHome(localConfig)/env.sh), reusing the same block-parsing logic doctor already relies on for the equivalent check. - extractEnvBlock/envBlockSourcesPath were private to doctor-delivery.ts; moved into utils/shell-profile.ts and imported by both doctor-delivery and uninstall so there is one implementation of "does this block belong to this data home", not two that could drift the same way detectShellProfile itself did in #682. - Documented the Windows shell-profile selection order and the scoped-cleanup behavior in usage-guide.md and its zh-CN counterpart. Verified with new unit tests (a stale-block-survives-in-another-scope case, and the original same-scope migration case using the real generated block format instead of a `~/env.sh` shorthand) and a real CLI build reproducing an unrelated scope's block surviving an uninstall that only touches its own. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 4 ++- docs/usage-guide.zh-CN.md | 4 ++- src/__tests__/uninstall.test.ts | 55 ++++++++++++++++++++++++++++++--- src/doctor-delivery.ts | 33 +++----------------- src/uninstall.ts | 19 +++++++++--- src/utils/shell-profile.ts | 35 +++++++++++++++++++++ 6 files changed, 110 insertions(+), 40 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 34aa99a8..593883e6 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -743,6 +743,8 @@ teamai env list teamai push ``` +On `pull`, when `injectShellProfile` is enabled (default), the env block goes into `~/.zshrc` if `$SHELL` is zsh, otherwise `~/.bashrc` — except on Windows: `$SHELL` is normally unset there, and Git Bash starts as a *login* shell that never reads `.bashrc`, so teamai instead prefers an existing `~/.bash_profile`, then `~/.bash_login`, then `~/.profile`, falling back to `~/.bashrc` only when none of them exist (a zsh installed via MSYS2/Cygwin, which does set `$SHELL`, still resolves to `.zshrc`). Override the target file with `sharing.env.shellProfilePath` in `teamai.yaml`. + ### Docs Place documentation in the team repo's `docs/` directory; after pushing, team members will automatically receive it on their next `pull`. @@ -1775,7 +1777,7 @@ What gets removed: - Team-synced skills, including OpenClaw workspace skills (your own skills are preserved) - Team-synced rules - Team-synced custom agents and CLI built-in agents (your own agents are preserved) -- The env block in your shell profile +- The env block in your shell profile — every candidate file (`.zshrc`, `.bashrc`, `.bash_profile`, `.bash_login`, `.profile`) carrying a block that sources this scope's own `env.sh` is cleaned, not only the one file `pull` would choose today; a block sourcing a different scope's `env.sh` is left alone - The `~/.teamai/` directory ### Uninstall a single tool (`--agent `) diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 9eb3f587..a47cb679 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -716,6 +716,8 @@ teamai env list teamai push ``` +`pull` 时，若启用了 `injectShellProfile`（默认启用），`$SHELL` 为 zsh 时环境变量块会写入 `~/.zshrc`，否则写入 `~/.bashrc`——但 Windows 上例外：`$SHELL` 通常未设置，而 Git Bash 以*登录 shell*方式启动，从不读取 `.bashrc`，因此 teamai 会优先选择已存在的 `~/.bash_profile`、其次 `~/.bash_login`、再次 `~/.profile`，只有三者都不存在时才回退到 `~/.bashrc`（通过 MSYS2/Cygwin 安装、会设置 `$SHELL` 的 zsh 仍会解析到 `.zshrc`）。可通过 `teamai.yaml` 中的 `sharing.env.shellProfilePath` 覆盖目标文件。 + ### Docs（文档） 将文档放入团队仓库 `docs/` 目录，push 后团队成员 pull 时自动同步。 @@ -1723,7 +1725,7 @@ teamai uninstall --agent claude - 团队同步的 skills，包括 OpenClaw workspace skills（保留用户自建 skills） - 团队同步的 rules - 团队同步的自定义 agents 和 CLI 内置 agents（保留用户自建 agents） -- Shell profile 中的 env 块 +- Shell profile 中的 env 块——会清理每一个候选文件（`.zshrc`、`.bashrc`、`.bash_profile`、`.bash_login`、`.profile`）中、代码块指向本作用域自身 `env.sh` 的那些，而不仅仅是当前 `pull` 会选中的那一个；指向其他作用域 `env.sh` 的代码块不受影响 - `~/.teamai/` 目录 ### 只卸载单个工具（`--agent `） diff --git a/src/__tests__/uninstall.test.ts b/src/__tests__/uninstall.test.ts index a9274dca..ed1715c8 100644 --- a/src/__tests__/uninstall.test.ts +++ b/src/__tests__/uninstall.test.ts @@ -156,14 +156,17 @@ async function setupFixture(tmpDir: string) { ].join('\n'); await fse.writeFile(path.join(homeDir, '.claude', 'CLAUDE.md'), claudeMd); - // Shell profile with env block + // Shell profile with env block. The source line points at this scope's own + // env.sh (not a `~` shorthand) since uninstall now only cleans a block that + // sources the current scope's data home (#693 review). + const envShPosix = path.join(homeDir, '.teamai', 'env.sh').split(path.sep).join('/'); const zshrc = [ '# My zshrc config', 'export PATH=$HOME/bin:$PATH', '', TEAMAI_ENV_START, '# DO NOT EDIT', - '[ -f ~/.teamai/env.sh ] && source ~/.teamai/env.sh', + `[ -f '${envShPosix}' ] && source '${envShPosix}'`, TEAMAI_ENV_END, '', '# More user config', @@ -264,12 +267,16 @@ describe('uninstall', () => { vi.stubEnv('SHELL', ''); vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + // Both blocks source THIS scope's own env.sh — only their file location + // differs, exactly like an upgrade across #682 would leave things. + const envShPosix = path.join(homeDir, '.teamai', 'env.sh').split(path.sep).join('/'); + // Stale block: what an older CLI wrote to .bashrc before #682. const staleBashrc = [ '# my bashrc', TEAMAI_ENV_START, '# DO NOT EDIT', - '[ -f ~/.teamai/env.sh ] && source ~/.teamai/env.sh', + `[ -f '${envShPosix}' ] && source '${envShPosix}'`, TEAMAI_ENV_END, ].join('\n'); await fse.writeFile(path.join(homeDir, '.bashrc'), staleBashrc); @@ -279,7 +286,7 @@ describe('uninstall', () => { '# my profile', TEAMAI_ENV_START, '# DO NOT EDIT', - '[ -f ~/.teamai/env.sh ] && source ~/.teamai/env.sh', + `[ -f '${envShPosix}' ] && source '${envShPosix}'`, TEAMAI_ENV_END, ].join('\n'); await fse.writeFile(path.join(homeDir, '.profile'), currentProfile); @@ -306,6 +313,46 @@ describe('uninstall', () => { expect(profile).not.toContain(TEAMAI_ENV_START); }); + // Regression (#693 review): scanning every candidate filename must not + // delete a DIFFERENT scope's still-active block just because it also + // happens to carry the teamai marker — only a block sourcing THIS scope's + // own env.sh may be touched. + it('leaves another scope\'s env block untouched even though it shares a candidate filename', async () => { + const { homeDir, repoPath, teamaiHome } = await setupFixture(tmpDir); + vi.stubEnv('HOME', homeDir); + vi.stubEnv('SHELL', ''); + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + + // A project scope's block, unrelated to the user-scope uninstall below. + const otherProjectEnvSh = path.join(tmpDir, 'other-project', '.teamai', 'env.sh') + .split(path.sep).join('/'); + const bashrc = [ + '# my bashrc', + TEAMAI_ENV_START, + '# DO NOT EDIT', + `[ -f '${otherProjectEnvSh}' ] && source '${otherProjectEnvSh}'`, + TEAMAI_ENV_END, + ].join('\n'); + await fse.writeFile(path.join(homeDir, '.bashrc'), bashrc); + + const teamConfig = makeTeamConfig({ + sharing: { + skills: {}, + rules: { enforced: [] }, + docs: { localDir: `${teamaiHome}/docs` }, + env: { injectShellProfile: true }, + }, + }); + const localConfig = makeLocalConfig(homeDir, repoPath); // scope: 'user' + mockAutoDetectInit.mockResolvedValue({ localConfig, teamConfig }); + + await uninstall({ force: true }); + + // The unrelated project-scope block must survive intact. + const bashrcAfter = await fse.readFile(path.join(homeDir, '.bashrc'), 'utf-8'); + expect(bashrcAfter).toBe(bashrc); + }); + // Regression: Cursor rules are `.mdc`; matching only `.md` left every team // rule on disk after uninstall, still injected into each Cursor session. it('removes cursor .mdc rules (and a legacy .md copy) on uninstall', async () => { diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index 555a6f5f..0f6c2f19 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -2,11 +2,12 @@ import path from 'node:path'; import fs from 'node:fs'; import { isDeepStrictEqual } from 'node:util'; import { expandHome, listFilesRecursive, pathExists, readFileSafe } from './utils/fs.js'; -import { getDataHome, getMcpSharing, isAgentExcluded, TEAMAI_ENV_START, TEAMAI_ENV_END } from './types.js'; +import { getDataHome, getMcpSharing, isAgentExcluded } from './types.js'; import type { DeliveryTarget, ResourceItem } from './types.js'; import { splitFrontmatter } from './utils/frontmatter.js'; import type { ResourceHandler } from './resources/base.js'; import type { Check, DoctorContext } from './doctor.js'; +import { extractEnvBlock, envBlockSourcesPath } from './utils/shell-profile.js'; /** * The checks that verify the payload rather than the plumbing: what each tool @@ -491,32 +492,6 @@ export async function buildMcpDeliveryChecks(ctx: DoctorContext): Promise { // Same resolution the injection runs, not a second copy of it. const profilePath = teamConfig?.sharing?.env?.shellProfilePath ?? await envHandler.detectShellProfile(); const profile = await readFileSafe(profilePath); - const block = profile === null ? null : envBlockIn(profile); + const block = profile === null ? null : extractEnvBlock(profile); if (block === null) { problems.push(`${profilePath} carries no TeamAI env block`); - } else if (envSh !== null && !envBlockLoads(block, envShPath)) { + } else if (envSh !== null && !envBlockSourcesPath(block, envShPath)) { problems.push( `the block in ${profilePath} does not load ${envShPath}: a POSIX shell reads an unquoted ` + 'backslash as an escape, so the `[ -f ... ]` test fails and `source` never runs', diff --git a/src/uninstall.ts b/src/uninstall.ts index ffda6aa6..d94f318d 100644 --- a/src/uninstall.ts +++ b/src/uninstall.ts @@ -53,7 +53,12 @@ import { import { log } from './utils/logger.js'; import { askConfirmation } from './utils/prompt.js'; import { getUserHome } from './utils/home.js'; -import { detectShellProfile } from './utils/shell-profile.js'; +import { + detectShellProfile, + extractEnvBlock, + envBlockSourcesPath, + SHELL_PROFILE_CANDIDATE_NAMES, +} from './utils/shell-profile.js'; // ─── Types ───────────────────────────────────────────── @@ -132,9 +137,6 @@ const CLAUDEMD_MARKER_PAIRS: Array<[string, string]> = [ [TEAMAI_RECALL_RULES_START, TEAMAI_RECALL_RULES_END], ]; -/** Every profile file `detectShellProfile()` could ever have resolved to, across platforms and CLI versions. */ -const SHELL_PROFILE_CANDIDATE_NAMES = ['.zshrc', '.bashrc', '.bash_profile', '.bash_login', '.profile']; - /** * Collect team repo skill names, handling both flat and namespaced layouts. * A directory is a namespace if it does NOT contain SKILL.md. @@ -518,17 +520,24 @@ async function buildRemovalPlan( // machine last pulled with an older CLI can carry a stale block in a file // the current resolution no longer points at, and a plain uninstall would // silently leave that managed block behind. + // + // A candidate only counts if its block actually sources THIS scope's + // env.sh (envBlockSourcesPath) — matching on the marker alone would let + // this uninstall delete a different scope's still-active block just + // because it also happens to live in one of the candidate filenames. const configuredProfilePath = teamConfig.sharing.env.shellProfilePath ? expandHome(teamConfig.sharing.env.shellProfilePath) : await detectShellProfile(); const home = getUserHome(); + const envShPath = path.join(getDataHome(localConfig), 'env.sh'); const candidateProfilePaths = Array.from(new Set([ configuredProfilePath, ...SHELL_PROFILE_CANDIDATE_NAMES.map((name) => path.join(home, name)), ])); for (const candidate of candidateProfilePaths) { const profileContent = await readFileSafe(candidate); - if (profileContent && profileContent.includes(TEAMAI_ENV_START)) { + const block = profileContent ? extractEnvBlock(profileContent) : null; + if (block && envBlockSourcesPath(block, envShPath)) { plan.shellProfiles.push(candidate); } } diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index f5885fd5..68f19396 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -1,6 +1,10 @@ import path from 'node:path'; import { pathExists } from './fs.js'; import { getUserHome } from './home.js'; +import { TEAMAI_ENV_START, TEAMAI_ENV_END } from '../types.js'; + +/** Every profile file `detectShellProfile()` could ever have resolved to, across platforms and CLI versions. */ +export const SHELL_PROFILE_CANDIDATE_NAMES = ['.zshrc', '.bashrc', '.bash_profile', '.bash_login', '.profile']; /** * Detect the shell profile file `teamai`'s env block should be injected into. @@ -52,3 +56,34 @@ export async function detectShellProfile( return path.join(home, '.bashrc'); } + +/** The TeamAI-managed block of a shell profile, or null when it is absent. */ +export function extractEnvBlock(profileContent: string): string | null { + const start = profileContent.indexOf(TEAMAI_ENV_START); + if (start === -1) return null; + const end = profileContent.indexOf(TEAMAI_ENV_END, start); + return end === -1 ? profileContent.slice(start) : profileContent.slice(start, end); +} + +/** + * Whether an env block's `source` line actually points at `envShPath`. + * + * A profile can carry more than one teamai-managed block over its lifetime — + * one per data home that ever injected into it (a different project scope, + * or a stale one #682 left in a file the current platform/version no longer + * resolves to). Matching on the marker alone would let one scope's uninstall + * delete another scope's still-active block just because it also happens to + * be a teamai block; comparing against this scope's own `env.sh` path scopes + * the match to blocks this run is actually responsible for. + * + * The block is generated by joining paths with the platform separator, so on + * Windows it carries backslashes. An unquoted `\` is an escape character in a + * POSIX shell, so the generator rewrites it to `/` before writing — compare + * against that same rewritten form, not the raw OS path. + */ +export function envBlockSourcesPath(block: string, envShPath: string): boolean { + const posixPath = envShPath.split(path.sep).join('/'); + if (!block.includes(posixPath)) return false; + if (!/\s/.test(posixPath)) return true; + return block.includes(`"${posixPath}"`) || block.includes(`'${posixPath}'`); +} From a983ef0a10f37a209abdebbe363c7a29bba9c103 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 11:54:52 +0530 Subject: [PATCH 4/9] fix(env): compare env-block paths against the generator's own quoting (review) envBlockSourcesPath compared the raw path against the block, but generateShellBlock always wraps the path in single quotes via shellQuoteValue, which escapes an embedded apostrophe as `'\''`. A path like /home/O'Brien/.teamai/env.sh never appears as a contiguous raw substring in the generated block, so doctor reported a correctly loading block as broken, and uninstall could not find it to clean up. Moved shellQuoteValue out of resources/env.ts into the shared utils/shell-profile.ts (env.ts now imports it) so the check compares against the exact string the generator would produce, instead of re-deriving the same escaping rule a second time. Added unit tests for envBlockSourcesPath covering the apostrophe case, a plain path, a non-matching path, and the pre-existing #661 unquoted Windows path case, using the real shellQuoteValue rather than hand-written escaping to avoid re-encoding the same assumption twice. Co-Authored-By: Claude Sonnet 5 --- src/__tests__/shell-profile.test.ts | 32 ++++++++++++++++++++++++++++- src/resources/env.ts | 12 +---------- src/utils/shell-profile.ts | 21 +++++++++++++++++++ 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index bcfc6d07..6a7bf5d5 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import path from 'node:path'; import os from 'node:os'; import fse from 'fs-extra'; -import { detectShellProfile } from '../utils/shell-profile.js'; +import { detectShellProfile, envBlockSourcesPath, shellQuoteValue } from '../utils/shell-profile.js'; /** * `platform` is passed explicitly to every call below rather than relying on @@ -81,3 +81,33 @@ describe('detectShellProfile', () => { }); }); }); + +describe('envBlockSourcesPath', () => { + it('matches a plain path in the generator\'s single-quoted form', () => { + const envShPath = '/home/user/.teamai/env.sh'; + const block = `[ -f ${shellQuoteValue(envShPath)} ] && source ${shellQuoteValue(envShPath)}`; + expect(envBlockSourcesPath(block, envShPath)).toBe(true); + }); + + // Regression (#693 review): shellQuoteValue escapes an embedded apostrophe + // as `'\''`, so a raw substring check for `/home/O'Brien/...` never matches + // — the block only ever contains the escaped form. + it('matches a home path containing an apostrophe (generator escapes it as \'\\\'\')', () => { + const envShPath = "/home/O'Brien/.teamai/env.sh"; + const block = `[ -f ${shellQuoteValue(envShPath)} ] && source ${shellQuoteValue(envShPath)}`; + expect(block).toContain(String.raw`O'\''Brien`); + expect(envBlockSourcesPath(block, envShPath)).toBe(true); + }); + + it('does not match a different path', () => { + const block = `[ -f ${shellQuoteValue('/home/user/.teamai/env.sh')} ] && source ${shellQuoteValue('/home/user/.teamai/env.sh')}`; + expect(envBlockSourcesPath(block, '/home/other/.teamai/env.sh')).toBe(false); + }); + + it('does not match an unquoted, unconverted Windows path (#661)', () => { + const envShPath = 'C:/Users/me/.teamai/env.sh'; + const windowsForm = envShPath.replace(/\//g, '\\'); + const block = `[ -f ${windowsForm} ] && source ${windowsForm}`; + expect(envBlockSourcesPath(block, envShPath)).toBe(false); + }); +}); diff --git a/src/resources/env.ts b/src/resources/env.ts index d66cb0ce..d13c894f 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -6,7 +6,7 @@ import type { ResourceItem, TeamaiConfig, LocalConfig } from '../types.js'; import { TEAMAI_ENV_START, TEAMAI_ENV_END, getDataHome, getEnvBackupPath, isSelfMode } from '../types.js'; import { pathExists, readFileSafe, writeFile, ensureDir, fileContentEqual } from '../utils/fs.js'; import { log } from '../utils/logger.js'; -import { detectShellProfile as resolveShellProfilePath } from '../utils/shell-profile.js'; +import { detectShellProfile as resolveShellProfilePath, shellQuoteValue } from '../utils/shell-profile.js'; // ─── Schema for env.yaml ──────────────────────────────── @@ -77,16 +77,6 @@ export function maskEnvValue(value: string): string { return `${value.slice(0, 2)}****`; } -/** - * Quote a string so it is safe to interpolate into a POSIX shell (bash/zsh/sh). - * Wraps the value in single quotes and encodes any embedded single quote as - * `'\''`, leaving all other characters (including `"`, `$`, `` ` ``, `\`) - * literal. Used when generating env.sh, which every team member sources. - */ -function shellQuoteValue(value: string): string { - return `'${value.replace(/'/g, "'\\''")}'`; -} - /** * True for a path a shell must read the Windows way: a drive-letter path * (`C:\...` or `C:/...`) or a UNC path (`\\server\share`). diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index 68f19396..57d1aaba 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -57,6 +57,17 @@ export async function detectShellProfile( return path.join(home, '.bashrc'); } +/** + * Quote a string so it is safe to interpolate into a POSIX shell (bash/zsh/sh). + * Wraps the value in single quotes and encodes any embedded single quote as + * `'\''`, leaving all other characters (including `"`, `$`, `` ` ``, `\`) + * literal. Used both when generating env.sh (env.ts) and when checking + * whether a block on disk matches that same generated form. + */ +export function shellQuoteValue(value: string): string { + return `'${value.replace(/'/g, "'\\''")}'`; +} + /** The TeamAI-managed block of a shell profile, or null when it is absent. */ export function extractEnvBlock(profileContent: string): string | null { const start = profileContent.indexOf(TEAMAI_ENV_START); @@ -83,6 +94,16 @@ export function extractEnvBlock(profileContent: string): string | null { */ export function envBlockSourcesPath(block: string, envShPath: string): boolean { const posixPath = envShPath.split(path.sep).join('/'); + + // The generator (generateShellBlock) always wraps the path in single + // quotes via shellQuoteValue, which escapes an embedded apostrophe as + // `'\''` — a path like `/home/O'Brien/.teamai/env.sh` never appears as a + // contiguous raw substring in the block, only in this escaped form. + if (block.includes(shellQuoteValue(posixPath))) return true; + + // Fall back to a raw/loosely-quoted match for anything not in the + // generator's own format — e.g. #661's legacy unconverted backslash path, + // which must still fail this check. if (!block.includes(posixPath)) return false; if (!/\s/.test(posixPath)) return true; return block.includes(`"${posixPath}"`) || block.includes(`'${posixPath}'`); From aea31efea809aade81c6ca71c2d3454020f8c429 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 14:22:58 +0530 Subject: [PATCH 5/9] fix(env): recognize legacy block spellings for cleanup and staleness (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real gaps found by @CarlosWonMore's hardware validation on real Windows 11 machines, both stemming from envBlockSourcesPath only recognizing the current writing format: - A pre-#661 block (raw, unquoted, unconverted backslashes) or a locally-patched build's MSYS/Cygwin drive-form block (/d/Users/...) names the same scope's env.sh but never matched, so uninstall could not find it and doctor never flagged it — a dead block could survive every pull and every uninstall indefinitely. Added envBlockReferencesDataHome: a looser "does this block belong to this scope" check (current + legacy spellings, quoted or not) that answers ownership, kept separate from the existing envBlockSourcesPath which answers "does this block actually load" and must stay strict — conflating them would make doctor stop reporting a real #661-style break just because a legacy spelling happens to match. - uninstall.ts's candidate scan now uses envBlockReferencesDataHome, so a legacy block for this scope is found and removed regardless of which format wrote it. - doctor's env-delivery check now also scans the other candidate files for a stray block belonging to this scope and reports it, naming `teamai uninstall` as the fix — closing the "doctor stays green while a dead block remains" gap. Since this check also runs automatically after every `pull`, a stale legacy block now surfaces immediately instead of staying invisible. Deliberately not doing in this round: automatically migrating/deleting a legacy block during `pull` itself. `doctor` (and `pull`'s own post-pull check) now names the file and the fix, and `uninstall` performs it — that closes the visibility and cleanup gap without teaching `pull`'s write path to also delete files elsewhere, which is a larger behavioral change worth its own review. Verified with unit tests for envBlockReferencesDataHome (current form, pre-#661 raw form, MSYS drive form, different-scope negative case), a new uninstall test cleaning a pre-#661 legacy .bashrc block, a new doctor test flagging a stray legacy block, and a real CLI build reproducing the reviewer's exact repro end-to-end: pull writes the new block to .profile, doctor immediately flags the leftover .bashrc block and names `teamai uninstall`, and uninstall removes both. Also refined the Windows fallback description in usage-guide.md / usage-guide.zh-CN.md with Git for Windows' exact guard condition, per the reviewer's note that the bug looks environment-specific without it. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 4 +- docs/usage-guide.zh-CN.md | 4 +- src/__tests__/doctor-env-delivery.test.ts | 27 ++++++++++++ src/__tests__/shell-profile.test.ts | 41 ++++++++++++++++++- src/__tests__/uninstall.test.ts | 40 ++++++++++++++++++ src/doctor-delivery.ts | 31 +++++++++++++- src/uninstall.ts | 16 +++++--- src/utils/shell-profile.ts | 50 +++++++++++++++++++++++ 8 files changed, 203 insertions(+), 10 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 593883e6..3b70a122 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -743,7 +743,9 @@ teamai env list teamai push ``` -On `pull`, when `injectShellProfile` is enabled (default), the env block goes into `~/.zshrc` if `$SHELL` is zsh, otherwise `~/.bashrc` — except on Windows: `$SHELL` is normally unset there, and Git Bash starts as a *login* shell that never reads `.bashrc`, so teamai instead prefers an existing `~/.bash_profile`, then `~/.bash_login`, then `~/.profile`, falling back to `~/.bashrc` only when none of them exist (a zsh installed via MSYS2/Cygwin, which does set `$SHELL`, still resolves to `.zshrc`). Override the target file with `sharing.env.shellProfilePath` in `teamai.yaml`. +On `pull`, when `injectShellProfile` is enabled (default), the env block goes into `~/.zshrc` if `$SHELL` is zsh, otherwise `~/.bashrc` — except on Windows: `$SHELL` is normally unset there, and Git Bash starts as a *login* shell that never reads `.bashrc`, so teamai instead prefers an existing `~/.bash_profile`, then `~/.bash_login`, then `~/.profile`, falling back to `~/.bashrc` only when none of them exist (a zsh installed via MSYS2/Cygwin, which does set `$SHELL`, still resolves to `.zshrc`). This matches Git for Windows' own fallback in `/etc/profile.d/bash_profile.sh`, whose guard is `[ -e ~/.bashrc -a ! -e ~/.bash_profile -a ! -e ~/.bash_login -a ! -e ~/.profile ]` — it only synthesizes a `.bash_profile` that sources `.bashrc` in that same one case, which is why a stray `~/.profile` (even one that just sources something else, e.g. `~/.local/bin/env`) is enough to make `.bashrc` alone go unread. Override the target file with `sharing.env.shellProfilePath` in `teamai.yaml`. + +`doctor` (and the check `pull` runs automatically afterward) also flags a teamai env block left behind in a *different* candidate file — e.g. a block a pre-#682 install wrote to `.bashrc` before this file-selection logic changed — even if that block is broken and was never functional. `teamai uninstall` removes it. ### Docs diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index a47cb679..cf592a8b 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -716,7 +716,9 @@ teamai env list teamai push ``` -`pull` 时，若启用了 `injectShellProfile`（默认启用），`$SHELL` 为 zsh 时环境变量块会写入 `~/.zshrc`，否则写入 `~/.bashrc`——但 Windows 上例外：`$SHELL` 通常未设置，而 Git Bash 以*登录 shell*方式启动，从不读取 `.bashrc`，因此 teamai 会优先选择已存在的 `~/.bash_profile`、其次 `~/.bash_login`、再次 `~/.profile`，只有三者都不存在时才回退到 `~/.bashrc`（通过 MSYS2/Cygwin 安装、会设置 `$SHELL` 的 zsh 仍会解析到 `.zshrc`）。可通过 `teamai.yaml` 中的 `sharing.env.shellProfilePath` 覆盖目标文件。 +`pull` 时，若启用了 `injectShellProfile`（默认启用），`$SHELL` 为 zsh 时环境变量块会写入 `~/.zshrc`，否则写入 `~/.bashrc`——但 Windows 上例外：`$SHELL` 通常未设置，而 Git Bash 以*登录 shell*方式启动，从不读取 `.bashrc`，因此 teamai 会优先选择已存在的 `~/.bash_profile`、其次 `~/.bash_login`、再次 `~/.profile`，只有三者都不存在时才回退到 `~/.bashrc`（通过 MSYS2/Cygwin 安装、会设置 `$SHELL` 的 zsh 仍会解析到 `.zshrc`）。这与 Git for Windows 自身在 `/etc/profile.d/bash_profile.sh` 中的回退逻辑一致，其判断条件是 `[ -e ~/.bashrc -a ! -e ~/.bash_profile -a ! -e ~/.bash_login -a ! -e ~/.profile ]`——只有在这一种情况下它才会生成一个会 source `.bashrc` 的 `.bash_profile`；这也是为什么哪怕一个只 source 了其他内容（例如 `~/.local/bin/env`）的 `~/.profile` 存在，也足以让 `.bashrc` 单独失效。可通过 `teamai.yaml` 中的 `sharing.env.shellProfilePath` 覆盖目标文件。 + +`doctor`（以及 `pull` 结束后自动运行的检查）还会标记出遗留在*其他*候选文件中的 teamai 环境变量块——例如 #682 之前的旧版本写入 `.bashrc` 的代码块，即便该代码块本身已损坏、从未生效。`teamai uninstall` 会清理它。 ### Docs（文档） diff --git a/src/__tests__/doctor-env-delivery.test.ts b/src/__tests__/doctor-env-delivery.test.ts index bf9bdc6e..9fe7cd06 100644 --- a/src/__tests__/doctor-env-delivery.test.ts +++ b/src/__tests__/doctor-env-delivery.test.ts @@ -119,6 +119,33 @@ describe('doctor — env variables reach a shell', () => { expect(check.fix).toContain(envShPath); }); + // Regression (#693 hardware review by @CarlosWonMore): which file `pull` + // prefers has changed (#682), and `pull` only ever adds a block, never + // migrates an old one away. A stray, still-scope-owned block left behind + // in a different candidate file must not go unreported forever. + it('flags a stray legacy block left in a different candidate file for this scope (#693)', async () => { + await writeEnvSh("export JIRA_PASSWORD='s3cret'\n"); + // Force the resolved profile to .profile, bypassing platform-dependent + // detectShellProfile() so this test is deterministic on any host. + teamConfig.sharing.env.shellProfilePath = path.join(homeDir, '.profile'); + await fse.writeFile( + path.join(homeDir, '.profile'), + `# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${envShPath} ] && source ${envShPath}\n# [teamai:env:end]\n`, + ); + // A pre-#661 legacy block for the SAME env.sh, left behind in .bashrc — + // raw, unquoted, unconverted backslashes. + const windowsEnvSh = envShPath.replace(/\//g, '\\'); + await fse.writeFile( + path.join(homeDir, '.bashrc'), + `# my bashrc\n# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${windowsEnvSh} ] && source ${windowsEnvSh}\n# [teamai:env:end]\n`, + ); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('.bashrc'); + expect(check.fix).toContain('teamai uninstall'); + }); + it('fails and names `variables:` for the shorthand env.yaml form (#662)', async () => { await writeEnvYaml('JIRA_PASSWORD: "s3cret"\n'); await writeEnvSh(''); diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 6a7bf5d5..85cdd16c 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -2,7 +2,12 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import path from 'node:path'; import os from 'node:os'; import fse from 'fs-extra'; -import { detectShellProfile, envBlockSourcesPath, shellQuoteValue } from '../utils/shell-profile.js'; +import { + detectShellProfile, + envBlockSourcesPath, + envBlockReferencesDataHome, + shellQuoteValue, +} from '../utils/shell-profile.js'; /** * `platform` is passed explicitly to every call below rather than relying on @@ -111,3 +116,37 @@ describe('envBlockSourcesPath', () => { expect(envBlockSourcesPath(block, envShPath)).toBe(false); }); }); + +// Regression (#693 hardware review by @CarlosWonMore): envBlockSourcesPath +// only recognizes the current writing format. A block from a pre-#661 or +// pre-#682 CLI names the same env.sh under a different, broken spelling — +// still owned by this scope, and uninstall/doctor's "is there a stray +// leftover" check must still find it to clean it up or flag it. +describe('envBlockReferencesDataHome', () => { + it('matches the current (quoted, forward-slash) form', () => { + const envShPath = 'D:\\Users\\me\\.teamai\\env.sh'; + const posix = envShPath.split('\\').join('/'); + const block = `[ -f ${shellQuoteValue(posix)} ] && source ${shellQuoteValue(posix)}`; + expect(envBlockReferencesDataHome(block, envShPath)).toBe(true); + }); + + it('matches a pre-#661 raw, unquoted, unconverted Windows path', () => { + const envShPath = 'D:\\Users\\me\\.teamai\\env.sh'; + const block = `[ -f ${envShPath} ] && source ${envShPath}`; + expect(envBlockReferencesDataHome(block, envShPath)).toBe(true); + }); + + it('matches the MSYS/Cygwin drive form (/d/Users/...) a locally-patched build wrote', () => { + const envShPath = 'D:\\Users\\me\\.teamai\\env.sh'; + const msysForm = '/d/Users/me/.teamai/env.sh'; + const block = `[ -f ${msysForm} ] && source ${msysForm}`; + expect(envBlockReferencesDataHome(block, envShPath)).toBe(true); + }); + + it('does not match a different scope\'s env.sh', () => { + const envShPath = 'D:\\Users\\me\\.teamai\\env.sh'; + const otherPosix = 'D:/some-other-project/.teamai/env.sh'; + const block = `[ -f ${shellQuoteValue(otherPosix)} ] && source ${shellQuoteValue(otherPosix)}`; + expect(envBlockReferencesDataHome(block, envShPath)).toBe(false); + }); +}); diff --git a/src/__tests__/uninstall.test.ts b/src/__tests__/uninstall.test.ts index ed1715c8..ddb52d83 100644 --- a/src/__tests__/uninstall.test.ts +++ b/src/__tests__/uninstall.test.ts @@ -353,6 +353,46 @@ describe('uninstall', () => { expect(bashrcAfter).toBe(bashrc); }); + // Regression (#693 hardware review by @CarlosWonMore): a pre-#661 CLI wrote + // the source path raw and unquoted, with unconverted backslashes. That + // block is broken (a POSIX shell never loads it) but still names this + // scope's own env.sh, and uninstall must still find and remove it — not + // just blocks written in the current quoted/forward-slash format. + it('cleans a pre-#661 legacy block (raw, unquoted, unconverted backslashes)', async () => { + const { homeDir, repoPath, teamaiHome } = await setupFixture(tmpDir); + vi.stubEnv('HOME', homeDir); + vi.stubEnv('SHELL', ''); + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + + const envShWindows = path.join(homeDir, '.teamai', 'env.sh'); + const legacyBashrc = [ + '# my bashrc', + TEAMAI_ENV_START, + '# DO NOT EDIT', + `[ -f ${envShWindows} ] && source ${envShWindows}`, + TEAMAI_ENV_END, + ].join('\n'); + await fse.writeFile(path.join(homeDir, '.bashrc'), legacyBashrc); + await fse.writeFile(path.join(homeDir, '.profile'), '# my profile'); + + const teamConfig = makeTeamConfig({ + sharing: { + skills: {}, + rules: { enforced: [] }, + docs: { localDir: `${teamaiHome}/docs` }, + env: { injectShellProfile: true }, + }, + }); + const localConfig = makeLocalConfig(homeDir, repoPath); + mockAutoDetectInit.mockResolvedValue({ localConfig, teamConfig }); + + await uninstall({ force: true }); + + const bashrcAfter = await fse.readFile(path.join(homeDir, '.bashrc'), 'utf-8'); + expect(bashrcAfter).toContain('# my bashrc'); + expect(bashrcAfter).not.toContain(TEAMAI_ENV_START); + }); + // Regression: Cursor rules are `.mdc`; matching only `.md` left every team // rule on disk after uninstall, still injected into each Cursor session. it('removes cursor .mdc rules (and a legacy .md copy) on uninstall', async () => { diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index 0f6c2f19..9a901dcc 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -7,7 +7,13 @@ import type { DeliveryTarget, ResourceItem } from './types.js'; import { splitFrontmatter } from './utils/frontmatter.js'; import type { ResourceHandler } from './resources/base.js'; import type { Check, DoctorContext } from './doctor.js'; -import { extractEnvBlock, envBlockSourcesPath } from './utils/shell-profile.js'; +import { + extractEnvBlock, + envBlockSourcesPath, + envBlockReferencesDataHome, + SHELL_PROFILE_CANDIDATE_NAMES, +} from './utils/shell-profile.js'; +import { getUserHome } from './utils/home.js'; /** * The checks that verify the payload rather than the plumbing: what each tool @@ -579,6 +585,29 @@ async function envDeliveryProblems(ctx: DoctorContext): Promise { ); } + // A stray block can also sit in a different candidate file: which file + // `pull` prefers has changed at least once (#682), and `pull` only ever + // adds a block, never migrates an old one away. Checking `profilePath` + // alone would stay green forever while a dead block for this same scope + // sits in, say, `.bashrc` from a pre-#682/#661 install (#693 review). + const home = getUserHome(); + const strayProfiles: string[] = []; + for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { + const candidate = path.join(home, name); + if (candidate === profilePath) continue; + const content = await readFileSafe(candidate); + const strayBlock = content ? extractEnvBlock(content) : null; + if (strayBlock && envBlockReferencesDataHome(strayBlock, envShPath)) { + strayProfiles.push(candidate); + } + } + if (strayProfiles.length > 0) { + problems.push( + `${nameList(strayProfiles)} still carries a teamai env block for this scope from an ` + + 'earlier install; run `teamai uninstall` to remove it, or delete the block manually', + ); + } + return problems; } diff --git a/src/uninstall.ts b/src/uninstall.ts index d94f318d..f5cd6499 100644 --- a/src/uninstall.ts +++ b/src/uninstall.ts @@ -56,7 +56,7 @@ import { getUserHome } from './utils/home.js'; import { detectShellProfile, extractEnvBlock, - envBlockSourcesPath, + envBlockReferencesDataHome, SHELL_PROFILE_CANDIDATE_NAMES, } from './utils/shell-profile.js'; @@ -521,10 +521,14 @@ async function buildRemovalPlan( // the current resolution no longer points at, and a plain uninstall would // silently leave that managed block behind. // - // A candidate only counts if its block actually sources THIS scope's - // env.sh (envBlockSourcesPath) — matching on the marker alone would let - // this uninstall delete a different scope's still-active block just - // because it also happens to live in one of the candidate filenames. + // A candidate only counts if its block actually names THIS scope's + // env.sh (envBlockReferencesDataHome) — matching on the marker alone + // would let this uninstall delete a different scope's still-active block + // just because it also happens to live in one of the candidate + // filenames. This check is deliberately looser than doctor's "does it + // load" check: a legacy block written by a pre-#661/#682 CLI (raw + // backslashes, or the MSYS drive form) still belongs to this scope and + // still has to be found and removed, even though it never worked. const configuredProfilePath = teamConfig.sharing.env.shellProfilePath ? expandHome(teamConfig.sharing.env.shellProfilePath) : await detectShellProfile(); @@ -537,7 +541,7 @@ async function buildRemovalPlan( for (const candidate of candidateProfilePaths) { const profileContent = await readFileSafe(candidate); const block = profileContent ? extractEnvBlock(profileContent) : null; - if (block && envBlockSourcesPath(block, envShPath)) { + if (block && envBlockReferencesDataHome(block, envShPath)) { plan.shellProfiles.push(candidate); } } diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index 57d1aaba..15d5318e 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -108,3 +108,53 @@ export function envBlockSourcesPath(block: string, envShPath: string): boolean { if (!/\s/.test(posixPath)) return true; return block.includes(`"${posixPath}"`) || block.includes(`'${posixPath}'`); } + +/** + * Every on-disk spelling of `envShPath` a teamai block — current or legacy — + * might contain. + * + * A CLI predating a given fix wrote the source path differently: the raw + * OS-native form with unconverted backslashes (pre-#661), or the MSYS/Cygwin + * drive form (`/d/Users/...`, what Git Bash's own `$PWD` shows) from a + * locally-built or hand-patched install. Those blocks are broken — a POSIX + * shell cannot read either form — but they still name this scope's own + * `env.sh`, and a plain string match against only the current format leaves + * them permanently invisible to both `doctor` and `uninstall` (#693 review). + */ +function candidateSpellings(envShPath: string): string[] { + const spellings = new Set([envShPath, envShPath.split(path.sep).join('/')]); + + const drive = /^([A-Za-z]):[\\/](.*)$/.exec(envShPath); + if (drive) { + spellings.add(`/${drive[1].toLowerCase()}/${drive[2].replace(/\\/g, '/')}`); + } + + return [...spellings]; +} + +/** + * Whether a block's `source` line names `envShPath` under any spelling + * teamai has ever written it in — current or legacy, quoted or not, + * forward- or back-slashed, drive- or MSYS-form — regardless of whether that + * spelling actually loads in a shell. + * + * This answers a different question than `envBlockSourcesPath`: "does this + * block belong to this scope" (ownership, for `uninstall` cleanup and for + * `doctor` flagging a stray leftover) rather than "does this block actually + * work" (correctness, for `doctor`'s #661 does-it-load check). A genuinely + * broken legacy block still belongs to this scope and still needs to be + * found and removed — conflating the two would make `doctor` stop reporting + * a real #661-style break just because the path happens to match. + */ +export function envBlockReferencesDataHome(block: string, envShPath: string): boolean { + for (const spelling of candidateSpellings(envShPath)) { + if ( + block.includes(spelling) + || block.includes(shellQuoteValue(spelling)) + || block.includes(`"${spelling}"`) + ) { + return true; + } + } + return false; +} From cd739c4714522c109061b2504de2c140d2e890a4 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 14:33:56 +0530 Subject: [PATCH 6/9] fix(env): expand shellProfilePath before comparing, fix host-dependent test bug (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot review on aea31ef found a real regression: with sharing.env.shellProfilePath: ~/.profile, profilePath stayed the literal unexpanded string while the stray-block scan's candidates are always absolute, so the resolved file never matched itself (candidate === profilePath) and got reported as a stray copy of its own valid block. Expand profilePath once, up front, mirroring the pattern uninstall.ts already used for the same override. Also: CI (ubuntu/macos) caught a real bug in the previous commit that my own Windows-host testing couldn't — candidateSpellings converted backslashes to forward slashes via `envShPath.split(path.sep).join('/')`, which is a no-op on a POSIX runner even for a Windows-shaped input path, since path.sep there is '/'. Moved isWindowsFormPath (shape-based, already used by generateShellBlock) out of resources/env.ts into the shared utils/shell-profile.ts and used it instead, so the conversion is correct regardless of which host runs it — matching the pattern the rest of this file already established for platform-independent logic. Fixed one test that fell into the same trap from the other direction: it fabricated a Windows-style block by replacing `/` with `\` in a POSIX-on-CI envShPath, producing a string with no drive letter at all that doesn't correspond to any real on-disk state. Rewrote it to use an unquoted (not backslashed) legacy form, which is realistic on any host; the Windows-specific legacy spellings are already covered directly in shell-profile.test.ts with explicit Windows-shaped literals. Added a regression test for the ~/.profile-override case and re-verified with a real CLI build: pull + doctor no longer report a tilde-configured profile as carrying a stray copy of its own block. Co-Authored-By: Claude Sonnet 5 --- src/__tests__/doctor-env-delivery.test.ts | 31 ++++++++++++++++++++--- src/doctor-delivery.ts | 11 ++++++-- src/resources/env.ts | 18 ++++--------- src/utils/shell-profile.ts | 27 ++++++++++++++++++-- 4 files changed, 66 insertions(+), 21 deletions(-) diff --git a/src/__tests__/doctor-env-delivery.test.ts b/src/__tests__/doctor-env-delivery.test.ts index 9fe7cd06..30033c21 100644 --- a/src/__tests__/doctor-env-delivery.test.ts +++ b/src/__tests__/doctor-env-delivery.test.ts @@ -132,12 +132,16 @@ describe('doctor — env variables reach a shell', () => { path.join(homeDir, '.profile'), `# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${envShPath} ] && source ${envShPath}\n# [teamai:env:end]\n`, ); - // A pre-#661 legacy block for the SAME env.sh, left behind in .bashrc — - // raw, unquoted, unconverted backslashes. - const windowsEnvSh = envShPath.replace(/\//g, '\\'); + // A legacy block for the SAME env.sh, left behind in .bashrc — raw and + // unquoted (the current generator always quotes via shellQuoteValue, so + // an unquoted block is necessarily from an older write path). Windows- + // specific legacy spellings (backslash, MSYS drive form) are covered + // directly in shell-profile.test.ts's envBlockReferencesDataHome suite, + // with explicit Windows-shaped test data rather than a host-dependent + // string transform of this test's own (POSIX-on-CI) envShPath. await fse.writeFile( path.join(homeDir, '.bashrc'), - `# my bashrc\n# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${windowsEnvSh} ] && source ${windowsEnvSh}\n# [teamai:env:end]\n`, + `# my bashrc\n# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${envShPath} ] && source ${envShPath}\n# [teamai:env:end]\n`, ); const check = await envCheck(); @@ -146,6 +150,25 @@ describe('doctor — env variables reach a shell', () => { expect(check.fix).toContain('teamai uninstall'); }); + // Regression (#693 review round 4): an unexpanded `~/...` override made + // the stray-block scan compare a literal `~/.profile` string against its + // own always-absolute candidate paths, so the resolved file never matched + // itself and got reported as a stray copy of its own valid block. + it('does not report shellProfilePath\'s own file as a stray copy of itself', async () => { + await writeEnvSh("export JIRA_PASSWORD='s3cret'\n"); + teamConfig.sharing.env.shellProfilePath = '~/.profile'; + // The generator always writes the forward-slash form; envShPath is a + // native OS path (backslashes on a Windows dev host), so convert it the + // same way generateShellBlock does before writing this test fixture. + const profileShPosix = envShPath.split(path.sep).join('/'); + await fse.writeFile( + path.join(homeDir, '.profile'), + `# [teamai:env:start]\n# DO NOT EDIT\n[ -f '${profileShPosix}' ] && source '${profileShPosix}'\n# [teamai:env:end]\n`, + ); + + expect(await (await envCheck()).check()).toBe(true); + }); + it('fails and names `variables:` for the shorthand env.yaml form (#662)', async () => { await writeEnvYaml('JIRA_PASSWORD: "s3cret"\n'); await writeEnvSh(''); diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index 9a901dcc..96a98e22 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -571,8 +571,15 @@ async function envDeliveryProblems(ctx: DoctorContext): Promise { } } - // Same resolution the injection runs, not a second copy of it. - const profilePath = teamConfig?.sharing?.env?.shellProfilePath ?? await envHandler.detectShellProfile(); + // Same resolution the injection runs, not a second copy of it. Expanded + // up front (not left to readFileSafe's internal expansion) because the + // stray-block scan below compares this string for identity against + // candidates that are always absolute — an unexpanded `~/...` override + // would never match its own resolved file and get reported as a stray + // copy of itself (#693 review round 4). + const profilePath = expandHome( + teamConfig?.sharing?.env?.shellProfilePath ?? await envHandler.detectShellProfile(), + ); const profile = await readFileSafe(profilePath); const block = profile === null ? null : extractEnvBlock(profile); diff --git a/src/resources/env.ts b/src/resources/env.ts index d13c894f..f0e07a95 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -6,7 +6,11 @@ import type { ResourceItem, TeamaiConfig, LocalConfig } from '../types.js'; import { TEAMAI_ENV_START, TEAMAI_ENV_END, getDataHome, getEnvBackupPath, isSelfMode } from '../types.js'; import { pathExists, readFileSafe, writeFile, ensureDir, fileContentEqual } from '../utils/fs.js'; import { log } from '../utils/logger.js'; -import { detectShellProfile as resolveShellProfilePath, shellQuoteValue } from '../utils/shell-profile.js'; +import { + detectShellProfile as resolveShellProfilePath, + shellQuoteValue, + isWindowsFormPath, +} from '../utils/shell-profile.js'; // ─── Schema for env.yaml ──────────────────────────────── @@ -77,18 +81,6 @@ export function maskEnvValue(value: string): string { return `${value.slice(0, 2)}****`; } -/** - * True for a path a shell must read the Windows way: a drive-letter path - * (`C:\...` or `C:/...`) or a UNC path (`\\server\share`). - * - * A POSIX path is deliberately excluded: there a backslash is an ordinary - * filename character, not a separator, so collapsing every one of them would - * silently point the shell at a different directory. - */ -function isWindowsFormPath(value: string): boolean { - return /^[A-Za-z]:[\\/]/.test(value) || value.startsWith('\\\\'); -} - /** * Read back the assignments `generateEnvFile` writes, as key → value. * diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index 15d5318e..e289ba4a 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -57,6 +57,29 @@ export async function detectShellProfile( return path.join(home, '.bashrc'); } +/** + * True for a path a shell must read the Windows way: a drive-letter path + * (`C:\...` or `C:/...`) or a UNC path (`\\server\share`). + * + * A POSIX path is deliberately excluded: there a backslash is an ordinary + * filename character, not a separator, so collapsing every one of them would + * silently point the shell at a different directory. + * + * Shape-based, not `path.sep`-based: `envShPath` was built by `path.join` on + * whichever host wrote it, and a check that reads the *current* host's + * separator is a no-op for a Windows-shaped path inspected from a POSIX host + * (or vice versa) — the very thing this file's own tests need to exercise, + * since CI only runs ubuntu/macos (#693 review round 4). + */ +export function isWindowsFormPath(value: string): boolean { + return /^[A-Za-z]:[\\/]/.test(value) || value.startsWith('\\\\'); +} + +/** `envShPath` rewritten to the forward-slash form the generator writes, regardless of which host built it. */ +function toGeneratedForm(envShPath: string): string { + return isWindowsFormPath(envShPath) ? envShPath.replace(/\\/g, '/') : envShPath; +} + /** * Quote a string so it is safe to interpolate into a POSIX shell (bash/zsh/sh). * Wraps the value in single quotes and encodes any embedded single quote as @@ -93,7 +116,7 @@ export function extractEnvBlock(profileContent: string): string | null { * against that same rewritten form, not the raw OS path. */ export function envBlockSourcesPath(block: string, envShPath: string): boolean { - const posixPath = envShPath.split(path.sep).join('/'); + const posixPath = toGeneratedForm(envShPath); // The generator (generateShellBlock) always wraps the path in single // quotes via shellQuoteValue, which escapes an embedded apostrophe as @@ -122,7 +145,7 @@ export function envBlockSourcesPath(block: string, envShPath: string): boolean { * them permanently invisible to both `doctor` and `uninstall` (#693 review). */ function candidateSpellings(envShPath: string): string[] { - const spellings = new Set([envShPath, envShPath.split(path.sep).join('/')]); + const spellings = new Set([envShPath, toGeneratedForm(envShPath)]); const drive = /^([A-Za-z]):[\\/](.*)$/.exec(envShPath); if (drive) { From 5f762ac005535d4ba09b4225c8b789bc268251e7 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 14:43:05 +0530 Subject: [PATCH 7/9] fix(doctor): report stale legacy env blocks as their own check (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A valid, working delivery reported as failed just because a stray leftover block sits in another candidate file — buildEnvDeliveryCheck folded "does this scope's env reach a shell" and "is there dead cruft to clean up" into the same problems list, so a user whose delivery genuinely works saw doctor/pull report failure until they ran `teamai uninstall` or edited files by hand. Split envDeliveryProblems to return { problems, staleProfiles }, and buildEnvDeliveryCheck now returns two independent Checks: "Env variables injected in shell profile" (unchanged delivery-correctness semantics) and a new "No stale env blocks left behind" that only fails when a legacy/duplicate block for this scope is found elsewhere, naming `teamai uninstall`. A working delivery now reports healthy regardless of leftover cruft; the cruft is still surfaced, just not as the same failure. Re-verified with a real CLI build reproducing the reviewer's exact scenario: `doctor` now shows "Env variables injected in shell profile" as a pass and "No stale env blocks left behind" as a separate, distinct failure, instead of one conflated failure. Co-Authored-By: Claude Sonnet 5 --- src/__tests__/doctor-env-delivery.test.ts | 31 +++++++++-- src/doctor-delivery.ts | 63 ++++++++++++++--------- 2 files changed, 65 insertions(+), 29 deletions(-) diff --git a/src/__tests__/doctor-env-delivery.test.ts b/src/__tests__/doctor-env-delivery.test.ts index 30033c21..8a8e2883 100644 --- a/src/__tests__/doctor-env-delivery.test.ts +++ b/src/__tests__/doctor-env-delivery.test.ts @@ -60,6 +60,17 @@ describe('doctor — env variables reach a shell', () => { return check; } + // A stray leftover block is cleanup hygiene, not a delivery failure — kept + // as its own Check (#693 review round 5) so a working delivery never + // reports as broken just because a dead file needs cleaning up. + async function staleBlockCheck(): Promise { + const ctx = await resolveDoctorContext(); + if (!ctx) throw new Error('expected a resolved doctor context'); + const check = (await buildChecks(ctx)).find((c) => c.name === 'No stale env blocks left behind'); + if (!check) throw new Error('no stale-block check'); + return check; + } + beforeEach(async () => { tempDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-env-delivery-')); homeDir = path.join(tempDir, 'home'); @@ -128,9 +139,14 @@ describe('doctor — env variables reach a shell', () => { // Force the resolved profile to .profile, bypassing platform-dependent // detectShellProfile() so this test is deterministic on any host. teamConfig.sharing.env.shellProfilePath = path.join(homeDir, '.profile'); + // The generator always writes the forward-slash, quoted form; envShPath + // is a native OS path (backslashes on a Windows dev host), so convert it + // the same way generateShellBlock does — this block must actually load, + // since the point of this test is that delivery stays healthy. + const envShPosix = envShPath.split(path.sep).join('/'); await fse.writeFile( path.join(homeDir, '.profile'), - `# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${envShPath} ] && source ${envShPath}\n# [teamai:env:end]\n`, + `# [teamai:env:start]\n# DO NOT EDIT\n[ -f '${envShPosix}' ] && source '${envShPosix}'\n# [teamai:env:end]\n`, ); // A legacy block for the SAME env.sh, left behind in .bashrc — raw and // unquoted (the current generator always quotes via shellQuoteValue, so @@ -144,10 +160,14 @@ describe('doctor — env variables reach a shell', () => { `# my bashrc\n# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${envShPath} ] && source ${envShPath}\n# [teamai:env:end]\n`, ); - const check = await envCheck(); - expect(await check.check()).toBe(false); - expect(check.fix).toContain('.bashrc'); - expect(check.fix).toContain('teamai uninstall'); + // Delivery itself is healthy — a stray leftover must not report as a + // delivery failure (#693 review round 5). + expect(await (await envCheck()).check()).toBe(true); + + const stale = await staleBlockCheck(); + expect(await stale.check()).toBe(false); + expect(stale.fix).toContain('.bashrc'); + expect(stale.fix).toContain('teamai uninstall'); }); // Regression (#693 review round 4): an unexpanded `~/...` override made @@ -167,6 +187,7 @@ describe('doctor — env variables reach a shell', () => { ); expect(await (await envCheck()).check()).toBe(true); + expect(await (await staleBlockCheck()).check()).toBe(true); }); it('fails and names `variables:` for the shorthand env.yaml form (#662)', async () => { diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index 96a98e22..eecebc7c 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -507,24 +507,45 @@ export async function buildMcpDeliveryChecks(ctx: DoctorContext): Promise { - const problems = await envDeliveryProblems(ctx); - return [{ - name: 'Env variables injected in shell profile', - source: 'local', - check: async () => problems.length === 0, - fix: problems.length === 0 - ? 'Run `teamai pull` to inject env variables into shell profile' - : `${problems.join('; ')}. Run \`teamai pull\` after fixing the cause, then open a new shell.`, - }]; + const { problems, staleProfiles } = await envDeliveryProblems(ctx); + return [ + { + name: 'Env variables injected in shell profile', + source: 'local', + check: async () => problems.length === 0, + fix: problems.length === 0 + ? 'Run `teamai pull` to inject env variables into shell profile' + : `${problems.join('; ')}. Run \`teamai pull\` after fixing the cause, then open a new shell.`, + }, + // A separate check, not folded into the one above: a stray leftover + // block for this same scope (e.g. from before #682 changed which file + // `pull` prefers) is dead weight, not a delivery failure — the variables + // are reaching a shell just fine through the resolved profile. Reporting + // it as the SAME failure as "your env vars aren't reaching a shell" + // would tell a user whose delivery genuinely works that it is broken + // (#693 review round 5). + { + name: 'No stale env blocks left behind', + source: 'local', + check: async () => staleProfiles.length === 0, + fix: staleProfiles.length === 0 + ? undefined + : `${nameList(staleProfiles)} still carries a teamai env block for this scope from an ` + + 'earlier install; run `teamai uninstall` to remove it, or delete the block manually.', + }, + ]; } -/** Every reason the team's env variables are not reaching a shell. */ -async function envDeliveryProblems(ctx: DoctorContext): Promise { +/** Every reason the team's env variables are not reaching a shell, and any stray leftover blocks found along the way. */ +async function envDeliveryProblems( + ctx: DoctorContext, +): Promise<{ problems: string[]; staleProfiles: string[] }> { const { localConfig, teamConfig } = ctx; - if (teamConfig?.sharing?.env?.injectShellProfile === false) return []; + const none = { problems: [], staleProfiles: [] }; + if (teamConfig?.sharing?.env?.injectShellProfile === false) return none; const envYamlPath = path.join(localConfig.repo.localPath, 'env', 'env.yaml'); - if (!await pathExists(envYamlPath)) return []; + if (!await pathExists(envYamlPath)) return none; const { EnvHandler } = await import('./resources/env.js'); const envHandler = new EnvHandler(); @@ -533,13 +554,13 @@ async function envDeliveryProblems(ctx: DoctorContext): Promise { // shorthand `KEY: value` mapping is reported (#662) while a deliberate // `variables: []` is not. Counting the variables alone cannot tell them apart. const read = await envHandler.readEnvYaml(envYamlPath); - if (!read.ok) return [read.reason]; + if (!read.ok) return { problems: [read.reason], staleProfiles: [] }; const declared = read.variables; const problems: string[] = []; // Nothing declared and nothing malformed: there is nothing to deliver. - if (declared.length === 0) return problems; + if (declared.length === 0) return none; // env.sh lives under teamaiHome, which is /.teamai in project // scope and ~/.teamai in user scope — mirror the path that `teamai pull` @@ -598,24 +619,18 @@ async function envDeliveryProblems(ctx: DoctorContext): Promise { // alone would stay green forever while a dead block for this same scope // sits in, say, `.bashrc` from a pre-#682/#661 install (#693 review). const home = getUserHome(); - const strayProfiles: string[] = []; + const staleProfiles: string[] = []; for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { const candidate = path.join(home, name); if (candidate === profilePath) continue; const content = await readFileSafe(candidate); const strayBlock = content ? extractEnvBlock(content) : null; if (strayBlock && envBlockReferencesDataHome(strayBlock, envShPath)) { - strayProfiles.push(candidate); + staleProfiles.push(candidate); } } - if (strayProfiles.length > 0) { - problems.push( - `${nameList(strayProfiles)} still carries a teamai env block for this scope from an ` - + 'earlier install; run `teamai uninstall` to remove it, or delete the block manually', - ); - } - return problems; + return { problems, staleProfiles }; } /** From c2ac32536ebefaaa535b5025c485dcb401814d1b Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 14:57:46 +0530 Subject: [PATCH 8/9] fix(pull): don't fold a stale env block into "check(s) failed" (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the bot's re-review of 5f762ac: - P1: pull.ts counted every failed check the same way, so the new informational "No stale env blocks left behind" check still made a genuinely healthy delivery print "Pull finished, but 1 check(s) failed" — the exact upgrade scenario this PR targets. Check now carries an `informational` flag; pull's post-pull summary excludes it from the failure count and prints it on its own line instead. doctor's own exit code is unaffected, since a stray leftover is still something worth acting on. - P2: the stray-block scan compared `shellProfilePath` against its generated candidate with raw `===`, so a valid override spelled with forward slashes (or, on Windows, different case) reported the block's own file as a stray copy of itself. Added `sameFile()`, resolved through `path.win32`/`path.posix` explicitly (so the win32 branch is actually exercised on ubuntu/macos CI, not just locally). Verified both fixes end-to-end on a real Windows 11 host: built dist/index.js, drove it against a scratch $HOME and a real local git team repo with a legacy .bashrc block plus a working .profile block (forward-slash shellProfilePath) — only the stray .bashrc block is named, no "check(s) failed" banner, and .profile is not misreported as a stray copy of itself. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 4 +- docs/usage-guide.zh-CN.md | 4 +- src/__tests__/doctor-env-delivery.test.ts | 22 ++++++++++ src/__tests__/pull-post-checks.test.ts | 49 +++++++++++++++++++++++ src/__tests__/shell-profile.test.ts | 29 ++++++++++++++ src/doctor-delivery.ts | 4 +- src/doctor.ts | 8 ++++ src/pull.ts | 25 +++++++++--- src/utils/shell-profile.ts | 20 +++++++++ 9 files changed, 155 insertions(+), 10 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 3b70a122..59635b12 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -1528,7 +1528,7 @@ teamai remove mcp teamai remove rules --force # Skip the prompt, for scripts and CI ``` -`teamai doctor` exits with code 0 only when every check passes, and code 1 when any check fails. Before initialization, it reports the missing configuration without assuming a Git provider. The same checks run at the end of a manual `teamai pull`, minus the provider ones and minus any check that pull already reported in its own words on that run. +`teamai doctor` exits with code 0 only when every check passes, and code 1 when any check fails. Before initialization, it reports the missing configuration without assuming a Git provider. The same checks run at the end of a manual `teamai pull`, minus the provider ones and minus any check that pull already reported in its own words on that run. A check marked informational — currently only `No stale env blocks left behind` — still counts toward `doctor`'s exit code, but a pull does not fold its failure into `Pull finished, but N check(s) failed`: a leftover file from an earlier install is cleanup, not a sign this pull broke anything, so it is still named but on its own, gentler line. Besides the provider, clone, config and hook checks, `doctor` verifies what reached your machine. ` is installed` fails when `enabledAgents` lists a tool that nothing would be delivered to, which is the case where a pull reports success and that tool receives nothing. It asks the same resolver the sync uses, so a tool that keeps its skills somewhere other than its tool root, as OpenClaw does with its workspace directory, is judged where the sync would actually write. It reports an installed tool as passing too, so `--json` carries one entry per enabled tool either way. The checks at the end of a pull cover the scope that pull resolved from the current directory; run `teamai doctor` in another scope to check that one. `Skills delivered to ` compares the skills your role namespaces, tag subscriptions and exclusions resolve to against what is on disk for each installed tool: it reports a skill that was never delivered separately from one that arrived unreadable — `SKILL.md` missing, its frontmatter unparseable, or its `name` not matching the directory, which keeps the agent from ever discovering it. `Team docs delivered` compares the docs bundle against `sharing.docs.localDir`, which has one destination rather than one per tool; each expected document has to be a file that can be read, so a directory or a dangling link sitting on the name counts as missing. @@ -1536,7 +1536,7 @@ Besides the provider, clone, config and hook checks, `doctor` verifies what reac Two tools do not read a rules directory, so a per-file check cannot speak for them and each gets one of its own. `Team rules are active in opencode` checks that `opencode.json` still lists the glob the pull owns under `instructions`: OpenCode does not auto-scan `.opencode/rules`, so without it every delivered `.md` is inert while the per-file check keeps passing. `Team rules are inlined in Hermes SOUL.md` compares the teamai-managed block of `SOUL.md` with what the team rules inline to, since Hermes reads standing instructions from that one file rather than from a directory — a deleted block, or one left on an older rule set, is a tool reading the wrong rules with nothing on disk to show for it. -`MCP servers delivered to ` compares each server the team's `mcp.yaml` resolves for that tool against the entry in the tool's own config, and names any the reconcile skipped with its reason. The comparison is the entry, not the name: reconciliation leaves an entry teamai does not own alone, so a server of your own under a team name holds the key while the team's definition never arrives, and a stale copy is just as undelivered. Both are reported as `not the team's definition`, and only `teamai pull --force` replaces an entry teamai did not write. An unresolved `${VAR}` is reported here with the variable's name, which is otherwise said once during a pull and never again. An `mcp.yaml` that does not parse is not a team without MCP: it is reported as `Team MCP servers can be read` with the parse error, since it injects nothing into any tool and every run after the first is silent about it. `Env variables injected in shell profile` no longer stops at finding the marker comment: it checks that `env/env.yaml` parses and declares its variables under the `variables:` key (a plain `KEY: value` mapping parses as none, while an explicit `variables: []` is a configuration with nothing to deliver and fails nothing), that each one reached `env.sh` with the value `env.yaml` declares — a key left over from an older value exports it to every shell and MCP server until the next pull, and the comparison reads `env.sh` back through the generator's own inverse, so a multiline value quoted across several lines is matched rather than called stale — and that the injected block would actually load it — an unquoted Windows path degrades to something a POSIX shell cannot read, so `source` never runs and nothing says so. +`MCP servers delivered to ` compares each server the team's `mcp.yaml` resolves for that tool against the entry in the tool's own config, and names any the reconcile skipped with its reason. The comparison is the entry, not the name: reconciliation leaves an entry teamai does not own alone, so a server of your own under a team name holds the key while the team's definition never arrives, and a stale copy is just as undelivered. Both are reported as `not the team's definition`, and only `teamai pull --force` replaces an entry teamai did not write. An unresolved `${VAR}` is reported here with the variable's name, which is otherwise said once during a pull and never again. An `mcp.yaml` that does not parse is not a team without MCP: it is reported as `Team MCP servers can be read` with the parse error, since it injects nothing into any tool and every run after the first is silent about it. `Env variables injected in shell profile` no longer stops at finding the marker comment: it checks that `env/env.yaml` parses and declares its variables under the `variables:` key (a plain `KEY: value` mapping parses as none, while an explicit `variables: []` is a configuration with nothing to deliver and fails nothing), that each one reached `env.sh` with the value `env.yaml` declares — a key left over from an older value exports it to every shell and MCP server until the next pull, and the comparison reads `env.sh` back through the generator's own inverse, so a multiline value quoted across several lines is matched rather than called stale — and that the injected block would actually load it — an unquoted Windows path degrades to something a POSIX shell cannot read, so `source` never runs and nothing says so. `No stale env blocks left behind` is a separate check: which file `pull` prefers has changed over time (Windows Git Bash's login shell reads `.bash_profile`/`.bash_login`/`.profile`, never `.bashrc`), and a pull only ever adds a block, never migrates an old one away, so a dead block from an earlier install or platform change can sit in another candidate file indefinitely. It names every such file (checking `.zshrc`, `.bashrc`, `.bash_profile`, `.bash_login` and `.profile`, current and legacy spellings alike) and points at `teamai uninstall` to remove them — separately from delivery, so a working env block never reads as broken just because an old one is still lying around. `Contributed learnings are published` fails while `teamai contribute` has notes queued that could not be pushed. A manual `teamai pull` does not repeat it at the end when the pull has already said it: the pull tries to publish the queue and reports the outcome itself, with the push error that made it fail — more than this check can tell you. If the pull never got that far, because the team repo failed to refresh, the check is printed as usual. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index cf592a8b..15f35f89 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -1488,7 +1488,7 @@ teamai remove mcp teamai remove rules --force # 跳过确认，用于脚本和 CI ``` -仅当所有检查通过时，`teamai doctor` 才以状态码 0 退出；任一检查失败时以状态码 1 退出。尚未初始化时，它只报告缺少配置，不会臆测 Git 托管平台。手动执行 `teamai pull` 结束时会运行同一批检查（不含托管平台相关的检查，也不含本次 pull 已经自行报告过的检查）。 +仅当所有检查通过时，`teamai doctor` 才以状态码 0 退出；任一检查失败时以状态码 1 退出。尚未初始化时，它只报告缺少配置，不会臆测 Git 托管平台。手动执行 `teamai pull` 结束时会运行同一批检查（不含托管平台相关的检查，也不含本次 pull 已经自行报告过的检查）。被标记为 informational 的检查——目前只有 `No stale env blocks left behind`——仍会计入 `doctor` 的退出码，但 pull 不会把它的失败并入 `Pull finished, but N check(s) failed`：早期安装留下的遗留文件属于清理事项，不代表这次 pull 弄坏了什么，因此依旧会被点名，只是单独用一行更轻的提示呈现。 除了托管平台、clone、配置和 hook 检查之外，`doctor` 还会验证落到本机上的内容。` is installed` 在 `enabledAgents` 列出了不会收到任何内容的工具时失败——这正是 pull 报告成功、而该工具什么都没收到的情况。它使用与同步相同的解析逻辑，因此像 OpenClaw 这样把 skills 放在 workspace 目录而非工具根目录的工具，会在同步真正写入的位置被判断。工具已安装时也会作为通过项报告，因此 `--json` 无论哪种情况都会为每个已启用工具给出一条记录。pull 结束时的检查只覆盖它从当前目录解析出的那个 scope；其他 scope 请在对应目录下运行 `teamai doctor`。`Skills delivered to ` 会把角色命名空间、标签订阅与排除规则解析出的 skill 集合，与每个已安装工具磁盘上的内容比对：从未送达的 skill 与送达但不可读的 skill 会分别报告——后者指 `SKILL.md` 缺失、frontmatter 无法解析，或其 `name` 与目录名不一致，导致 agent 永远发现不了它。`Team docs delivered` 将 docs 包与 `sharing.docs.localDir` 比对（它只有一个目标目录，而非每个工具一个）；每个应有的文档都必须是可读取的文件，因此占用了该名字的目录或断链接也算缺失。 @@ -1496,7 +1496,7 @@ teamai remove rules --force # 跳过确认，用于脚本和 CI 有两个工具并不读取 rules 目录，按文件比对的检查无法代表它们，因此各自单列一项。`Team rules are active in opencode` 检查 `opencode.json` 的 `instructions` 中是否仍列着 teamai 所拥有的那条 glob：OpenCode 不会自动扫描 `.opencode/rules`，缺了它，已送达的每个 `.md` 都不会生效，而按文件比对的检查依旧通过。`Team rules are inlined in Hermes SOUL.md` 把 `SOUL.md` 中 teamai 管理的代码块与团队 rule 内联后的内容比对——Hermes 的常驻指令来自这一个文件而非某个目录，因此代码块被删除或停留在旧版规则集上，都意味着该工具读到的是错误的规则，而磁盘上看不出任何异常。 -`MCP servers delivered to ` 将团队 `mcp.yaml` 为该工具解析出的每个 server 与该工具自己配置文件中的条目逐一比对，并列出 reconcile 跳过的 server 及原因。比对的是条目内容而非名字：reconcile 不会覆盖不属于 teamai 的条目，因此你自己写的同名 server 会占住这个名字，团队的定义从未真正送达；过期的旧副本同样等于没送达。两者都报告为 `not the team's definition`，而覆盖非 teamai 写入的条目只有 `teamai pull --force` 能做到。未解析的 `${VAR}` 会在这里连同变量名一起报告——否则它只在 pull 时出现一次，之后再无提示。无法解析的 `mcp.yaml` 并不等于团队没有 MCP：它会作为 `Team MCP servers can be read` 连同解析错误一起报告，因为这种文件不会向任何工具注入内容，而且除第一次之外的每次运行都对此保持沉默。`Env variables injected in shell profile` 不再只查标记注释：它会检查 `env/env.yaml` 能否解析、以及是否在 `variables:` 键下声明了变量（写成普通的 `KEY: value` 映射等于没有声明；而显式写成 `variables: []` 属于没有内容要下发的配置，不会判为失败）、每个变量是否以 `env.yaml` 声明的值写进了 `env.sh`（残留的旧值会一直被导出到每个 shell 和 MCP server，直到下次 pull；比对时会用生成器自身的逆运算读回 `env.sh`，因此跨多行引用的多行值能够正确匹配，而不会被误判为过期），以及注入的代码块是否真的能加载它——未加引号的 Windows 路径在 POSIX shell 中会被转义破坏，`source` 从不执行，而且没有任何提示。 +`MCP servers delivered to ` 将团队 `mcp.yaml` 为该工具解析出的每个 server 与该工具自己配置文件中的条目逐一比对，并列出 reconcile 跳过的 server 及原因。比对的是条目内容而非名字：reconcile 不会覆盖不属于 teamai 的条目，因此你自己写的同名 server 会占住这个名字，团队的定义从未真正送达；过期的旧副本同样等于没送达。两者都报告为 `not the team's definition`，而覆盖非 teamai 写入的条目只有 `teamai pull --force` 能做到。未解析的 `${VAR}` 会在这里连同变量名一起报告——否则它只在 pull 时出现一次，之后再无提示。无法解析的 `mcp.yaml` 并不等于团队没有 MCP：它会作为 `Team MCP servers can be read` 连同解析错误一起报告，因为这种文件不会向任何工具注入内容，而且除第一次之外的每次运行都对此保持沉默。`Env variables injected in shell profile` 不再只查标记注释：它会检查 `env/env.yaml` 能否解析、以及是否在 `variables:` 键下声明了变量（写成普通的 `KEY: value` 映射等于没有声明；而显式写成 `variables: []` 属于没有内容要下发的配置，不会判为失败）、每个变量是否以 `env.yaml` 声明的值写进了 `env.sh`（残留的旧值会一直被导出到每个 shell 和 MCP server，直到下次 pull；比对时会用生成器自身的逆运算读回 `env.sh`，因此跨多行引用的多行值能够正确匹配，而不会被误判为过期），以及注入的代码块是否真的能加载它——未加引号的 Windows 路径在 POSIX shell 中会被转义破坏，`source` 从不执行，而且没有任何提示。`No stale env blocks left behind` 是独立的一项检查：pull 优先选用哪个文件会随时间变化（Windows 上 Git Bash 的登录 shell 读取的是 `.bash_profile`/`.bash_login`/`.profile`，从不读取 `.bashrc`），而 pull 只会新增代码块，从不迁移旧的，因此早期安装或平台变化留下的失效代码块可能一直留在另一个候选文件里。它会列出每一个这样的文件（检查 `.zshrc`、`.bashrc`、`.bash_profile`、`.bash_login` 和 `.profile`，新旧写法都算），并指向 `teamai uninstall` 来清除它们——这与投递检查分开进行，因此不会因为还留着一个旧副本，就让一个正常工作的 env 代码块被判成故障。 `Contributed learnings are published` 会在 `teamai contribute` 写下、但尚未推送成功的笔记仍在队列中时失败。当本次 pull 已经说过时，手动 `teamai pull` 结束时不会再重复它：pull 会尝试发布队列并自行报告结果，还会带上导致失败的推送错误——这是该检查本身给不出的信息。如果 pull 因为团队仓库刷新失败而根本没走到那一步，该检查会照常打印。 diff --git a/src/__tests__/doctor-env-delivery.test.ts b/src/__tests__/doctor-env-delivery.test.ts index 8a8e2883..c9c1b179 100644 --- a/src/__tests__/doctor-env-delivery.test.ts +++ b/src/__tests__/doctor-env-delivery.test.ts @@ -190,6 +190,28 @@ describe('doctor — env variables reach a shell', () => { expect(await (await staleBlockCheck()).check()).toBe(true); }); + // Regression (#693 review round 6): `shellProfilePath` is user-supplied and + // may use forward slashes (or, on Windows, different case) even though the + // stray-block scan's own candidate is built with `path.join`, which uses + // the host's native separator. A raw string comparison between the two + // told the check its own resolved file was a stray copy of itself whenever + // the two spellings of the same path did not match byte-for-byte. + it('does not report shellProfilePath as a stray copy of itself when its spelling differs by separator', async () => { + await writeEnvSh("export JIRA_PASSWORD='s3cret'\n"); + // path.join(homeDir, '.profile') is native-separated; this override names + // the same file with forward slashes throughout, which on a POSIX host is + // already identical and on Windows is the exact shape the review reported. + teamConfig.sharing.env.shellProfilePath = path.join(homeDir, '.profile').split(path.sep).join('/'); + const profileShPosix = envShPath.split(path.sep).join('/'); + await fse.writeFile( + path.join(homeDir, '.profile'), + `# [teamai:env:start]\n# DO NOT EDIT\n[ -f '${profileShPosix}' ] && source '${profileShPosix}'\n# [teamai:env:end]\n`, + ); + + expect(await (await envCheck()).check()).toBe(true); + expect(await (await staleBlockCheck()).check()).toBe(true); + }); + it('fails and names `variables:` for the shorthand env.yaml form (#662)', async () => { await writeEnvYaml('JIRA_PASSWORD: "s3cret"\n'); await writeEnvSh(''); diff --git a/src/__tests__/pull-post-checks.test.ts b/src/__tests__/pull-post-checks.test.ts index 78a00b23..8423148c 100644 --- a/src/__tests__/pull-post-checks.test.ts +++ b/src/__tests__/pull-post-checks.test.ts @@ -285,6 +285,55 @@ describe('checks at the end of an interactive pull', () => { expect(buildChecks).not.toHaveBeenCalled(); }); + it('does not count an informational failure toward "check(s) failed", but still reports it', async () => { + // A stray legacy env block is real leftover state worth mentioning, but + // it is not a sign that anything this pull just delivered is broken — + // the "N check(s) failed" banner must not fire on its account alone + // (#693 review round 6). + vi.mocked(buildChecks).mockResolvedValue([ + { name: 'Team repo exists locally', source: 'local', check: async () => true }, + { + name: 'No stale env blocks left behind', + source: 'local', + informational: true, + check: async () => false, + fix: '~/.bashrc still carries a teamai env block for this scope from an earlier install', + }, + ]); + + await pull({ force: true }); + + const output = printedOutput(); + expect(output).not.toContain('check(s) failed'); + expect(output).toContain('No stale env blocks left behind'); + expect(output).toContain('~/.bashrc still carries a teamai env block'); + }); + + it('counts a blocking failure but not an informational one alongside it', async () => { + vi.mocked(buildChecks).mockResolvedValue([ + { + name: 'teamai hooks in claude settings', + source: 'local', + check: async () => false, + fix: 'Run `teamai hooks inject` to inject/update hooks', + }, + { + name: 'No stale env blocks left behind', + source: 'local', + informational: true, + check: async () => false, + fix: 'stray block', + }, + ]); + + await pull({ force: true }); + + const output = printedOutput(); + expect(output).toContain('Pull finished, but 1 check(s) failed'); + expect(output).toContain('teamai hooks in claude settings'); + expect(output).toContain('No stale env blocks left behind'); + }); + it('a check that throws does not fail the pull', async () => { const failing: Check = { name: 'explodes', diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 85cdd16c..86e30fa6 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -6,6 +6,7 @@ import { detectShellProfile, envBlockSourcesPath, envBlockReferencesDataHome, + sameFile, shellQuoteValue, } from '../utils/shell-profile.js'; @@ -150,3 +151,31 @@ describe('envBlockReferencesDataHome', () => { expect(envBlockReferencesDataHome(block, envShPath)).toBe(false); }); }); + +// Regression (#693 review round 6): the stray-block scan compares a +// user-supplied `shellProfilePath` override against a `path.join`-built +// candidate. A raw `===` made a valid override a false positive "stray copy +// of itself" whenever the two spellings of the same path did not match +// byte-for-byte — an override with forward slashes, or (Windows only) a +// different case. +describe('sameFile', () => { + it('matches a forward-slash override against a backslash candidate on win32', () => { + expect(sameFile('C:/Users/me/.profile', 'C:\\Users\\me\\.profile', 'win32')).toBe(true); + }); + + it('matches regardless of case on win32', () => { + expect(sameFile('C:\\Users\\Me\\.profile', 'c:\\users\\me\\.profile', 'win32')).toBe(true); + }); + + it('does not match a genuinely different file on win32', () => { + expect(sameFile('C:\\Users\\me\\.profile', 'C:\\Users\\me\\.bashrc', 'win32')).toBe(false); + }); + + it('is case-sensitive on posix, where a case difference is a different file', () => { + expect(sameFile('/home/me/.profile', '/home/me/.PROFILE', 'linux')).toBe(false); + }); + + it('matches identical posix paths', () => { + expect(sameFile('/home/me/.profile', '/home/me/.profile', 'linux')).toBe(true); + }); +}); diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index eecebc7c..961d89fe 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -11,6 +11,7 @@ import { extractEnvBlock, envBlockSourcesPath, envBlockReferencesDataHome, + sameFile, SHELL_PROFILE_CANDIDATE_NAMES, } from './utils/shell-profile.js'; import { getUserHome } from './utils/home.js'; @@ -527,6 +528,7 @@ export async function buildEnvDeliveryCheck(ctx: DoctorContext): Promise staleProfiles.length === 0, fix: staleProfiles.length === 0 ? undefined @@ -622,7 +624,7 @@ async function envDeliveryProblems( const staleProfiles: string[] = []; for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { const candidate = path.join(home, name); - if (candidate === profilePath) continue; + if (sameFile(candidate, profilePath)) continue; const content = await readFileSafe(candidate); const strayBlock = content ? extractEnvBlock(content) : null; if (strayBlock && envBlockReferencesDataHome(strayBlock, envShPath)) { diff --git a/src/doctor.ts b/src/doctor.ts index 231fe9c4..b15cc243 100644 --- a/src/doctor.ts +++ b/src/doctor.ts @@ -47,6 +47,14 @@ export interface Check { * only voice left and must be heard. */ reportedByPull?: string; + /** + * True for a check whose failure is a cleanup opportunity, not a sign that + * anything a user asked for is actually broken. `doctor` still reports it + * like any other check; `pull`'s post-pull summary excludes it from the + * "N check(s) failed" count so a healthy delivery is not announced as + * broken because of unrelated leftover state (#693 review round 6). + */ + informational?: boolean; check: () => Promise; fix?: string; } diff --git a/src/pull.ts b/src/pull.ts index 98f8cdac..3629f376 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -2017,12 +2017,12 @@ async function reportPostPullChecks( // registry is built, which is where the I/O actually is. The `'pull'` // stage leaves out the two that would spend it — rules read every file per // tool, agents parse every spec — so the cheap ones still get to run. - const results = await withTimeout( + const { local, results } = await withTimeout( (async () => { const local = (await buildChecks(ctx, 'pull')) .filter((c) => c.source === 'local') .filter((c) => !c.reportedByPull || !reported.has(c.reportedByPull)); - return runChecks(local); + return { local, results: await runChecks(local) }; })(), POST_PULL_CHECKS_TIMEOUT_MS, `Post-pull checks are still running after ${POST_PULL_CHECKS_TIMEOUT_MS}ms`, @@ -2031,10 +2031,25 @@ async function reportPostPullChecks( const failures = results.filter((r) => !r.ok); if (failures.length === 0) return; - log.warn(`Pull finished, but ${failures.length} check(s) failed:`); - for (const failure of failures) { + // A check marked `informational` (e.g. a stale leftover file) is a + // cleanup opportunity, not a sign the pull that just ran did anything + // wrong — it must not turn a genuinely healthy delivery into "Pull + // finished, but N check(s) failed" (#693 review round 6). + const informationalNames = new Set(local.filter((c) => c.informational).map((c) => c.name)); + const blocking = failures.filter((f) => !informationalNames.has(f.name)); + const informational = failures.filter((f) => informationalNames.has(f.name)); + + if (blocking.length > 0) { + log.warn(`Pull finished, but ${blocking.length} check(s) failed:`); + for (const failure of blocking) { + const [headline, ...detail] = formatCheckResult(failure); + log.warn(headline); + for (const line of detail) log.dim(line); + } + } + for (const failure of informational) { const [headline, ...detail] = formatCheckResult(failure); - log.warn(headline); + log.dim(headline); for (const line of detail) log.dim(line); } log.dim(' Run `teamai doctor` for the full report.'); diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index e289ba4a..230c3f15 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -80,6 +80,26 @@ function toGeneratedForm(envShPath: string): string { return isWindowsFormPath(envShPath) ? envShPath.replace(/\\/g, '/') : envShPath; } +/** + * Whether two paths name the same file on disk, independent of separator + * style or (on Windows) case. + * + * Resolved with `path.win32`/`path.posix` explicitly rather than the ambient + * `path` — both normalize `/` and `\` to one separator either way, but only + * an explicit choice lets a test exercise the win32 branch on ubuntu/macos CI + * (same reason `platform` is injectable elsewhere in this file). An override + * written as `C:/Users/me/.profile` then compares equal to the generated + * candidate `path.join(home, '.profile')`, which is backslash-separated on + * win32. Windows filesystems are case-insensitive, so a case difference alone + * must not make two paths look distinct there either (#693 review round 6). + */ +export function sameFile(a: string, b: string, platform: NodeJS.Platform = process.platform): boolean { + const resolve = platform === 'win32' ? path.win32.resolve : path.posix.resolve; + const left = resolve(a); + const right = resolve(b); + return platform === 'win32' ? left.toLowerCase() === right.toLowerCase() : left === right; +} + /** * Quote a string so it is safe to interpolate into a POSIX shell (bash/zsh/sh). * Wraps the value in single quotes and encodes any embedded single quote as From 02c93fe59a708f9f386d136e37cbc297bf6248db Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 15:14:00 +0530 Subject: [PATCH 9/9] fix(env): stick to whichever profile already owns this scope's block (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bot's round-7 review found a real flip-flop: Git for Windows' /etc/profile.d/bash_profile.sh auto-generates ~/.bash_profile (a plain file sourcing ~/.bashrc, not a symlink) the first time a login shell starts with .bashrc present but none of .bash_profile/ .bash_login/.profile. That satisfies exactly the condition a first pull leaves behind (.bashrc written, nothing else exists yet), so the very next login shell creates .bash_profile out from under it. detectShellProfile()'s order then prefers that newly-existing file on the *next* pull, injecting a second block there and reporting the original .bashrc block — still loading, just one hop further away — as a dead leftover. Added resolveActiveShellProfile() in shell-profile.ts: before falling back to the order-based detectShellProfile(), it checks whether any candidate already carries a block for this scope's env.sh and, if so, reuses it. Both the write path (EnvHandler.pullItem, via detectShellProfile) and the read path (doctor-delivery's envDeliveryProblems) now resolve through it, so pull and doctor keep agreeing on the same file. (The bot's second finding described the fix as "resolve real paths / compare file identity" — .bash_profile isn't a symlink or hardlink to .bashrc, it's a regular file with a `source ~/.bashrc` line in it, so realpath() wouldn't touch this at all. Confirmed the exact generated content directly from a local Git for Windows install (etc/profile.d/bash_profile.sh) before designing the fix around that.) Verified end-to-end on a real Windows host: two `teamai pull` runs against a real local git team repo, with the exact Git-for-Windows forwarding .bash_profile content dropped in between (not written by teamai) — second pull updates .bashrc in place, no duplicate block, `teamai doctor` reports both env checks clean. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 2 + docs/usage-guide.zh-CN.md | 2 + src/__tests__/env-handler.test.ts | 35 +++++++++++++++ src/__tests__/shell-profile.test.ts | 68 +++++++++++++++++++++++++++++ src/doctor-delivery.ts | 2 +- src/resources/env.ts | 12 ++--- src/utils/shell-profile.ts | 34 ++++++++++++++- 7 files changed, 148 insertions(+), 7 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 59635b12..a76f5cb7 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -745,6 +745,8 @@ teamai push On `pull`, when `injectShellProfile` is enabled (default), the env block goes into `~/.zshrc` if `$SHELL` is zsh, otherwise `~/.bashrc` — except on Windows: `$SHELL` is normally unset there, and Git Bash starts as a *login* shell that never reads `.bashrc`, so teamai instead prefers an existing `~/.bash_profile`, then `~/.bash_login`, then `~/.profile`, falling back to `~/.bashrc` only when none of them exist (a zsh installed via MSYS2/Cygwin, which does set `$SHELL`, still resolves to `.zshrc`). This matches Git for Windows' own fallback in `/etc/profile.d/bash_profile.sh`, whose guard is `[ -e ~/.bashrc -a ! -e ~/.bash_profile -a ! -e ~/.bash_login -a ! -e ~/.profile ]` — it only synthesizes a `.bash_profile` that sources `.bashrc` in that same one case, which is why a stray `~/.profile` (even one that just sources something else, e.g. `~/.local/bin/env`) is enough to make `.bashrc` alone go unread. Override the target file with `sharing.env.shellProfilePath` in `teamai.yaml`. +This preference order only decides where a *first* pull writes. Every pull after that sticks to whichever candidate already carries this scope's block, rather than re-running the order — otherwise Git for Windows' own bootstrap would move the target out from under it: the same `/etc/profile.d/bash_profile.sh` guard above also means that first pull satisfies its condition (`.bashrc` now exists, nothing else does yet), so the next Git Bash login shell auto-generates a `~/.bash_profile` that sources it. Without sticking to `.bashrc`, the next pull would prefer that newly-created file and inject a second block there, leaving the original — still working, just loaded one hop further away — reported as a dead leftover. + `doctor` (and the check `pull` runs automatically afterward) also flags a teamai env block left behind in a *different* candidate file — e.g. a block a pre-#682 install wrote to `.bashrc` before this file-selection logic changed — even if that block is broken and was never functional. `teamai uninstall` removes it. ### Docs diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 15f35f89..388acf3d 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -718,6 +718,8 @@ teamai push `pull` 时，若启用了 `injectShellProfile`（默认启用），`$SHELL` 为 zsh 时环境变量块会写入 `~/.zshrc`，否则写入 `~/.bashrc`——但 Windows 上例外：`$SHELL` 通常未设置，而 Git Bash 以*登录 shell*方式启动，从不读取 `.bashrc`，因此 teamai 会优先选择已存在的 `~/.bash_profile`、其次 `~/.bash_login`、再次 `~/.profile`，只有三者都不存在时才回退到 `~/.bashrc`（通过 MSYS2/Cygwin 安装、会设置 `$SHELL` 的 zsh 仍会解析到 `.zshrc`）。这与 Git for Windows 自身在 `/etc/profile.d/bash_profile.sh` 中的回退逻辑一致，其判断条件是 `[ -e ~/.bashrc -a ! -e ~/.bash_profile -a ! -e ~/.bash_login -a ! -e ~/.profile ]`——只有在这一种情况下它才会生成一个会 source `.bashrc` 的 `.bash_profile`；这也是为什么哪怕一个只 source 了其他内容（例如 `~/.local/bin/env`）的 `~/.profile` 存在，也足以让 `.bashrc` 单独失效。可通过 `teamai.yaml` 中的 `sharing.env.shellProfilePath` 覆盖目标文件。 +这个优先级顺序只决定*第一次* pull 写到哪里。此后的每次 pull 都会沿用已经承载着本作用域代码块的那个候选文件，而不会重新走一遍优先级判断——否则 Git for Windows 自身的引导逻辑会把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 之后同样会成立（`.bashrc` 已存在，其余候选文件都还不存在），于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`。如果不沿用 `.bashrc`，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是多绕了一跳——则会被误报为失效的遗留代码块。 + `doctor`（以及 `pull` 结束后自动运行的检查）还会标记出遗留在*其他*候选文件中的 teamai 环境变量块——例如 #682 之前的旧版本写入 `.bashrc` 的代码块，即便该代码块本身已损坏、从未生效。`teamai uninstall` 会清理它。 ### Docs（文档） diff --git a/src/__tests__/env-handler.test.ts b/src/__tests__/env-handler.test.ts index f7d5850b..8c8a7d28 100644 --- a/src/__tests__/env-handler.test.ts +++ b/src/__tests__/env-handler.test.ts @@ -434,6 +434,41 @@ scope: 'user', expect(content).toContain(TEAMAI_ENV_START); }); + // Regression (#693 review round 7): Git for Windows' own + // /etc/profile.d/bash_profile.sh auto-generates ~/.bash_profile (a plain + // forwarding file, not a symlink) the first time a login shell starts + // with ~/.bashrc present but none of the other candidates. Without + // sticking to wherever the block already lives, the next pull would + // prefer that newly-existing .bash_profile and inject a second, separate + // block there instead of updating the one already in .bashrc. + it('keeps updating .bashrc in place after Git for Windows auto-generates a forwarding .bash_profile', async () => { + vi.stubEnv('SHELL', ''); + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + const bashrcPath = path.join(homeDir, '.bashrc'); + const bashProfilePath = path.join(homeDir, '.bash_profile'); + + // First pull: nothing exists yet, falls back to .bashrc. + await handler.pullItem(item, teamConfig, localConfig); + expect(await fse.pathExists(bashrcPath)).toBe(true); + expect(await fse.pathExists(bashProfilePath)).toBe(false); + + // Git for Windows generates the forwarding .bash_profile on its own, + // between the two pulls — teamai never wrote this file. + await fse.writeFile( + bashProfilePath, + '# generated by Git for Windows\ntest -f ~/.profile && . ~/.profile\ntest -f ~/.bashrc && . ~/.bashrc\n', + ); + + await handler.pullItem(item, teamConfig, localConfig); + + const bashrcContent = await fse.readFile(bashrcPath, 'utf-8'); + expect(bashrcContent).toContain(TEAMAI_ENV_START); + expect(bashrcContent.split(TEAMAI_ENV_START).length).toBe(2); + + const bashProfileContent = await fse.readFile(bashProfilePath, 'utf-8'); + expect(bashProfileContent).not.toContain(TEAMAI_ENV_START); + }); + it('should skip shell injection when injectShellProfile is false', async () => { const noInjectConfig: TeamaiConfig = { ...teamConfig, diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 86e30fa6..ce80e7a2 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -6,6 +6,7 @@ import { detectShellProfile, envBlockSourcesPath, envBlockReferencesDataHome, + resolveActiveShellProfile, sameFile, shellQuoteValue, } from '../utils/shell-profile.js'; @@ -88,6 +89,73 @@ describe('detectShellProfile', () => { }); }); +// Regression (#693 review round 7): Git for Windows' own +// /etc/profile.d/bash_profile.sh auto-generates ~/.bash_profile the first +// time a login shell starts with ~/.bashrc present but none of +// ~/.bash_profile, ~/.bash_login or ~/.profile — a plain file containing +// `test -f ~/.bashrc && . ~/.bashrc`, not a symlink. detectShellProfile's +// order then prefers that newly-existing file on the next pull, so the +// resolver must stick to wherever this scope's block already lives instead +// of re-running the order-based fallback every time. +describe('resolveActiveShellProfile', () => { + let tmpDir: string; + let homeDir: string; + let envShPath: string; + + beforeEach(async () => { + tmpDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-active-shell-profile-test-')); + homeDir = path.join(tmpDir, 'home'); + await fse.ensureDir(homeDir); + vi.stubEnv('HOME', homeDir); + vi.stubEnv('SHELL', ''); + envShPath = path.join(homeDir, '.teamai', 'env.sh'); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + await fse.remove(tmpDir); + }); + + function teamaiBlock(): string { + const posix = envShPath.split(path.sep).join('/'); + return `# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${shellQuoteValue(posix)} ] && source ${shellQuoteValue(posix)}\n# [teamai:env:end]\n`; + } + + it('sticks to .bashrc even after Git for Windows auto-generates a forwarding .bash_profile', async () => { + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + // The exact content Git for Windows' bash_profile.sh generates — a plain + // forwarding file, never a symlink, and carries no teamai markers. + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '# generated by Git for Windows\ntest -f ~/.profile && . ~/.profile\ntest -f ~/.bashrc && . ~/.bashrc\n', + ); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('sticks to a lower-priority candidate over a higher-priority one that exists but carries no block', async () => { + // Order-based detection would prefer .bash_profile over .profile; the + // sticky block living in .profile must still win. + await fse.writeFile(path.join(homeDir, '.bash_profile'), 'unrelated content\n'); + await fse.writeFile(path.join(homeDir, '.profile'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); + }); + + it('falls back to order-based detection when no candidate owns a block yet (first pull)', async () => { + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('does not stick to a different scope\'s block; falls back to order-based detection', async () => { + const otherEnvSh = path.join(homeDir, 'other-project', '.teamai', 'env.sh'); + const otherPosix = otherEnvSh.split(path.sep).join('/'); + await fse.writeFile( + path.join(homeDir, '.bashrc'), + `# [teamai:env:start]\n# DO NOT EDIT\n[ -f ${shellQuoteValue(otherPosix)} ] && source ${shellQuoteValue(otherPosix)}\n# [teamai:env:end]\n`, + ); + await fse.writeFile(path.join(homeDir, '.profile'), ''); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); + }); +}); + describe('envBlockSourcesPath', () => { it('matches a plain path in the generator\'s single-quoted form', () => { const envShPath = '/home/user/.teamai/env.sh'; diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index 961d89fe..204f2b04 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -601,7 +601,7 @@ async function envDeliveryProblems( // would never match its own resolved file and get reported as a stray // copy of itself (#693 review round 4). const profilePath = expandHome( - teamConfig?.sharing?.env?.shellProfilePath ?? await envHandler.detectShellProfile(), + teamConfig?.sharing?.env?.shellProfilePath ?? await envHandler.detectShellProfile(envShPath), ); const profile = await readFileSafe(profilePath); const block = profile === null ? null : extractEnvBlock(profile); diff --git a/src/resources/env.ts b/src/resources/env.ts index f0e07a95..507e9a9c 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -7,7 +7,7 @@ import { TEAMAI_ENV_START, TEAMAI_ENV_END, getDataHome, getEnvBackupPath, isSelf import { pathExists, readFileSafe, writeFile, ensureDir, fileContentEqual } from '../utils/fs.js'; import { log } from '../utils/logger.js'; import { - detectShellProfile as resolveShellProfilePath, + resolveActiveShellProfile, shellQuoteValue, isWindowsFormPath, } from '../utils/shell-profile.js'; @@ -262,7 +262,7 @@ export class EnvHandler extends ResourceHandler { if (inject) { const profilePath = teamConfig.sharing.env.shellProfilePath ? teamConfig.sharing.env.shellProfilePath - : await this.detectShellProfile(); + : await this.detectShellProfile(path.join(teamaiHome, 'env.sh')); const shellBlock = this.generateShellBlock(teamaiHome); await this.injectShellProfile(profilePath, shellBlock); @@ -414,10 +414,12 @@ export class EnvHandler extends ResourceHandler { * a second spelling of this choice would check `.bashrc` while the pull * wrote `.zshrc`, and report a correct install as broken. Delegates to the * shared `utils/shell-profile.js` so `teamai uninstall` resolves the same - * file too (#682). + * file too (#682), and stays on whichever candidate already carries this + * scope's block rather than re-deriving it from scratch every pull (#693 + * review round 7). */ - detectShellProfile(platform: NodeJS.Platform = process.platform): Promise { - return resolveShellProfilePath(platform); + detectShellProfile(envShPath: string, platform: NodeJS.Platform = process.platform): Promise { + return resolveActiveShellProfile(envShPath, platform); } /** diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index 230c3f15..b602df3f 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -1,5 +1,5 @@ import path from 'node:path'; -import { pathExists } from './fs.js'; +import { pathExists, readFileSafe } from './fs.js'; import { getUserHome } from './home.js'; import { TEAMAI_ENV_START, TEAMAI_ENV_END } from '../types.js'; @@ -201,3 +201,35 @@ export function envBlockReferencesDataHome(block: string, envShPath: string): bo } return false; } + +/** + * Resolve which shell profile file this scope's env block belongs in. + * + * Sticky by design: a candidate that already carries a block for this + * scope's `env.sh` is reused, rather than re-running `detectShellProfile`'s + * order-based fallback on every pull. Without this, Git for Windows' own + * `/etc/profile.d/bash_profile.sh` changes which candidate *exists* between + * two pulls out from under it: the first time a login shell starts with + * `~/.bashrc` present but none of `~/.bash_profile`, `~/.bash_login` or + * `~/.profile`, it auto-generates a `~/.bash_profile` that sources both — + * not a symlink, a plain file containing `test -f ~/.bashrc && . ~/.bashrc`. + * `detectShellProfile`'s order then prefers that newly-existing file on the + * *next* pull, injecting a second block there and reporting the still-loading + * `.bashrc` one (loaded transitively through the generated forwarder) as a + * stray leftover, even though nothing ever stopped working (#693 review + * round 7). Only when no candidate already owns a block — a genuinely first + * pull — does the order-based fallback decide. + */ +export async function resolveActiveShellProfile( + envShPath: string, + platform: NodeJS.Platform = process.platform, +): Promise { + const home = getUserHome(); + for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { + const candidate = path.join(home, name); + const content = await readFileSafe(candidate); + const block = content ? extractEnvBlock(content) : null; + if (block && envBlockReferencesDataHome(block, envShPath)) return candidate; + } + return detectShellProfile(platform); +}