From cc12cab0e3c8941990dfcf91e78f7f2650ebf17e Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Mon, 21 Sep 2026 15:39:40 +0530 Subject: [PATCH 1/9] fix(env): only stick to a candidate the active profile actually reaches (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 02c93fe's resolveActiveShellProfile scanned every SHELL_PROFILE_CANDIDATE_NAMES entry for a matching block and returned the first hit, in a fixed order (.zshrc, .bashrc, .bash_profile, .bash_login, .profile). That's broader than the Git-for-Windows-forwarding case it was written for: a stale block a pre-#682 install left in .bashrc would outrank a correctly order-picked .profile that hasn't been written to yet, since .bashrc sorts earlier in the candidate list — silently reintroducing #682 for exactly the installs upgrading through this fix, with doctor unable to catch it because the stale block is well-formed where it sits. Reworked to start from detectShellProfile's order-based pick (the file the current environment actually reads) and only diverge from it when that pick's own content references another candidate by a home-relative path (~/.bashrc, $HOME/.bashrc) — the shape Git for Windows' generated forwarding file actually takes. A block sitting in a candidate the pick never reaches is no longer preferred over the pick, regardless of what it contains. Also caught and fixed a case of exactly the failure mode this PR is about: the first cut of the forwarding check was a bare substring match on the candidate's filename, and my own test's plain-English comment ("...unrelated to .bashrc") satisfied it. Tightened to require the home-relative reference form a real sourcing line uses. Verified both scenarios end-to-end on a real Windows host: - The exact bot-reported upgrade case (stale .bashrc block, empty .profile, no forwarding between them): pull now writes into .profile and correctly flags .bashrc as stale; doctor reports delivery healthy. - The Git-for-Windows forwarding case from the prior round: still sticks to .bashrc through the generated .bash_profile, no duplicate, no stale-block warning. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- src/__tests__/shell-profile.test.ts | 26 ++++++++++-- src/utils/shell-profile.ts | 65 ++++++++++++++++++++--------- 4 files changed, 69 insertions(+), 26 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index b62bd87b..1b45dbec 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -745,7 +745,7 @@ 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. +Every pull re-runs this order to find the file the current environment actually reads, then only diverges from that pick in one narrow case: if the picked file carries no block of its own but its own content names another candidate (a home-relative reference like `~/.bashrc`), and that candidate does carry the block, the pull stays there instead of duplicating it. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it — that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without recognizing that forwarding relationship the next pull would prefer the 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. A block sitting in a candidate the current pick does *not* itself reference is never preferred over the pick, however — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. `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. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index aee69483..9bf1654e 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -718,7 +718,7 @@ 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 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是多绕了一跳——则会被误报为失效的遗留代码块。 +每次 pull 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件；只有一种情况会偏离这个结果：被选中的文件自己没有代码块，但它的内容里提到了另一个候选文件（形如 `~/.bashrc` 这种以家目录为基准的引用），且那个候选文件确实带着代码块——这时 pull 会留在那个候选文件，而不是重复注入。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不识别这种转发关系，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是多绕了一跳——则会被误报为失效的遗留代码块。但反过来，当前选中的文件并未引用到的某个候选文件，即便它本身带着代码块，也绝不会因此被优先选中——否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 `doctor`（以及 `pull` 结束后自动运行的检查）还会标记出遗留在*其他*候选文件中的 teamai 环境变量块——例如 #682 之前的旧版本写入 `.bashrc` 的代码块，即便该代码块本身已损坏、从未生效。`teamai uninstall` 会清理它。 diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index ce80e7a2..0af90e23 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -132,12 +132,30 @@ describe('resolveActiveShellProfile', () => { 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. + // Regression (#693 review round 8): the original version of this resolver + // scanned every candidate for a matching block regardless of whether the + // order-based pick could ever reach it, so a stale pre-#682 block sitting + // in `.bashrc` outranked a genuinely unwritten, currently-read `.profile` + // — silently reintroducing #682 for exactly the installs this PR fixes, + // with `doctor` unable to catch it since the stale block is well-formed + // where it sits. The order-based pick's own content must name a candidate + // before that candidate's block is ever preferred over it. + it('does not stick to a stale block in a candidate the order-based pick never reads (#682 upgrade case)', async () => { + // The exact #682 repro: .bashrc present, .bash_profile/.bash_login absent, + // .profile present — order-based detection reads .profile, never .bashrc, + // and .profile does not itself source .bashrc. + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + await fse.writeFile(path.join(homeDir, '.profile'), '# just a profile, unrelated to .bashrc\n'); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); + }); + + it('prefers the order-based pick over an unrelated candidate that merely carries a block', async () => { + // .bash_profile exists (order-based winner) but has content unrelated to + // any other candidate; a block sitting in .profile must not be preferred + // just because it exists somewhere in the candidate list. 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')); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); it('falls back to order-based detection when no candidate owns a block yet (first pull)', async () => { diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index b602df3f..d83816ea 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -205,31 +205,56 @@ export function envBlockReferencesDataHome(block: string, envShPath: string): bo /** * 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. + * Starts from `detectShellProfile`'s order-based pick — the file the current + * environment actually reads — and only diverges from it in two cases, both + * narrower than "any candidate with a block wins" (#693 review round 8: that + * broader rule let a stale pre-#682 block in `.bashrc` outrank a genuinely + * unwritten, currently-read `.profile`, silently reintroducing #682 for + * exactly the installs upgrading through this fix, with `doctor` no longer + * able to catch it since the stale block is well-formed where it sits): + * + * 1. The order-based pick already carries this scope's block — the common + * steady state, unchanged from before. + * 2. The order-based pick carries no block of its own, but its own content + * names another candidate that does — e.g. Git for Windows' + * `/etc/profile.d/bash_profile.sh` auto-generates `~/.bash_profile` + * (`test -f ~/.bashrc && . ~/.bashrc`, a plain file, not a symlink) the + * first time a login shell starts with `~/.bashrc` present but none of + * `~/.bash_profile`, `~/.bash_login` or `~/.profile`. `detectShellProfile` + * then prefers that newly-existing file on the *next* pull; injecting a + * second block there would leave the still-loading `.bashrc` one (loaded + * transitively through the generated forwarder) reported as a stray + * leftover, even though nothing ever stopped working. + * + * A candidate the order-based pick does not itself read is never preferred, + * regardless of what it contains. */ 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; + const activePick = await detectShellProfile(platform); + + const activeContent = await readFileSafe(activePick); + const activeBlock = activeContent ? extractEnvBlock(activeContent) : null; + if (activeBlock && envBlockReferencesDataHome(activeBlock, envShPath)) return activePick; + + if (activeContent) { + for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { + const candidate = path.join(home, name); + // A home-relative reference (`~/.bashrc`, `$HOME/.bashrc`), the shape a + // sourcing line actually takes — not a bare substring match, which a + // plain English comment mentioning the filename would also satisfy. + const referencesCandidate = activeContent.includes(`~/${name}`) + || activeContent.includes(`$HOME/${name}`) + || activeContent.includes('${HOME}/' + name); + if (candidate === activePick || !referencesCandidate) continue; + const content = await readFileSafe(candidate); + const block = content ? extractEnvBlock(content) : null; + if (block && envBlockReferencesDataHome(block, envShPath)) return candidate; + } } - return detectShellProfile(platform); + + return activePick; } From 75f3eac35dc8fa55c680b444ce439c2d4d714a5f Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 23 Sep 2026 09:22:53 +0530 Subject: [PATCH 2/9] fix(env): match real source commands, resolve reachability transitively (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P1s from the bot's review of #715: - resolveActiveShellProfile's reachability check was a bare substring search on the active pick's content. A comment mentioning a filename (never executed) or a longer file sharing the same prefix (~/.bashrc.local) would both satisfy it, letting a stale block win the same way #682 did. Replaced with referencesCandidate(): strips full-line comments, splits each remaining line into statements on &&/||/;, and only counts a statement whose first word is literally `.` or `source` and whose second word is an anchored home-relative reference to exactly that candidate. - The check only followed one hop: .bash_profile sourcing .profile sourcing .bashrc (the common Debian .profile pattern, sourcing .bashrc for interactive shells) would miss a block two hops away and inject a duplicate. Reworked into a loop that walks the chain of files the pick actually sources, with a visited set for cycle protection, stopping at the first one that carries the block. Also fixed the P2: EnvHandler.detectShellProfile's doc comment still claimed it "stays on whichever candidate already carries this scope's block" unconditionally, which stopped being true once reachability was required. Verified end-to-end on a real Windows host: - The new two-hop chain (.bash_profile -> .profile -> .bashrc, block in .bashrc): resolves to .bashrc, no duplicate, doctor fully clean. - Re-ran the Git-for-Windows one-hop scenario and the #682 upgrade scenario from the prior round — both still correct, no regression. Co-Authored-By: Claude Sonnet 5 --- src/__tests__/shell-profile.test.ts | 39 +++++++++++ src/resources/env.ts | 7 +- src/utils/shell-profile.ts | 100 ++++++++++++++++++---------- 3 files changed, 106 insertions(+), 40 deletions(-) diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 0af90e23..3c6a1118 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -172,6 +172,45 @@ describe('resolveActiveShellProfile', () => { await fse.writeFile(path.join(homeDir, '.profile'), ''); expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); }); + + // Regression (#693 review round 9): a bare substring search matched a + // comment mentioning the filename (never executed) and a different, + // longer-named file sharing the same prefix. + it('does not stick to a candidate merely mentioned in a comment', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '# source ~/.bashrc\nunrelated content\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not stick to a different, longer-named file sharing the same prefix', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'source ~/.bashrc.local\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + // Regression (#693 review round 9): the resolver only followed one hop of + // sourcing, so a chain like .bash_profile -> .profile -> .bashrc (the + // common Debian .profile pattern, sourcing .bashrc for interactive + // shells) missed a block two hops away and would have injected a + // duplicate into .bash_profile instead of reusing .bashrc. + it('follows a two-hop sourcing chain to reach a block (.bash_profile -> .profile -> .bashrc)', async () => { + await fse.writeFile(path.join(homeDir, '.bash_profile'), '. ~/.profile\n'); + await fse.writeFile(path.join(homeDir, '.profile'), '[ -f ~/.bashrc ] && . ~/.bashrc\n'); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('does not hang on a reference cycle and falls back to the order-based pick', async () => { + await fse.writeFile(path.join(homeDir, '.bash_profile'), 'source ~/.profile\n'); + await fse.writeFile(path.join(homeDir, '.profile'), 'source ~/.bash_profile\n'); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); }); describe('envBlockSourcesPath', () => { diff --git a/src/resources/env.ts b/src/resources/env.ts index 507e9a9c..73a29090 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -414,9 +414,10 @@ 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), and stays on whichever candidate already carries this - * scope's block rather than re-deriving it from scratch every pull (#693 - * review round 7). + * file too (#682), and follows the chain of files the order-based pick + * actually `source`s to reuse a candidate that already carries this + * scope's block, rather than injecting a duplicate every time a new file + * enters that chain (#693 review rounds 7-9). */ 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 d83816ea..60d8bb2d 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -202,32 +202,61 @@ export function envBlockReferencesDataHome(block: string, envShPath: string): bo return false; } +/** + * Whether `content` runs a `source`/`.` command on a home-relative reference + * to `name` (`~/.bashrc`, `$HOME/.bashrc`, `${HOME}/.bashrc`) — the shape a + * real forwarding line takes, e.g. Git for Windows' generated + * `test -f ~/.bashrc && . ~/.bashrc`. + * + * Deliberately narrower than a substring search (#693 review round 9): that + * matched a comment mentioning the filename (inert, never executed) and a + * same-prefixed but different file (`~/.bashrc.local` contains `~/.bashrc` + * as a substring). Comment lines are dropped outright; each remaining line + * is split on `&&`/`||`/`;` into statements, and a statement only counts + * when its first word is literally `.` or `source` and its second word is + * exactly the home-relative reference — anchored, so a longer filename + * cannot satisfy it by prefix. + */ +function referencesCandidate(content: string, name: string): boolean { + const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const target = new RegExp(`^["']?(?:~|\\$\\{?HOME\\}?)/${escaped}(?![\\w.-])["']?$`); + for (const rawLine of content.split('\n')) { + if (rawLine.trimStart().startsWith('#')) continue; + for (const statement of rawLine.split(/&&|\|\||;/)) { + const tokens = statement.trim().split(/\s+/); + if (tokens.length >= 2 && (tokens[0] === '.' || tokens[0] === 'source') && target.test(tokens[1])) { + return true; + } + } + } + return false; +} + /** * Resolve which shell profile file this scope's env block belongs in. * * Starts from `detectShellProfile`'s order-based pick — the file the current - * environment actually reads — and only diverges from it in two cases, both - * narrower than "any candidate with a block wins" (#693 review round 8: that - * broader rule let a stale pre-#682 block in `.bashrc` outrank a genuinely - * unwritten, currently-read `.profile`, silently reintroducing #682 for - * exactly the installs upgrading through this fix, with `doctor` no longer - * able to catch it since the stale block is well-formed where it sits): + * environment actually reads — and follows the chain of files it actually + * `source`s (transitively, with cycle protection) looking for one that + * already carries this scope's block. A candidate the chain never reaches is + * never preferred, regardless of what it contains: an earlier version + * matched any candidate with a block anywhere (#693 review round 8: a stale + * pre-#682 block in `.bashrc` then outranked a genuinely unwritten, + * currently-read `.profile`, reintroducing #682 for exactly the installs + * upgrading through this fix) and checked only one hop of sourcing (#693 + * review round 9: `.bash_profile` sourcing `.profile` sourcing `.bashrc` — + * the common Debian `.profile` pattern — would miss a block sitting in + * `.bashrc` two hops away and inject a duplicate into `.bash_profile`). * - * 1. The order-based pick already carries this scope's block — the common - * steady state, unchanged from before. - * 2. The order-based pick carries no block of its own, but its own content - * names another candidate that does — e.g. Git for Windows' - * `/etc/profile.d/bash_profile.sh` auto-generates `~/.bash_profile` - * (`test -f ~/.bashrc && . ~/.bashrc`, a plain file, not a symlink) the - * first time a login shell starts with `~/.bashrc` present but none of - * `~/.bash_profile`, `~/.bash_login` or `~/.profile`. `detectShellProfile` - * then prefers that newly-existing file on the *next* pull; injecting a - * second block there would leave the still-loading `.bashrc` one (loaded - * transitively through the generated forwarder) reported as a stray - * leftover, even though nothing ever stopped working. - * - * A candidate the order-based pick does not itself read is never preferred, - * regardless of what it contains. + * The common real case this exists for: Git for Windows' + * `/etc/profile.d/bash_profile.sh` auto-generates `~/.bash_profile` + * (`test -f ~/.bashrc && . ~/.bashrc`, a plain file, not a symlink) the + * first time a login shell starts with `~/.bashrc` present but none of + * `~/.bash_profile`, `~/.bash_login` or `~/.profile`. `detectShellProfile` + * then prefers that newly-existing file on the *next* pull; without + * following the chain it opens, injecting a second block there would leave + * the still-loading `.bashrc` one reported as a stray leftover, even though + * nothing ever stopped working. */ export async function resolveActiveShellProfile( envShPath: string, @@ -236,24 +265,21 @@ export async function resolveActiveShellProfile( const home = getUserHome(); const activePick = await detectShellProfile(platform); - const activeContent = await readFileSafe(activePick); - const activeBlock = activeContent ? extractEnvBlock(activeContent) : null; - if (activeBlock && envBlockReferencesDataHome(activeBlock, envShPath)) return activePick; + const visited = new Set(); + let current = activePick; + while (!visited.has(current)) { + visited.add(current); + const content = await readFileSafe(current); + const block = content ? extractEnvBlock(content) : null; + if (block && envBlockReferencesDataHome(block, envShPath)) return current; + if (!content) break; - if (activeContent) { - for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { + const next = SHELL_PROFILE_CANDIDATE_NAMES.find((name) => { const candidate = path.join(home, name); - // A home-relative reference (`~/.bashrc`, `$HOME/.bashrc`), the shape a - // sourcing line actually takes — not a bare substring match, which a - // plain English comment mentioning the filename would also satisfy. - const referencesCandidate = activeContent.includes(`~/${name}`) - || activeContent.includes(`$HOME/${name}`) - || activeContent.includes('${HOME}/' + name); - if (candidate === activePick || !referencesCandidate) continue; - const content = await readFileSafe(candidate); - const block = content ? extractEnvBlock(content) : null; - if (block && envBlockReferencesDataHome(block, envShPath)) return candidate; - } + return candidate !== current && !visited.has(candidate) && referencesCandidate(content, name); + }); + if (!next) break; + current = path.join(home, next); } return activePick; From aaa1142729712f88bcf33bc2a8cc69774b66ad4f Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 23 Sep 2026 09:31:39 +0530 Subject: [PATCH 3/9] fix(env): search every referenced candidate, respect || conditionality (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more P1s from the bot's round-10 review of 75f3eac: - The traversal committed to the first referenced candidate in SHELL_PROFILE_CANDIDATE_NAMES's fixed priority order and gave up if that branch was a dead end, instead of trying every candidate the current file actually references. Git for Windows' own generated .bash_profile sources both .bashrc and .profile in one file — if the real block sits in .profile but .bashrc (sorting earlier) has none, the walk stopped at .bashrc without ever trying .profile. Reworked into a breadth-first search over the whole reference graph. - Splitting statements on `||` treated its right side as unconditionally reached, but `||`'s right side only runs if the left side fails, which isn't something this code can establish. `source ~/.profile || source ~/.bashrc` would mark .bashrc reachable even when .profile succeeds. Statements no longer split on `||`; a `source`/`.` sitting only after it is folded into its left side's statement and never recognized as its own reference, so it's never preferred over a block the left side already reaches. Conservative by construction: worst case is falling back to the order-based pick (the pre-#693-fix behavior), never a false "reachable". Verified the exact branching scenario end-to-end on a real Windows host: .bash_profile with the literal Git-for-Windows-generated content (sources both .bashrc and .profile), .bashrc empty, real block in .profile — resolves to .profile, no duplicate, doctor fully clean. Co-Authored-By: Claude Sonnet 5 --- src/__tests__/shell-profile.test.ts | 37 +++++++++++++++++++ src/utils/shell-profile.ts | 55 ++++++++++++++++++----------- 2 files changed, 72 insertions(+), 20 deletions(-) diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 3c6a1118..6de10549 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -211,6 +211,43 @@ describe('resolveActiveShellProfile', () => { await fse.writeFile(path.join(homeDir, '.profile'), 'source ~/.bash_profile\n'); expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); + + // Regression (#693 review round 10): the resolver committed to the first + // referenced candidate in SHELL_PROFILE_CANDIDATE_NAMES's fixed order and + // gave up if that branch was a dead end, instead of trying every candidate + // the active pick actually references. .bash_profile sourcing both + // .bashrc and .profile is exactly Git for Windows' own generated content + // — .bashrc sorts earlier in the candidate list, so a dead .bashrc branch + // would previously stop the search before it ever reached .profile. + it('tries every referenced candidate, not just the first in priority order', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'test -f ~/.bashrc && . ~/.bashrc\ntest -f ~/.profile && . ~/.profile\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), '# no block here\n'); + await fse.writeFile(path.join(homeDir, '.profile'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); + }); + + // Regression (#693 review round 10): splitting on `||` treated its + // right-hand side as unconditionally reached, but it only runs if the left + // side fails — undetermined here. A stale block behind `||` must not win + // over a working one the left side already reaches. + it('does not treat the right side of || as reachable', async () => { + // Both .profile and .bashrc carry a valid block for this scope; the + // point is which one the resolver *reaches* through the || line, not + // which one has a well-formed block. .bashrc sorts earlier than + // .profile in SHELL_PROFILE_CANDIDATE_NAMES, so a naive "any referenced + // candidate in priority order" search would wrongly land on .bashrc even + // though it only runs if the left side (.profile) fails. + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'source ~/.profile || source ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.profile'), teamaiBlock()); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); + }); }); describe('envBlockSourcesPath', () => { diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index 60d8bb2d..53ec751b 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -222,7 +222,14 @@ function referencesCandidate(content: string, name: string): boolean { const target = new RegExp(`^["']?(?:~|\\$\\{?HOME\\}?)/${escaped}(?![\\w.-])["']?$`); for (const rawLine of content.split('\n')) { if (rawLine.trimStart().startsWith('#')) continue; - for (const statement of rawLine.split(/&&|\|\||;/)) { + // Only `&&`/`;` split into statements that are still unconditionally + // attempted (or gated on the referenced candidate's own existence, which + // is independently re-checked by reading that candidate). `||`'s right + // side runs only if its left side fails — something not established + // here — so it is left folded into the same statement as its left side: + // that statement's first `.`/`source` command (the unconditional one) + // still matches, but a `source` sitting only after `||` never does. + for (const statement of rawLine.split(/&&|;/)) { const tokens = statement.trim().split(/\s+/); if (tokens.length >= 2 && (tokens[0] === '.' || tokens[0] === 'source') && target.test(tokens[1])) { return true; @@ -236,17 +243,22 @@ function referencesCandidate(content: string, name: string): boolean { * Resolve which shell profile file this scope's env block belongs in. * * Starts from `detectShellProfile`'s order-based pick — the file the current - * environment actually reads — and follows the chain of files it actually - * `source`s (transitively, with cycle protection) looking for one that - * already carries this scope's block. A candidate the chain never reaches is - * never preferred, regardless of what it contains: an earlier version - * matched any candidate with a block anywhere (#693 review round 8: a stale - * pre-#682 block in `.bashrc` then outranked a genuinely unwritten, - * currently-read `.profile`, reintroducing #682 for exactly the installs - * upgrading through this fix) and checked only one hop of sourcing (#693 - * review round 9: `.bash_profile` sourcing `.profile` sourcing `.bashrc` — - * the common Debian `.profile` pattern — would miss a block sitting in - * `.bashrc` two hops away and inject a duplicate into `.bash_profile`). + * environment actually reads — and searches every file it actually `source`s + * (transitively, breadth-first, with cycle protection) for one that already + * carries this scope's block. A candidate the search never reaches is never + * preferred, regardless of what it contains: earlier versions matched any + * candidate with a block anywhere (#693 review round 8: a stale pre-#682 + * block in `.bashrc` then outranked a genuinely unwritten, currently-read + * `.profile`, reintroducing #682 for exactly the installs upgrading through + * this fix), checked only one hop of sourcing (#693 review round 9: + * `.bash_profile` sourcing `.profile` sourcing `.bashrc` — the common Debian + * `.profile` pattern — would miss a block sitting in `.bashrc` two hops away + * and inject a duplicate into `.bash_profile`), and followed only the first + * referenced candidate in a fixed priority order rather than every one + * (#693 review round 10: `.bash_profile` sourcing both `.bashrc` and + * `.profile`, with the block actually sitting in `.profile`, would commit to + * the dead-end `.bashrc` branch first — earlier in `SHELL_PROFILE_CANDIDATE_ + * NAMES` — and give up without ever trying `.profile`). * * The common real case this exists for: Git for Windows' * `/etc/profile.d/bash_profile.sh` auto-generates `~/.bash_profile` @@ -266,20 +278,23 @@ export async function resolveActiveShellProfile( const activePick = await detectShellProfile(platform); const visited = new Set(); - let current = activePick; - while (!visited.has(current)) { + const queue: string[] = [activePick]; + while (queue.length > 0) { + const current = queue.shift() as string; + if (visited.has(current)) continue; visited.add(current); + const content = await readFileSafe(current); const block = content ? extractEnvBlock(content) : null; if (block && envBlockReferencesDataHome(block, envShPath)) return current; - if (!content) break; + if (!content) continue; - const next = SHELL_PROFILE_CANDIDATE_NAMES.find((name) => { + for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { const candidate = path.join(home, name); - return candidate !== current && !visited.has(candidate) && referencesCandidate(content, name); - }); - if (!next) break; - current = path.join(home, next); + if (candidate !== current && !visited.has(candidate) && referencesCandidate(content, name)) { + queue.push(candidate); + } + } } return activePick; From 60a2da0252f29040b34d03c1c60d5346ad79a237 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 23 Sep 2026 09:46:33 +0530 Subject: [PATCH 4/9] fix(env): only trust verifiable && and || conditions, ignore if bodies (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more P1s from the bot's round-11 review of aaa1142, both about referencesCandidate() trusting shell control flow it can't actually evaluate: - Any `&&` was treated as making its right side reachable, without checking what the left side's condition even was. A guard like `[ "$TERM_PROGRAM" = vscode ] && source ~/.bashrc` would mark .bashrc reachable unconditionally, even though it only runs inside VS Code. Also flagged: a source sitting inside a multiline `if` body looks, line by line, identical to a top-level one. - Folding `||`'s right side into its left statement (the round-9 fix) went too conservative the other way: `source ~/.profile || source ~/.bashrc` DOES guarantee .bashrc runs when ~/.profile doesn't exist, and the resolver was never even trying it. Rather than growing another ad hoc regex tweak, rewrote referencesCandidate() around what it can actually verify without a real shell parser: - Unconditional: a bare `. REF` / `source REF` — but nothing inside an `if` block counts, conditional or not. An `if`'s condition is opaque to a line scanner; trusting some conditions and not others would just be guessing. - Existence-gated `&&`: only the self-referential idiom `test -f REF && . REF` / `[ -f REF ] && . REF`, where the tested path and the sourced path are the same candidate — the one `&&` condition this code can independently verify, by visiting that candidate itself later in the search. - `||` fallback: the left side always counts (always attempted); the right side counts only when the left side's own target file does not exist on disk — the one case an `||` fallback is actually guaranteed to run. Anything this can't resolve either way is never trusted: the search just doesn't queue that candidate, and the caller falls back to the order-based pick — at worst a harmless duplicate block (the pre-#693-fix behavior), never a false "reachable" that would reintroduce #682. Verified end-to-end on a real Windows host: re-ran the core Git-for-Windows two-pull scenario from #693 (self-referential &&, still recognized) with no regression. Added 3 unit tests for the new boundaries: || recognized when the left target is missing, a non-existence && condition rejected, and a source nested inside an if block rejected. Co-Authored-By: Claude Sonnet 5 --- src/__tests__/shell-profile.test.ts | 43 ++++++++++++ src/utils/shell-profile.ts | 100 ++++++++++++++++++++-------- 2 files changed, 115 insertions(+), 28 deletions(-) diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 6de10549..224358ac 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -248,6 +248,49 @@ describe('resolveActiveShellProfile', () => { await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); }); + + // Regression (#693 review round 11): the right side of `||` genuinely is + // guaranteed to run when the left side's own target file does not exist — + // the one case this scanner can verify without a real shell. Failing to + // recognize it falls back to injecting a duplicate, which round 10's fix + // was meant to avoid for exactly this shape of line. + it('does treat the right side of || as reachable when the left side\'s target is missing', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'source ~/.profile || source ~/.bashrc\n', + ); + // .profile is deliberately absent. + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + // Regression (#693 review round 11): `&&` only establishes reachability + // when this scanner can independently verify the guarding condition — the + // self-referential existence test. A condition testing anything else + // (here, an environment variable) is not verifiable, so a stale block + // behind it must not outrank a genuinely unwritten, currently-read file. + it('does not treat a non-existence && condition as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '[ "$TERM_PROGRAM" = vscode ] && source ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + // Regression (#693 review round 11): a source line's own text looks + // identical whether it sits at top level or three lines inside an `if` + // block this scanner cannot evaluate. Nothing inside an `if` is trusted, + // conditional or not, so a block only reachable through one is not + // preferred over the order-based pick. + it('does not treat a source nested inside an if block as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'if [ -n "$BASH_VERSION" ]; then\n . ~/.bashrc\nfi\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); }); describe('envBlockSourcesPath', () => { diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index 53ec751b..93b2bd2a 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -202,38 +202,82 @@ export function envBlockReferencesDataHome(block: string, envShPath: string): bo return false; } +/** `~/name`, `$HOME/name` or `${HOME}/name`, optionally quoted, as a token this scanner accepts as a reference to `name`. */ +function homeRelativeRef(name: string): string { + const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + return `["']?(?:~|\\$\\{?HOME\\}?)/${escaped}(?![\\w.-])["']?`; +} + +/** If `token` is a home-relative reference (`~/name`, `$HOME/name`, `${HOME}/name`), the real path it names. */ +function homeRelativePath(token: string, home: string): string | null { + const stripped = token.replace(/^["']/, '').replace(/["']$/, ''); + const m = stripped.match(/^(?:~|\$\{?HOME\}?)\/(.+)$/); + return m ? path.join(home, m[1]) : null; +} + /** - * Whether `content` runs a `source`/`.` command on a home-relative reference - * to `name` (`~/.bashrc`, `$HOME/.bashrc`, `${HOME}/.bashrc`) — the shape a - * real forwarding line takes, e.g. Git for Windows' generated - * `test -f ~/.bashrc && . ~/.bashrc`. + * Whether `content` runs a `source`/`.` command reaching `name` + * (`~/.bashrc`, `$HOME/.bashrc`, `${HOME}/.bashrc`), restricted to the forms + * this scanner can reason about without a real shell parser (#693 review + * round 11 named two more gaps a bare substring/`&&`/`;` split left open): + * + * - **Unconditional**: a bare `. REF` / `source REF`, as its own `;`-joined + * statement. Nothing inside an `if` block counts, conditional or not — + * the condition is opaque to a line scanner, so a source sitting three + * lines under `if [ -n "$BASH_VERSION" ]; then` is no more verifiable + * than one under `if [ "$TERM_PROGRAM" = vscode ]; then`, and trusting + * either would risk the same false "reachable" #682 regression the + * sticky resolver exists to prevent. + * - **Existence-gated**: `test -f REF && . REF` / `[ -f REF ] && . REF`, + * self-referential only — the tested path and the sourced path must both + * be `name`, the one condition this scanner can independently verify (by + * visiting that candidate itself later in the search). A condition + * testing anything else grants nothing. + * - **`||` fallback**: `A || B`, where `A` is a source of some other + * candidate. The left side of `||` is always attempted, so it counts + * unconditionally; the right side runs only if the left one fails, which + * is verifiable in exactly one case — `A`'s own target does not exist on + * disk — so `B` counts only then. * - * Deliberately narrower than a substring search (#693 review round 9): that - * matched a comment mentioning the filename (inert, never executed) and a - * same-prefixed but different file (`~/.bashrc.local` contains `~/.bashrc` - * as a substring). Comment lines are dropped outright; each remaining line - * is split on `&&`/`||`/`;` into statements, and a statement only counts - * when its first word is literally `.` or `source` and its second word is - * exactly the home-relative reference — anchored, so a longer filename - * cannot satisfy it by prefix. + * A file, or a shell construct, this cannot resolve one way or the other is + * never trusted either way: the caller falls back to the order-based pick, + * which is safe (at worst a harmless duplicate block, the pre-#693-fix + * behavior) — never a false "reachable" that would silently reintroduce + * #682. */ -function referencesCandidate(content: string, name: string): boolean { - const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const target = new RegExp(`^["']?(?:~|\\$\\{?HOME\\}?)/${escaped}(?![\\w.-])["']?$`); +async function referencesCandidate(content: string, name: string, home: string): Promise { + const ref = homeRelativeRef(name); + const refOnly = new RegExp(`^${ref}$`); + const existenceGated = new RegExp( + `^(?:test\\s+-f\\s+${ref}|\\[\\s+-f\\s+${ref}\\s*\\])\\s*&&\\s*(?:\\.|source)\\s+${ref}$`, + ); + const sourceOf = /^(?:\.|source)\s+(\S+)$/; + + let ifDepth = 0; for (const rawLine of content.split('\n')) { - if (rawLine.trimStart().startsWith('#')) continue; - // Only `&&`/`;` split into statements that are still unconditionally - // attempted (or gated on the referenced candidate's own existence, which - // is independently re-checked by reading that candidate). `||`'s right - // side runs only if its left side fails — something not established - // here — so it is left folded into the same statement as its left side: - // that statement's first `.`/`source` command (the unconditional one) - // still matches, but a `source` sitting only after `||` never does. - for (const statement of rawLine.split(/&&|;/)) { - const tokens = statement.trim().split(/\s+/); - if (tokens.length >= 2 && (tokens[0] === '.' || tokens[0] === 'source') && target.test(tokens[1])) { - return true; + const line = rawLine.trim(); + if (!line || line.startsWith('#')) continue; + if (/^if\b/.test(line)) { ifDepth += 1; continue; } + if (/^fi\b/.test(line)) { ifDepth = Math.max(0, ifDepth - 1); continue; } + if (ifDepth > 0) continue; + + for (const statement of line.split(';').map((s) => s.trim()).filter(Boolean)) { + if (existenceGated.test(statement)) return true; + + const orParts = statement.split('||').map((s) => s.trim()); + if (orParts.length === 2) { + const left = orParts[0].match(sourceOf); + const right = orParts[1].match(sourceOf); + if (left && refOnly.test(left[1])) return true; + if (left && right && refOnly.test(right[1])) { + const leftPath = homeRelativePath(left[1], home); + if (leftPath && !(await pathExists(leftPath))) return true; + } + continue; } + + const plain = statement.match(sourceOf); + if (plain && refOnly.test(plain[1])) return true; } } return false; @@ -291,7 +335,7 @@ export async function resolveActiveShellProfile( for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { const candidate = path.join(home, name); - if (candidate !== current && !visited.has(candidate) && referencesCandidate(content, name)) { + if (candidate !== current && !visited.has(candidate) && await referencesCandidate(content, name, home)) { queue.push(candidate); } } From 158df65632366eb84cb91ea7235f955609be54a5 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 23 Sep 2026 09:47:47 +0530 Subject: [PATCH 5/9] docs(env): document transitive chaining and the verifiable-reference boundary (review) Round-11 P2: both docs described only a single directly-referenced candidate, but the resolver has followed transitive chains since 75f3eac and now only trusts specific verifiable && / || forms (60a2da0). Describes the Debian .profile -> .bashrc two-hop case alongside the Git-for-Windows one, and names the three reference shapes recognized (bare source, self-referential existence-gated &&, existence-checked || fallback) and that if-bodies are never trusted. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 4 +++- docs/usage-guide.zh-CN.md | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 1b45dbec..7c34ae26 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -745,7 +745,9 @@ 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`. -Every pull re-runs this order to find the file the current environment actually reads, then only diverges from that pick in one narrow case: if the picked file carries no block of its own but its own content names another candidate (a home-relative reference like `~/.bashrc`), and that candidate does carry the block, the pull stays there instead of duplicating it. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it — that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without recognizing that forwarding relationship the next pull would prefer the 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. A block sitting in a candidate the current pick does *not* itself reference is never preferred over the pick, however — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. +Every pull re-runs this order to find the file the current environment actually reads, then follows whatever that file `source`s — transitively, through as many hops as it takes — looking for a candidate that already carries the block, rather than duplicating it. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it: that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without following that forwarding relationship the next pull would prefer the newly-created file and inject a second block there, leaving the original — still working, just loaded further away — reported as a dead leftover. The same reasoning covers a plain `.profile` that sources `.bashrc` for interactive shells (the standard Debian/Ubuntu template), two hops from whatever a login shell reads first. + +Only a reference this can verify without running a real shell counts, though: a bare `source`/`.` line (as long as it isn't sitting inside an `if` block, whose condition is opaque to a line scanner either way), a `test -f X && . X` / `[ -f X ] && . X` guard where the tested and sourced path are the same file, or the left side of an `A || B` fallback (always attempted) — plus the right side, but only when `A`'s own target does not exist on disk, the one case that fallback is guaranteed to run. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. `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. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 9bf1654e..a512b105 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -718,7 +718,9 @@ 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 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件；只有一种情况会偏离这个结果：被选中的文件自己没有代码块，但它的内容里提到了另一个候选文件（形如 `~/.bashrc` 这种以家目录为基准的引用），且那个候选文件确实带着代码块——这时 pull 会留在那个候选文件，而不是重复注入。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不识别这种转发关系，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是多绕了一跳——则会被误报为失效的遗留代码块。但反过来，当前选中的文件并未引用到的某个候选文件，即便它本身带着代码块，也绝不会因此被优先选中——否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 +每次 pull 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件，然后沿着它 `source` 的内容一路查下去——无论要经过多少跳——寻找一个已经带着代码块的候选文件，而不是重复注入。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不沿着这条转发链去找，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是绕得更远了——则会被误报为失效的遗留代码块。同样的道理也适用于一个普通的 `.profile`（为交互式 shell source `.bashrc`，也就是 Debian/Ubuntu 的标准模板）：这时登录 shell 最先读到的文件，离实际代码块有两跳之遥。 + +不过，只有这段扫描逻辑能在不真正运行 shell 的前提下确认的引用才算数：一条裸的 `source`/`.` 语句（只要不是嵌在某个 `if` 代码块里——`if` 的判断条件对逐行扫描来说是不透明的，无论条件是什么都不予采信）、形如 `test -f X && . X` / `[ -f X ] && . X` 这种被测试路径与被 source 路径完全相同的存在性守卫，或者 `A || B` 这种回退结构里 A 的那一侧（总会被尝试）——至于 B 的那一侧，只有在 A 自身指向的文件在磁盘上确实不存在时才算数，因为那是这种回退结构唯一能确定一定会执行的情形。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 `doctor`（以及 `pull` 结束后自动运行的检查）还会标记出遗留在*其他*候选文件中的 teamai 环境变量块——例如 #682 之前的旧版本写入 `.bashrc` 的代码块，即便该代码块本身已损坏、从未生效。`teamai uninstall` 会清理它。 From 0e05baaa74d28f5b86407d8b613fa1ea0e96cbbe Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 23 Sep 2026 10:06:33 +0530 Subject: [PATCH 6/9] fix(env): validate reference quoting, credit &&'s left side, generalize block-skip (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 12 found three more real gaps in referencesCandidate(), plus one I agree isn't worth chasing further (see PR reply): - The quote check accepted `source "~/.bashrc"` and `source '$HOME/.bashrc'` as valid references, but a shell never tilde-expands inside any quotes and never variable-expands inside single quotes — both source a literal, near-certainly nonexistent path. Tightened to the three forms that actually expand: bare `~/name`, and `$HOME/name` either bare or double-quoted. - `&&`'s left side is always attempted, the same as `||`'s — `source ~/.bashrc && echo ready` does reach .bashrc regardless of the trailing command, but the old "whole statement must be exactly `. REF`" check missed it. The leftmost command before the first `&&` (or no `&&` at all) is now checked the same way `||`'s left side already was. - Only `if`/`fi` was tracked, so a source inside an uncalled function, a non-selected `case` arm, or a loop body — none of them any more guaranteed to run than an `if` body — was wrongly treated as top-level. Generalized the "don't trust it" depth counter to cover for/while/until, case/esac, and function/brace groups too, sharing one counter since we only need to know whether we're inside *any* of them, not which one. Declined to extend if-body trust to cover the standard nested Debian `.profile` template (`if [ -n "$BASH_VERSION" ]; then if [ -f "$HOME/.bashrc" ]; then . "$HOME/.bashrc"; fi; fi`) — doing so would mean trusting the outer `$BASH_VERSION` check, which is exactly the class of unverifiable shell condition this design has refused since round 11. Fixed the docs instead: they previously (incorrectly) claimed this exact template was recognized; now they say plainly that nested conditionals of any kind fall back to the order-based pick. Verified end-to-end on a real Windows host: re-ran the Git-for-Windows two-pull scenario unaffected. Added 6 unit tests for the new boundaries (invalid vs. valid quoting, &&'s left side, function/case/ loop bodies). 41/41 in shell-profile.test.ts. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 4 +- docs/usage-guide.zh-CN.md | 4 +- src/__tests__/shell-profile.test.ts | 60 +++++++++++++++++++++ src/utils/shell-profile.ts | 83 +++++++++++++++++++---------- 4 files changed, 119 insertions(+), 32 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 7c34ae26..835724a0 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -745,9 +745,9 @@ 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`. -Every pull re-runs this order to find the file the current environment actually reads, then follows whatever that file `source`s — transitively, through as many hops as it takes — looking for a candidate that already carries the block, rather than duplicating it. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it: that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without following that forwarding relationship the next pull would prefer the newly-created file and inject a second block there, leaving the original — still working, just loaded further away — reported as a dead leftover. The same reasoning covers a plain `.profile` that sources `.bashrc` for interactive shells (the standard Debian/Ubuntu template), two hops from whatever a login shell reads first. +Every pull re-runs this order to find the file the current environment actually reads, then follows whatever that file `source`s — transitively, through as many hops as it takes — looking for a candidate that already carries the block, rather than duplicating it. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it: that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without following that forwarding relationship the next pull would prefer the newly-created file and inject a second block there, leaving the original — still working, just loaded further away — reported as a dead leftover. The same reasoning covers a plain `.profile` that flat-guards a source of `.bashrc` for interactive shells (`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`), two hops from whatever a login shell reads first. -Only a reference this can verify without running a real shell counts, though: a bare `source`/`.` line (as long as it isn't sitting inside an `if` block, whose condition is opaque to a line scanner either way), a `test -f X && . X` / `[ -f X ] && . X` guard where the tested and sourced path are the same file, or the left side of an `A || B` fallback (always attempted) — plus the right side, but only when `A`'s own target does not exist on disk, the one case that fallback is guaranteed to run. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. +Only a reference this can verify without running a real shell counts, though — an unquoted `~/name` or an unquoted-or-double-quoted `$HOME/name` (never a quoted `~`, never a single-quoted `$HOME`: a shell does not expand either, so a reference that looks right there would source a literal, nonexistent path): a bare `source`/`.` command, on its own or as the always-attempted left side of an `&&` or `||`; the specific self-referential guard `test -f X && . X` / `[ -f X ] && . X` (tested and sourced path the same file, the one `&&` condition this can verify by visiting that file itself); or the right side of an `A || B` fallback, but only when `A`'s own target does not exist on disk, the one case that side is guaranteed to run. Nothing inside an `if`, `for`/`while`/`until`, `case`, or a function body counts, however it's guarded — none of those are guaranteed to run and there is no way to verify one without running a real shell, which also means the standard Debian/Ubuntu `.profile` template (the same source, but nested two `if`s deep, checking `$BASH_VERSION` on the way) is not recognized and falls back to the order-based pick. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. `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. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index a512b105..ec35c3f6 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -718,9 +718,9 @@ 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 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件，然后沿着它 `source` 的内容一路查下去——无论要经过多少跳——寻找一个已经带着代码块的候选文件，而不是重复注入。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不沿着这条转发链去找，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是绕得更远了——则会被误报为失效的遗留代码块。同样的道理也适用于一个普通的 `.profile`（为交互式 shell source `.bashrc`，也就是 Debian/Ubuntu 的标准模板）：这时登录 shell 最先读到的文件，离实际代码块有两跳之遥。 +每次 pull 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件，然后沿着它 `source` 的内容一路查下去——无论要经过多少跳——寻找一个已经带着代码块的候选文件，而不是重复注入。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不沿着这条转发链去找，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是绕得更远了——则会被误报为失效的遗留代码块。同样的道理也适用于一个普通的 `.profile`：它用一条扁平的存在性守卫（`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`）为交互式 shell source `.bashrc`——这时登录 shell 最先读到的文件，离实际代码块有两跳之遥。 -不过，只有这段扫描逻辑能在不真正运行 shell 的前提下确认的引用才算数：一条裸的 `source`/`.` 语句（只要不是嵌在某个 `if` 代码块里——`if` 的判断条件对逐行扫描来说是不透明的，无论条件是什么都不予采信）、形如 `test -f X && . X` / `[ -f X ] && . X` 这种被测试路径与被 source 路径完全相同的存在性守卫，或者 `A || B` 这种回退结构里 A 的那一侧（总会被尝试）——至于 B 的那一侧，只有在 A 自身指向的文件在磁盘上确实不存在时才算数，因为那是这种回退结构唯一能确定一定会执行的情形。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 +不过，只有这段扫描逻辑能在不真正运行 shell 的前提下确认的引用才算数——不加引号的 `~/name`，或不加引号/用双引号包裹的 `$HOME/name`（绝不是加了引号的 `~`，也绝不是用单引号包裹的 `$HOME`：shell 不会展开这两种写法，看起来对的引用实际会 source 一个不存在的字面路径）：一条裸的 `source`/`.` 命令，单独出现，或者作为 `&&`/`||` 里总会被尝试的左侧；形如 `test -f X && . X` / `[ -f X ] && . X` 这种被测试路径与被 source 路径完全相同的存在性守卫（这是唯一能靠亲自访问该文件来验证的 `&&` 条件）；或者 `A || B` 这种回退结构里 B 的那一侧——只有在 A 自身指向的文件在磁盘上确实不存在时才算数，因为那是这种回退结构唯一能确定一定会执行的情形。嵌在 `if`、`for`/`while`/`until`、`case` 或函数体里的内容一律不算数，不管外层条件写的是什么——这些结构都不保证一定会执行，而不真正运行 shell 就没有办法验证，这也意味着 Debian/Ubuntu 标准模板里那种嵌套两层 `if`、沿途还检查 `$BASH_VERSION` 的写法无法被识别，会回退到按优先级选出的文件。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 `doctor`（以及 `pull` 结束后自动运行的检查）还会标记出遗留在*其他*候选文件中的 teamai 环境变量块——例如 #682 之前的旧版本写入 `.bashrc` 的代码块，即便该代码块本身已损坏、从未生效。`teamai uninstall` 会清理它。 diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 224358ac..5913f728 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -291,6 +291,66 @@ describe('resolveActiveShellProfile', () => { await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); + + // Regression (#693 review round 12): a shell never tilde-expands inside + // any quotes and never variable-expands inside single quotes, so + // `source "~/.bashrc"` and `source '$HOME/.bashrc'` both source a + // literal, near-certainly nonexistent path — a reference that "looks + // right" but would never actually run must not be trusted. + it('does not treat an invalidly-quoted reference as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'source "~/.bashrc"\nsource \'$HOME/.bashrc\'\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does treat a double-quoted $HOME reference as reachable', async () => { + await fse.writeFile(path.join(homeDir, '.bash_profile'), 'source "$HOME/.bashrc"\n'); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + // Regression (#693 review round 12): `&&`'s left side is always attempted, + // the same as `||`'s — a trailing unrelated command after it (`&& echo + // ready`) does not make the source itself conditional. + it('treats the left side of && as reachable even when the right side is unrelated', async () => { + await fse.writeFile(path.join(homeDir, '.bash_profile'), 'source ~/.bashrc && echo ready\n'); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + // Regression (#693 review round 12): only `if` nesting was tracked, so a + // source inside an uncalled function, a non-selected `case` arm, or a + // loop body — none of them guaranteed to run any more than an `if` body + // is — was wrongly treated as unconditional. + it('does not treat a source inside a function body as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'my_func() {\n . ~/.bashrc\n}\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not treat a source inside a case arm as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'case "$-" in\n *i*) . ~/.bashrc ;;\nesac\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not treat a source inside a loop body as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'for f in ~/.bashrc; do\n . ~/.bashrc\ndone\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); }); describe('envBlockSourcesPath', () => { diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index 93b2bd2a..e263181e 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -203,41 +203,65 @@ export function envBlockReferencesDataHome(block: string, envShPath: string): bo } /** `~/name`, `$HOME/name` or `${HOME}/name`, optionally quoted, as a token this scanner accepts as a reference to `name`. */ +/** + * `~/name` (never quoted — a shell does not tilde-expand inside any quotes) + * or `$HOME/name` / `${HOME}/name` (unquoted or double-quoted — a shell + * does not variable-expand inside single quotes) as a token this scanner + * accepts as a reference to `name`. `source "~/.bashrc"` and + * `source '$HOME/.bashrc'` both source a literal, near-certainly + * nonexistent path, not `name` — a reference that "looks right" but would + * never actually reach the file must not be trusted (#693 review round 12). + */ function homeRelativeRef(name: string): string { const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - return `["']?(?:~|\\$\\{?HOME\\}?)/${escaped}(?![\\w.-])["']?`; + const suffix = `${escaped}(?![\\w.-])`; + return `(?:~/${suffix}|\\$\\{?HOME\\}?/${suffix}|"\\$\\{?HOME\\}?/${suffix}")`; } -/** If `token` is a home-relative reference (`~/name`, `$HOME/name`, `${HOME}/name`), the real path it names. */ +/** If `token` is a home-relative reference by the same rule `homeRelativeRef` accepts, the real path it names. */ function homeRelativePath(token: string, home: string): string | null { - const stripped = token.replace(/^["']/, '').replace(/["']$/, ''); - const m = stripped.match(/^(?:~|\$\{?HOME\}?)\/(.+)$/); + let m = token.match(/^~\/(.+)$/); + if (!m) m = token.match(/^\$\{?HOME\}?\/(.+)$/); + if (!m) m = token.match(/^"\$\{?HOME\}?\/(.+)"$/); return m ? path.join(home, m[1]) : null; } +/** Whether `line` opens, or closes, a construct whose body is not guaranteed to run — `if`, `for`/`while`/`until`, `case`, or a function/brace group. */ +function opensUnverifiedBlock(line: string): boolean { + return /^(?:if|for|while|until|case)\b/.test(line) + || /^function\s+\S/.test(line) + || /^\S+\s*\(\)\s*\{?\s*$/.test(line) + || line === '{'; +} +function closesUnverifiedBlock(line: string): boolean { + return /^(?:fi|done|esac)\b/.test(line) || line === '}'; +} + /** - * Whether `content` runs a `source`/`.` command reaching `name` - * (`~/.bashrc`, `$HOME/.bashrc`, `${HOME}/.bashrc`), restricted to the forms - * this scanner can reason about without a real shell parser (#693 review - * round 11 named two more gaps a bare substring/`&&`/`;` split left open): + * Whether `content` runs a `source`/`.` command reaching `name`, restricted + * to the forms this scanner can reason about without a real shell parser + * (#693 review rounds 11-12 named the gaps a plain substring/`&&`/`;` split + * left open, one at a time): * * - **Unconditional**: a bare `. REF` / `source REF`, as its own `;`-joined - * statement. Nothing inside an `if` block counts, conditional or not — - * the condition is opaque to a line scanner, so a source sitting three - * lines under `if [ -n "$BASH_VERSION" ]; then` is no more verifiable - * than one under `if [ "$TERM_PROGRAM" = vscode ]; then`, and trusting - * either would risk the same false "reachable" #682 regression the - * sticky resolver exists to prevent. - * - **Existence-gated**: `test -f REF && . REF` / `[ -f REF ] && . REF`, + * statement, or the leftmost command before the first `&&`/`||` in one + * (both operators always attempt their left side first). Nothing inside + * an `if`, `for`/`while`/`until`, `case`, or a function/brace-group body + * counts, no matter how it is written — none of those bodies are + * guaranteed to run, and this scanner has no way to tell a body that + * would from one that wouldn't, so trusting some and not others would + * just be guessing (the same false "reachable" #682 regression the + * sticky resolver exists to prevent). + * - **Existence-gated `&&`**: `test -f REF && . REF` / `[ -f REF ] && . REF`, * self-referential only — the tested path and the sourced path must both - * be `name`, the one condition this scanner can independently verify (by - * visiting that candidate itself later in the search). A condition - * testing anything else grants nothing. + * be `name`, the one `&&` condition this scanner can independently verify + * (by visiting that candidate itself later in the search). A condition + * testing anything else grants nothing beyond its own unconditional left + * side. * - **`||` fallback**: `A || B`, where `A` is a source of some other - * candidate. The left side of `||` is always attempted, so it counts - * unconditionally; the right side runs only if the left one fails, which - * is verifiable in exactly one case — `A`'s own target does not exist on - * disk — so `B` counts only then. + * candidate. `B` runs only if `A` fails, which is verifiable in exactly + * one case — `A`'s own target does not exist on disk — so `B` counts + * only then. * * A file, or a shell construct, this cannot resolve one way or the other is * never trusted either way: the caller falls back to the order-based pick, @@ -253,13 +277,13 @@ async function referencesCandidate(content: string, name: string, home: string): ); const sourceOf = /^(?:\.|source)\s+(\S+)$/; - let ifDepth = 0; + let depth = 0; for (const rawLine of content.split('\n')) { const line = rawLine.trim(); if (!line || line.startsWith('#')) continue; - if (/^if\b/.test(line)) { ifDepth += 1; continue; } - if (/^fi\b/.test(line)) { ifDepth = Math.max(0, ifDepth - 1); continue; } - if (ifDepth > 0) continue; + if (opensUnverifiedBlock(line)) { depth += 1; continue; } + if (closesUnverifiedBlock(line)) { depth = Math.max(0, depth - 1); continue; } + if (depth > 0) continue; for (const statement of line.split(';').map((s) => s.trim()).filter(Boolean)) { if (existenceGated.test(statement)) return true; @@ -276,8 +300,11 @@ async function referencesCandidate(content: string, name: string, home: string): continue; } - const plain = statement.match(sourceOf); - if (plain && refOnly.test(plain[1])) return true; + // The leftmost command before the first `&&` (if any) is always + // attempted, same as `||`'s left side above — only its right side's + // extra condition is unverifiable in general. + const andLeft = statement.split('&&')[0].trim().match(sourceOf); + if (andLeft && refOnly.test(andLeft[1])) return true; } } return false; From 6adaf9e01bc0c16f23d731b1081ccd4cc6965ab6 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 23 Sep 2026 10:22:11 +0530 Subject: [PATCH 7/9] fix(env): handle comments, line continuations, heredocs, and chained && in profile scanning (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 13 review found five genuine structural gaps in referencesCandidate, all fixed by moving open/close-block detection to per-statement (post `;`-split) instead of per-line, and adding a logicalLines() preprocessing pass: - Backslash-continued lines were scanned independently, losing the conditional context of the line they continue (`cond && \` followed by `source X` on the next line looked unconditional). - A one-line `if ...; then ...; fi` only incremented depth (matched via the whole-line "opens" check) and never saw its own `fi` close it, permanently disabling recognition of every later unconditional source in the file. Two-line function definitions (`fn()` then `{` on its own line) double-incremented for the same reason. - The existence-gated `&&` guard was fully anchored, so a guarded source followed by further `&&`-chained commands (`[ -f X ] && . X && export Y`) didn't match even though the guard still holds. - Comment stripping only skipped whole-comment lines; a comment following a semicolon on the same line was still split into a "real" statement. - Heredoc bodies were scanned as literal executable lines. Declined the sixth (recognizing the Debian/Ubuntu nested `if [ -n "$BASH_VERSION" ]; then if [ -f ... ]; then . ...; fi; fi` template) for the same reason given in review round 12: the outer condition is unverifiable without a real shell, and this resolver's explicit, repeatedly-restated design boundary is to never trust an unverifiable condition — falling back to the order-based pick (a harmless duplicate block) is the intended safe behavior there, not a bug. Verified with 6 new unit tests (47/47 passing) plus a standalone real-fs script driving the actual resolveActiveShellProfile against a scratch HOME for all seven round-13 scenarios (all pass). Full suite unchanged at the pre-existing 30-failed-file/66-failed-test Windows-host baseline. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 4 +- docs/usage-guide.zh-CN.md | 4 +- src/__tests__/shell-profile.test.ts | 54 +++++++++++++++++ src/utils/shell-profile.ts | 90 ++++++++++++++++++++++------- 4 files changed, 127 insertions(+), 25 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 835724a0..7b44aeee 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -745,9 +745,9 @@ 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`. -Every pull re-runs this order to find the file the current environment actually reads, then follows whatever that file `source`s — transitively, through as many hops as it takes — looking for a candidate that already carries the block, rather than duplicating it. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it: that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without following that forwarding relationship the next pull would prefer the newly-created file and inject a second block there, leaving the original — still working, just loaded further away — reported as a dead leftover. The same reasoning covers a plain `.profile` that flat-guards a source of `.bashrc` for interactive shells (`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`), two hops from whatever a login shell reads first. +Every pull re-runs this order to find the file the current environment actually reads, then follows every reference from it to one of the other four candidate filenames — transitively, through as many hops as it takes — looking for a candidate that already carries the block, rather than duplicating it. A chain through a file outside that fixed set of five (e.g. a custom `~/.config/shell/profile` some setups source instead) is not followed. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it: that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without following that forwarding relationship the next pull would prefer the newly-created file and inject a second block there, leaving the original — still working, just loaded further away — reported as a dead leftover. The same reasoning covers a plain `.profile` that flat-guards a source of `.bashrc` for interactive shells (`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`), two hops from whatever a login shell reads first. -Only a reference this can verify without running a real shell counts, though — an unquoted `~/name` or an unquoted-or-double-quoted `$HOME/name` (never a quoted `~`, never a single-quoted `$HOME`: a shell does not expand either, so a reference that looks right there would source a literal, nonexistent path): a bare `source`/`.` command, on its own or as the always-attempted left side of an `&&` or `||`; the specific self-referential guard `test -f X && . X` / `[ -f X ] && . X` (tested and sourced path the same file, the one `&&` condition this can verify by visiting that file itself); or the right side of an `A || B` fallback, but only when `A`'s own target does not exist on disk, the one case that side is guaranteed to run. Nothing inside an `if`, `for`/`while`/`until`, `case`, or a function body counts, however it's guarded — none of those are guaranteed to run and there is no way to verify one without running a real shell, which also means the standard Debian/Ubuntu `.profile` template (the same source, but nested two `if`s deep, checking `$BASH_VERSION` on the way) is not recognized and falls back to the order-based pick. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. +Only a reference this can verify without running a real shell counts, though — an unquoted `~/name` or an unquoted-or-double-quoted `$HOME/name` (never a quoted `~`, never a single-quoted `$HOME`: a shell does not expand either, so a reference that looks right there would source a literal, nonexistent path): a bare `source`/`.` command, on its own or as the always-attempted left side of an `&&` or `||`; the specific self-referential guard `test -f X && . X` / `[ -f X ] && . X` (tested and sourced path the same file, the one `&&` condition this can verify by visiting that file itself — further `&&`-chained commands after the guarded source don't change whether it ran, so they don't affect the match either); or the right side of an `A || B` fallback, but only when `A`'s own target does not exist on disk, the one case that side is guaranteed to run. Nothing inside an `if`, `for`/`while`/`until`, `case`, or a function body counts, however it's guarded — none of those are guaranteed to run and there is no way to verify one without running a real shell, which also means the standard Debian/Ubuntu `.profile` template (the same source, but nested two `if`s deep, checking `$BASH_VERSION` on the way) is not recognized and falls back to the order-based pick. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. `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. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index ec35c3f6..2466b51a 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -718,9 +718,9 @@ 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 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件，然后沿着它 `source` 的内容一路查下去——无论要经过多少跳——寻找一个已经带着代码块的候选文件，而不是重复注入。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不沿着这条转发链去找，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是绕得更远了——则会被误报为失效的遗留代码块。同样的道理也适用于一个普通的 `.profile`：它用一条扁平的存在性守卫（`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`）为交互式 shell source `.bashrc`——这时登录 shell 最先读到的文件，离实际代码块有两跳之遥。 +每次 pull 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件，然后沿着它对另外四个候选文件名的引用一路查下去——无论要经过多少跳——寻找一个已经带着代码块的候选文件，而不是重复注入。如果链条中间经过的是这五个候选文件名之外的文件（比如某些环境会改用 `~/.config/shell/profile` 这类自定义文件来 source），这条链就不会被继续跟踪。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不沿着这条转发链去找，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是绕得更远了——则会被误报为失效的遗留代码块。同样的道理也适用于一个普通的 `.profile`：它用一条扁平的存在性守卫（`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`）为交互式 shell source `.bashrc`——这时登录 shell 最先读到的文件，离实际代码块有两跳之遥。 -不过，只有这段扫描逻辑能在不真正运行 shell 的前提下确认的引用才算数——不加引号的 `~/name`，或不加引号/用双引号包裹的 `$HOME/name`（绝不是加了引号的 `~`，也绝不是用单引号包裹的 `$HOME`：shell 不会展开这两种写法，看起来对的引用实际会 source 一个不存在的字面路径）：一条裸的 `source`/`.` 命令，单独出现，或者作为 `&&`/`||` 里总会被尝试的左侧；形如 `test -f X && . X` / `[ -f X ] && . X` 这种被测试路径与被 source 路径完全相同的存在性守卫（这是唯一能靠亲自访问该文件来验证的 `&&` 条件）；或者 `A || B` 这种回退结构里 B 的那一侧——只有在 A 自身指向的文件在磁盘上确实不存在时才算数，因为那是这种回退结构唯一能确定一定会执行的情形。嵌在 `if`、`for`/`while`/`until`、`case` 或函数体里的内容一律不算数，不管外层条件写的是什么——这些结构都不保证一定会执行，而不真正运行 shell 就没有办法验证，这也意味着 Debian/Ubuntu 标准模板里那种嵌套两层 `if`、沿途还检查 `$BASH_VERSION` 的写法无法被识别，会回退到按优先级选出的文件。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 +不过，只有这段扫描逻辑能在不真正运行 shell 的前提下确认的引用才算数——不加引号的 `~/name`，或不加引号/用双引号包裹的 `$HOME/name`（绝不是加了引号的 `~`，也绝不是用单引号包裹的 `$HOME`：shell 不会展开这两种写法，看起来对的引用实际会 source 一个不存在的字面路径）：一条裸的 `source`/`.` 命令，单独出现，或者作为 `&&`/`||` 里总会被尝试的左侧；形如 `test -f X && . X` / `[ -f X ] && . X` 这种被测试路径与被 source 路径完全相同的存在性守卫（这是唯一能靠亲自访问该文件来验证的 `&&` 条件——守卫之后如果还用 `&&` 接了别的命令，并不影响被守卫的 source 是否执行，所以也不影响这里的判断）；或者 `A || B` 这种回退结构里 B 的那一侧——只有在 A 自身指向的文件在磁盘上确实不存在时才算数，因为那是这种回退结构唯一能确定一定会执行的情形。嵌在 `if`、`for`/`while`/`until`、`case` 或函数体里的内容一律不算数，不管外层条件写的是什么——这些结构都不保证一定会执行，而不真正运行 shell 就没有办法验证，这也意味着 Debian/Ubuntu 标准模板里那种嵌套两层 `if`、沿途还检查 `$BASH_VERSION` 的写法无法被识别，会回退到按优先级选出的文件。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 `doctor`（以及 `pull` 结束后自动运行的检查）还会标记出遗留在*其他*候选文件中的 teamai 环境变量块——例如 #682 之前的旧版本写入 `.bashrc` 的代码块，即便该代码块本身已损坏、从未生效。`teamai uninstall` 会清理它。 diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 5913f728..0773fe4e 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -351,6 +351,60 @@ describe('resolveActiveShellProfile', () => { await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); + + it('does not treat a source after a backslash-continued unrelated condition as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '[ "$TERM_PROGRAM" = vscode ] && \\\nsource ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not stop recognizing later unconditional sources after a one-line if/then/fi', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'if [ -f ~/.zshrc ]; then . ~/.zshrc; fi\nsource ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('does not stop recognizing later unconditional sources after a two-line function definition', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'my_func()\n{\n echo hi\n}\nsource ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('treats an existence-gated source as reachable even with further &&-chained commands', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '[ -f ~/.bashrc ] && . ~/.bashrc && export READY=1\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('does not treat a source inside a comment after a semicolon as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + ': # old setup; source ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not treat a source inside a heredoc body as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + "cat <<'EOF'\nsource ~/.bashrc\nEOF\n", + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); }); describe('envBlockSourcesPath', () => { diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index e263181e..eda807f1 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -226,15 +226,58 @@ function homeRelativePath(token: string, home: string): string | null { return m ? path.join(home, m[1]) : null; } -/** Whether `line` opens, or closes, a construct whose body is not guaranteed to run — `if`, `for`/`while`/`until`, `case`, or a function/brace group. */ -function opensUnverifiedBlock(line: string): boolean { - return /^(?:if|for|while|until|case)\b/.test(line) - || /^function\s+\S/.test(line) - || /^\S+\s*\(\)\s*\{?\s*$/.test(line) - || line === '{'; +/** Whether `statement` opens, or closes, a construct whose body is not guaranteed to run — `if`, `for`/`while`/`until`, `case`, or a function/brace group. */ +function opensUnverifiedBlock(statement: string): boolean { + return /^(?:if|for|while|until|case)\b/.test(statement) + || /^function\s+\S/.test(statement) + || /^\S+\s*\(\)\s*\{?\s*$/.test(statement) + || statement === '{'; } -function closesUnverifiedBlock(line: string): boolean { - return /^(?:fi|done|esac)\b/.test(line) || line === '}'; +function closesUnverifiedBlock(statement: string): boolean { + return /^(?:fi|done|esac)\b/.test(statement) || statement === '}'; +} + +/** Strips a trailing shell comment (`#` at the start of a word, outside this scanner's quote-naive view) from `line`. */ +function stripComment(line: string): string { + const at = line.search(/(?:^|\s)#/); + if (at === -1) return line; + return line.slice(0, line.indexOf('#', at)).trimEnd(); +} + +/** + * Splits `content` into logical lines: strips comments, joins `\`-continued + * lines, joins a lone `{` onto the function/construct header it opens (`fn()` + * then `{` on its own line), and drops heredoc bodies entirely (their text is + * data, never executed statements — #693 review round 13). + */ +function logicalLines(content: string): string[] { + const result: string[] = []; + const rawLines = content.split('\n'); + let heredocEnd: string | null = null; + + for (let i = 0; i < rawLines.length; i += 1) { + if (heredocEnd !== null) { + if (rawLines[i].trim() === heredocEnd) heredocEnd = null; + continue; + } + + let line = stripComment(rawLines[i]).trim(); + while (line.endsWith('\\') && !line.endsWith('\\\\')) { + i += 1; + line = `${line.slice(0, -1).trimEnd()} ${(i < rawLines.length ? stripComment(rawLines[i]) : '').trim()}`.trim(); + } + if (!line) continue; + + const heredoc = line.match(/<<-?\s*(['"]?)(\w+)\1/); + if (heredoc) heredocEnd = heredoc[2]; + + if (line === '{' && result.length > 0) { + result[result.length - 1] += ' {'; + continue; + } + result.push(line); + } + return result; } /** @@ -257,7 +300,9 @@ function closesUnverifiedBlock(line: string): boolean { * be `name`, the one `&&` condition this scanner can independently verify * (by visiting that candidate itself later in the search). A condition * testing anything else grants nothing beyond its own unconditional left - * side. + * side. Further `&&`-chained commands after the guarded source don't + * change whether it ran, so they don't affect the match either (#693 + * review round 13). * - **`||` fallback**: `A || B`, where `A` is a source of some other * candidate. `B` runs only if `A` fails, which is verifiable in exactly * one case — `A`'s own target does not exist on disk — so `B` counts @@ -272,21 +317,24 @@ function closesUnverifiedBlock(line: string): boolean { async function referencesCandidate(content: string, name: string, home: string): Promise { const ref = homeRelativeRef(name); const refOnly = new RegExp(`^${ref}$`); - const existenceGated = new RegExp( - `^(?:test\\s+-f\\s+${ref}|\\[\\s+-f\\s+${ref}\\s*\\])\\s*&&\\s*(?:\\.|source)\\s+${ref}$`, - ); + const existenceGuard = new RegExp(`^(?:test\\s+-f\\s+${ref}|\\[\\s+-f\\s+${ref}\\s*\\])$`); const sourceOf = /^(?:\.|source)\s+(\S+)$/; let depth = 0; - for (const rawLine of content.split('\n')) { - const line = rawLine.trim(); - if (!line || line.startsWith('#')) continue; - if (opensUnverifiedBlock(line)) { depth += 1; continue; } - if (closesUnverifiedBlock(line)) { depth = Math.max(0, depth - 1); continue; } - if (depth > 0) continue; - + for (const line of logicalLines(content)) { for (const statement of line.split(';').map((s) => s.trim()).filter(Boolean)) { - if (existenceGated.test(statement)) return true; + if (opensUnverifiedBlock(statement)) { depth += 1; continue; } + if (closesUnverifiedBlock(statement)) { depth = Math.max(0, depth - 1); continue; } + if (depth > 0) continue; + + // Existence-gated `&&`: `test -f REF && . REF`, self-referential, with + // any further `&&`-chained commands after it not affecting whether the + // guarded source itself ran (#693 review round 13). + const andParts = statement.split('&&').map((s) => s.trim()); + if (andParts.length >= 2 && existenceGuard.test(andParts[0])) { + const guarded = andParts[1].match(sourceOf); + if (guarded && refOnly.test(guarded[1])) return true; + } const orParts = statement.split('||').map((s) => s.trim()); if (orParts.length === 2) { @@ -303,7 +351,7 @@ async function referencesCandidate(content: string, name: string, home: string): // The leftmost command before the first `&&` (if any) is always // attempted, same as `||`'s left side above — only its right side's // extra condition is unverifiable in general. - const andLeft = statement.split('&&')[0].trim().match(sourceOf); + const andLeft = andParts[0].match(sourceOf); if (andLeft && refOnly.test(andLeft[1])) return true; } } From 413b0a4921dfa1b3e83b0a86967a83f054cfd850 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 23 Sep 2026 10:33:53 +0530 Subject: [PATCH 8/9] fix(env): quote-aware statement splitting, subshells, dead code after return/exit, N-way || (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 14 review found six more genuine gaps in referencesCandidate, all fixed: - The `;`/`&&`/`||` splits were plain `String.split`, so a separator character inside a quoted argument (e.g. `printf '%s' 'x; source ~/.bashrc; y'`) was treated as a real statement boundary, inventing an executed source out of string data. Added splitTopLevel(), a small quote-aware splitter (tracks single/double-quote spans, skips separators inside them) used everywhere a naive split was previously used. - `(...)` subshells weren't tracked as an unverified-block construct — a source inside one always runs, but its exports never reach the caller, so it must not count as reaching a candidate any more than an `if` body does. Added to opensUnverifiedBlock/closesUnverifiedBlock alongside the existing if/for/while/until/case/function handling. - An unconditional, top-level `return`/`exit` ends the file's control flow right there; anything textually after it was still being scanned as if reachable. Added a `halted` flag set on a bare return/exit statement, gating everything after it for the rest of the scan. - `sourceOf` required the source's argument to be the entire statement, so `. "$HOME/.bashrc" 2>/dev/null` and `source ~/.bashrc extra_arg` (both valid, both really sourcing the target) went unrecognized. Relaxed to capture just the first argument and allow anything after it. - The `||` fallback only handled exactly two operands — a three-way chain like `source ~/.profile || source ~/.bash_login || source ~/.bashrc` wasn't recognized at all, not even the always-attempted left side. Generalized to N operands: each one counts only when every operand before it is a recognized source whose target is verifiably missing from disk. - Multiple heredocs opened by one command (`cat < --- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- src/__tests__/shell-profile.test.ts | 73 ++++++++++++++++++++ src/utils/shell-profile.ts | 103 ++++++++++++++++++++++------ 4 files changed, 158 insertions(+), 22 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 7b44aeee..aa65df95 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -747,7 +747,7 @@ On `pull`, when `injectShellProfile` is enabled (default), the env block goes in Every pull re-runs this order to find the file the current environment actually reads, then follows every reference from it to one of the other four candidate filenames — transitively, through as many hops as it takes — looking for a candidate that already carries the block, rather than duplicating it. A chain through a file outside that fixed set of five (e.g. a custom `~/.config/shell/profile` some setups source instead) is not followed. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it: that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without following that forwarding relationship the next pull would prefer the newly-created file and inject a second block there, leaving the original — still working, just loaded further away — reported as a dead leftover. The same reasoning covers a plain `.profile` that flat-guards a source of `.bashrc` for interactive shells (`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`), two hops from whatever a login shell reads first. -Only a reference this can verify without running a real shell counts, though — an unquoted `~/name` or an unquoted-or-double-quoted `$HOME/name` (never a quoted `~`, never a single-quoted `$HOME`: a shell does not expand either, so a reference that looks right there would source a literal, nonexistent path): a bare `source`/`.` command, on its own or as the always-attempted left side of an `&&` or `||`; the specific self-referential guard `test -f X && . X` / `[ -f X ] && . X` (tested and sourced path the same file, the one `&&` condition this can verify by visiting that file itself — further `&&`-chained commands after the guarded source don't change whether it ran, so they don't affect the match either); or the right side of an `A || B` fallback, but only when `A`'s own target does not exist on disk, the one case that side is guaranteed to run. Nothing inside an `if`, `for`/`while`/`until`, `case`, or a function body counts, however it's guarded — none of those are guaranteed to run and there is no way to verify one without running a real shell, which also means the standard Debian/Ubuntu `.profile` template (the same source, but nested two `if`s deep, checking `$BASH_VERSION` on the way) is not recognized and falls back to the order-based pick. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. +Only a reference this can verify without running a real shell counts, though — an unquoted `~/name` or an unquoted-or-double-quoted `$HOME/name` (never a quoted `~`, never a single-quoted `$HOME`: a shell does not expand either, so a reference that looks right there would source a literal, nonexistent path): a bare `source`/`.` command, on its own or as the always-attempted left side of an `&&` or `||`; the specific self-referential guard `test -f X && . X` / `[ -f X ] && . X` (tested and sourced path the same file, the one `&&` condition this can verify by visiting that file itself — further `&&`-chained commands after the guarded source don't change whether it ran, so they don't affect the match either); or an operand of an `A || B || C || ...` fallback chain, but only when every operand before it is a recognized source whose own target does not exist on disk, the one case that operand is guaranteed to be reached. Nothing inside an `if`, `for`/`while`/`until`, `case`, a function body, or a `(...)` subshell counts, however it's guarded — none of those are guaranteed to run (a subshell's body may always run, but its exports never reach the caller either way) and there is no way to verify one without running a real shell, which also means the standard Debian/Ubuntu `.profile` template (the same source, but nested two `if`s deep, checking `$BASH_VERSION` on the way) is not recognized and falls back to the order-based pick. Nothing textually after an unconditional, top-level `return` or `exit` counts either, since control never reaches it. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. `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. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 2466b51a..bcd765bc 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -720,7 +720,7 @@ teamai push 每次 pull 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件，然后沿着它对另外四个候选文件名的引用一路查下去——无论要经过多少跳——寻找一个已经带着代码块的候选文件，而不是重复注入。如果链条中间经过的是这五个候选文件名之外的文件（比如某些环境会改用 `~/.config/shell/profile` 这类自定义文件来 source），这条链就不会被继续跟踪。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不沿着这条转发链去找，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是绕得更远了——则会被误报为失效的遗留代码块。同样的道理也适用于一个普通的 `.profile`：它用一条扁平的存在性守卫（`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`）为交互式 shell source `.bashrc`——这时登录 shell 最先读到的文件，离实际代码块有两跳之遥。 -不过，只有这段扫描逻辑能在不真正运行 shell 的前提下确认的引用才算数——不加引号的 `~/name`，或不加引号/用双引号包裹的 `$HOME/name`（绝不是加了引号的 `~`，也绝不是用单引号包裹的 `$HOME`：shell 不会展开这两种写法，看起来对的引用实际会 source 一个不存在的字面路径）：一条裸的 `source`/`.` 命令，单独出现，或者作为 `&&`/`||` 里总会被尝试的左侧；形如 `test -f X && . X` / `[ -f X ] && . X` 这种被测试路径与被 source 路径完全相同的存在性守卫（这是唯一能靠亲自访问该文件来验证的 `&&` 条件——守卫之后如果还用 `&&` 接了别的命令，并不影响被守卫的 source 是否执行，所以也不影响这里的判断）；或者 `A || B` 这种回退结构里 B 的那一侧——只有在 A 自身指向的文件在磁盘上确实不存在时才算数，因为那是这种回退结构唯一能确定一定会执行的情形。嵌在 `if`、`for`/`while`/`until`、`case` 或函数体里的内容一律不算数，不管外层条件写的是什么——这些结构都不保证一定会执行，而不真正运行 shell 就没有办法验证，这也意味着 Debian/Ubuntu 标准模板里那种嵌套两层 `if`、沿途还检查 `$BASH_VERSION` 的写法无法被识别，会回退到按优先级选出的文件。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 +不过，只有这段扫描逻辑能在不真正运行 shell 的前提下确认的引用才算数——不加引号的 `~/name`，或不加引号/用双引号包裹的 `$HOME/name`（绝不是加了引号的 `~`，也绝不是用单引号包裹的 `$HOME`：shell 不会展开这两种写法，看起来对的引用实际会 source 一个不存在的字面路径）：一条裸的 `source`/`.` 命令，单独出现，或者作为 `&&`/`||` 里总会被尝试的左侧；形如 `test -f X && . X` / `[ -f X ] && . X` 这种被测试路径与被 source 路径完全相同的存在性守卫（这是唯一能靠亲自访问该文件来验证的 `&&` 条件——守卫之后如果还用 `&&` 接了别的命令，并不影响被守卫的 source 是否执行，所以也不影响这里的判断）；或者 `A || B || C || ...` 这种回退链条里的某一项——只有当它前面的每一项都是能识别的 source、且各自指向的文件在磁盘上确实不存在时才算数，因为那是这一项唯一能确定一定会被执行到的情形。嵌在 `if`、`for`/`while`/`until`、`case`、函数体，或者 `(...)` 子 shell 里的内容一律不算数，不管外层条件写的是什么——这些结构要么不保证一定会执行，要么即使一定会执行（比如子 shell），它导出的环境变量也传不到调用它的 shell 里；而不真正运行 shell 就没有办法验证前一种情况，这也意味着 Debian/Ubuntu 标准模板里那种嵌套两层 `if`、沿途还检查 `$BASH_VERSION` 的写法无法被识别，会回退到按优先级选出的文件。位于无条件的顶层 `return` 或 `exit` 之后的内容同样不算数，因为控制流根本不会执行到那里。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 `doctor`（以及 `pull` 结束后自动运行的检查）还会标记出遗留在*其他*候选文件中的 teamai 环境变量块——例如 #682 之前的旧版本写入 `.bashrc` 的代码块，即便该代码块本身已损坏、从未生效。`teamai uninstall` 会清理它。 diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 0773fe4e..819b1dcd 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -405,6 +405,79 @@ describe('resolveActiveShellProfile', () => { await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); + + it('does not treat a quoted separator as a real statement boundary', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + "printf '%s\\n' 'x; source ~/.bashrc; y'\n", + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not treat a source inside a subshell as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '(\n source ~/.bashrc\n)\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not treat a source after an unconditional return as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'return\nsource ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not treat a source after an unconditional exit as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'exit 0\nsource ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('treats a source with a trailing redirection as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'source ~/.bashrc 2>/dev/null\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('treats the last operand of a three-way || fallback as reachable when the earlier ones are missing', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'source ~/.profile || source ~/.bash_login || source ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('does not treat the last operand of a three-way || fallback as reachable when an earlier one exists', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'source ~/.profile || source ~/.bash_login || source ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.profile'), '# unrelated\n'); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not treat a source inside the second of two heredocs on one command as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + "cat < { diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index eda807f1..a5b1f293 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -226,15 +226,16 @@ function homeRelativePath(token: string, home: string): string | null { return m ? path.join(home, m[1]) : null; } -/** Whether `statement` opens, or closes, a construct whose body is not guaranteed to run — `if`, `for`/`while`/`until`, `case`, or a function/brace group. */ +/** Whether `statement` opens, or closes, a construct whose body is not guaranteed to run — `if`, `for`/`while`/`until`, `case`, a function/brace group, or a `(...)` subshell (whose exports never reach the caller even when its body always runs — #693 review round 14). */ function opensUnverifiedBlock(statement: string): boolean { return /^(?:if|for|while|until|case)\b/.test(statement) || /^function\s+\S/.test(statement) || /^\S+\s*\(\)\s*\{?\s*$/.test(statement) - || statement === '{'; + || statement === '{' + || statement === '('; } function closesUnverifiedBlock(statement: string): boolean { - return /^(?:fi|done|esac)\b/.test(statement) || statement === '}'; + return /^(?:fi|done|esac)\b/.test(statement) || statement === '}' || statement === ')'; } /** Strips a trailing shell comment (`#` at the start of a word, outside this scanner's quote-naive view) from `line`. */ @@ -244,20 +245,63 @@ function stripComment(line: string): string { return line.slice(0, line.indexOf('#', at)).trimEnd(); } +/** + * Splits `s` on every top-level occurrence of `sep`, skipping any that fall + * inside single or double quotes — the naive `.split(sep)` this replaces + * would otherwise cut a quoted argument in half, e.g. treating the `;` in + * `printf '%s' 'x; source ~/.bashrc; y'` as a real statement separator and + * inventing an executed `source` that was actually just string data (#693 + * review round 14). No escape handling beyond that (matching this scanner's + * existing quote-naive view elsewhere) — good enough to stop a quoted + * separator from being mistaken for a real one, not a full shell lexer. + */ +function splitTopLevel(s: string, sep: string): string[] { + const parts: string[] = []; + let current = ''; + let quote: string | null = null; + for (let i = 0; i < s.length; ) { + const ch = s[i]; + if (quote) { + current += ch; + if (ch === quote) quote = null; + i += 1; + continue; + } + if (ch === '"' || ch === "'") { + quote = ch; + current += ch; + i += 1; + continue; + } + if (s.startsWith(sep, i)) { + parts.push(current); + current = ''; + i += sep.length; + continue; + } + current += ch; + i += 1; + } + parts.push(current); + return parts; +} + /** * Splits `content` into logical lines: strips comments, joins `\`-continued * lines, joins a lone `{` onto the function/construct header it opens (`fn()` * then `{` on its own line), and drops heredoc bodies entirely (their text is - * data, never executed statements — #693 review round 13). + * data, never executed statements — #693 review round 13), tracking every + * terminator in order when a single command opens more than one heredoc + * (`cat < 0) { + if (rawLines[i].trim() === heredocQueue[0]) heredocQueue.shift(); continue; } @@ -268,8 +312,9 @@ function logicalLines(content: string): string[] { } if (!line) continue; - const heredoc = line.match(/<<-?\s*(['"]?)(\w+)\1/); - if (heredoc) heredocEnd = heredoc[2]; + for (const heredoc of line.matchAll(/<<-?\s*(['"]?)(\w+)\1/g)) { + heredocQueue.push(heredoc[2]); + } if (line === '{' && result.length > 0) { result[result.length - 1] += ' {'; @@ -318,32 +363,50 @@ async function referencesCandidate(content: string, name: string, home: string): const ref = homeRelativeRef(name); const refOnly = new RegExp(`^${ref}$`); const existenceGuard = new RegExp(`^(?:test\\s+-f\\s+${ref}|\\[\\s+-f\\s+${ref}\\s*\\])$`); - const sourceOf = /^(?:\.|source)\s+(\S+)$/; + // The target is the first whitespace-run-delimited argument; anything after + // it (extra positional args passed to the sourced script, a redirection + // like `2>/dev/null`) doesn't change whether the source itself runs (#693 + // review round 14). + const sourceOf = /^(?:\.|source)\s+(\S+)(?:\s+\S.*)?$/; let depth = 0; + let halted = false; for (const line of logicalLines(content)) { - for (const statement of line.split(';').map((s) => s.trim()).filter(Boolean)) { + for (const statement of splitTopLevel(line, ';').map((s) => s.trim()).filter(Boolean)) { if (opensUnverifiedBlock(statement)) { depth += 1; continue; } if (closesUnverifiedBlock(statement)) { depth = Math.max(0, depth - 1); continue; } if (depth > 0) continue; + // `return`/`exit`, unconditional and at top level, ends this file's + // control flow right there — nothing textually after it, however it + // looks, ever runs (#693 review round 14). + if (/^(?:return|exit)(?:\s+\S+)?$/.test(statement)) { halted = true; continue; } + if (halted) continue; + // Existence-gated `&&`: `test -f REF && . REF`, self-referential, with // any further `&&`-chained commands after it not affecting whether the // guarded source itself ran (#693 review round 13). - const andParts = statement.split('&&').map((s) => s.trim()); + const andParts = splitTopLevel(statement, '&&').map((s) => s.trim()); if (andParts.length >= 2 && existenceGuard.test(andParts[0])) { const guarded = andParts[1].match(sourceOf); if (guarded && refOnly.test(guarded[1])) return true; } - const orParts = statement.split('||').map((s) => s.trim()); - if (orParts.length === 2) { - const left = orParts[0].match(sourceOf); - const right = orParts[1].match(sourceOf); - if (left && refOnly.test(left[1])) return true; - if (left && right && refOnly.test(right[1])) { - const leftPath = homeRelativePath(left[1], home); - if (leftPath && !(await pathExists(leftPath))) return true; + // `A || B || C || ...`: each operand is reached only if every operand + // before it is a recognized source of a target verifiably missing from + // disk (the one way a `||` fallback is guaranteed to run) — the + // leftmost is always attempted regardless. An operand this can't + // resolve one way or the other stops the chain from being trusted any + // further (#693 review round 14 generalized this past two operands). + const orParts = splitTopLevel(statement, '||').map((s) => s.trim()); + if (orParts.length >= 2) { + let reachable = true; + for (const part of orParts) { + const m = part.match(sourceOf); + if (reachable && m && refOnly.test(m[1])) return true; + if (!reachable) break; + const p = m && homeRelativePath(m[1], home); + reachable = !!p && !(await pathExists(p)); } continue; } From 3c292905dbbf58689555567141712dcbd706bd08 Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 23 Sep 2026 10:58:56 +0530 Subject: [PATCH 9/9] refactor(env): replace the growing ad-hoc shell scanner with a narrow, closed recognizer (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 15 review found nine more genuine parsing bugs, most of them direct consequences of the general-purpose statement/operand machinery added in rounds 13-14 (quote/escape-aware `;`/`&&`/`||` splitting, N-way `||` chains, trailing-argument tolerance on `source`). It also included a meta-finding, correctly: this had grown into a large, incomplete ad-hoc shell parser for what should be a narrow forwarding-detection case, and each round's fix was mostly patching bugs the previous round's own machinery introduced. Full shell parsing is undecidable without a real shell; chasing it one adversarial regex at a time was never going to finish, and it was already producing real regressions (the round-14 `sourceOf` relaxation meant to recognize valid trailing arguments also started recognizing `source ~/.bashrc | cat` and `source ~/.bashrc &`, both of which run in a subshell and never actually reach the caller). Replaced `referencesCandidate`'s open-ended grammar with a closed recognizer of exactly two forms, each matched as a complete logical line: - bare unconditional `. REF` / `source REF` - the self-referential existence guard `test -f REF && . REF` / `[ -f REF ] && . REF` — the literal line Git for Windows itself generates Deleted entirely: quote/escape-aware statement splitting (no longer needed — nothing is split into statements anymore), `||` fallback handling (both the original two-operand and round 14's N-way generalization), trailing-argument/redirection tolerance on `source` (the source of the pipe/background regression above), and comment-stripping (unnecessary now — a line with anything extra on it simply fails the exact-match check, which is a large part of why the statement machinery could be deleted rather than just patched again). Kept, since dropping them would reopen a real false-positive risk rather than just narrow scope: block-depth tracking for `if`/`for`/`while`/`until`/`case`/`select`/function/subshell/brace-group (content inside is either conditional or non-propagating, generalized this round with `select` and a fixed one-liner if/for/while/until/case collapse so a self-contained one-liner doesn't corrupt depth tracking for the rest of the file), heredoc body skipping (fixed three real bugs in it: a `<<<` here-string was mistaken for a `<<` heredoc and swallowed the rest of the file; a non-`-` heredoc's terminator was compared with `.trim()`, letting an indented look-alike end it early; the delimiter charset was `\w` only, missing real delimiters like `END-CONFIG`), a `return`/`exit` halt flag (cheap, and the alternative — textually dead code after an unconditional exit still being scanned — is a genuine false positive, however unlikely the pattern), and joining a line ending in `\`, `&&`, or `||` onto the next (real, unremarkable shell continuation with no backslash required for the latter two — the risk this closes isn't hypothetical: an unrelated trailing `&&` followed by an unconditional-looking `source` on the next line is exactly the shape that would have produced a false "reachable"). Declined the Debian/Ubuntu nested-`if` finding a fourth time, unchanged from rounds 12-14: the outer `$BASH_VERSION` check is unverifiable without a real shell, and this resolver's explicit boundary is that an unverifiable condition is never trusted. The `||`-existence-only pushback from round 14 is now moot — `||` isn't recognized in any form. Net change to shell-profile.ts is negative (-244/+something smaller) despite fixing more bugs than it added, confirming this is a real simplification rather than another round of patches. Verified with an updated unit test suite (61/61 passing — six tests for now-out-of-scope behavior replaced with tests confirming the safe fallback, new tests added for every round-15 fix that was kept) and a standalone real-fs script against the actual built resolver covering all twelve round-15 scenarios (all pass, including the real motivating Git-for-Windows case and the still-declined Debian template). Full suite unchanged at the pre-existing 30-failed-file/66-failed-test Windows-host baseline. Co-Authored-By: Claude Sonnet 5 --- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- src/__tests__/shell-profile.test.ts | 167 ++++++++++++++----- src/utils/shell-profile.ts | 244 ++++++++++------------------ 4 files changed, 211 insertions(+), 204 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index aa65df95..9cde3af6 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -747,7 +747,7 @@ On `pull`, when `injectShellProfile` is enabled (default), the env block goes in Every pull re-runs this order to find the file the current environment actually reads, then follows every reference from it to one of the other four candidate filenames — transitively, through as many hops as it takes — looking for a candidate that already carries the block, rather than duplicating it. A chain through a file outside that fixed set of five (e.g. a custom `~/.config/shell/profile` some setups source instead) is not followed. This is what keeps the Git-for-Windows bootstrap above from moving the target out from under it: that same guard condition means a first pull into `.bashrc` leaves the exact state that makes the next login shell auto-generate a `~/.bash_profile` sourcing it, and without following that forwarding relationship the next pull would prefer the newly-created file and inject a second block there, leaving the original — still working, just loaded further away — reported as a dead leftover. The same reasoning covers a plain `.profile` that flat-guards a source of `.bashrc` for interactive shells (`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`), two hops from whatever a login shell reads first. -Only a reference this can verify without running a real shell counts, though — an unquoted `~/name` or an unquoted-or-double-quoted `$HOME/name` (never a quoted `~`, never a single-quoted `$HOME`: a shell does not expand either, so a reference that looks right there would source a literal, nonexistent path): a bare `source`/`.` command, on its own or as the always-attempted left side of an `&&` or `||`; the specific self-referential guard `test -f X && . X` / `[ -f X ] && . X` (tested and sourced path the same file, the one `&&` condition this can verify by visiting that file itself — further `&&`-chained commands after the guarded source don't change whether it ran, so they don't affect the match either); or an operand of an `A || B || C || ...` fallback chain, but only when every operand before it is a recognized source whose own target does not exist on disk, the one case that operand is guaranteed to be reached. Nothing inside an `if`, `for`/`while`/`until`, `case`, a function body, or a `(...)` subshell counts, however it's guarded — none of those are guaranteed to run (a subshell's body may always run, but its exports never reach the caller either way) and there is no way to verify one without running a real shell, which also means the standard Debian/Ubuntu `.profile` template (the same source, but nested two `if`s deep, checking `$BASH_VERSION` on the way) is not recognized and falls back to the order-based pick. Nothing textually after an unconditional, top-level `return` or `exit` counts either, since control never reaches it. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. +Only two literal line shapes count as a real reference, though: a bare `source X` / `. X` on a line by itself, or the exact self-referential existence guard Git for Windows itself generates, `test -f X && . X` / `[ -f X ] && . X` (tested and sourced path the same file), also on a line by itself — in both cases `X` must be an unquoted `~/name` or an unquoted-or-double-quoted `$HOME/name` (never a quoted `~`, never a single-quoted `$HOME`: a shell does not expand either, so a reference that looks right there would source a literal, nonexistent path). Anything else — a trailing redirection or extra argument on the source itself, an `||` fallback, an unrelated `&&`-chained command, a condition this can't independently verify — is not recognized, and falls back to the order-based pick rather than being guessed at. This is a deliberately narrow, closed set of two forms rather than an attempt to parse arbitrary shell conditionals: matching everything a real shell script could do to make a line conditional (or to disguise one as inert text) needs an actual shell parser, and no fixed-size grammar ever finishes that job. Nothing inside an `if`, `for`/`while`/`until`, `case`, `select`, a function body, or a `(...)`/`{...}` group counts, however it's guarded — none of those are guaranteed to run (a subshell or brace group's body may always run, but its exports never reach the caller either way) — which also means the standard Debian/Ubuntu `.profile` template (the same source, but nested two `if`s deep, checking `$BASH_VERSION` on the way) is not recognized and falls back to the order-based pick. Nothing textually after an unconditional, top-level `return` or `exit` counts either, since control never reaches it. Anything this can't resolve one way or the other, and a block sitting in a candidate nothing in the chain actually reaches, is never preferred over the order-based pick — otherwise a stale block left by a pre-#682 install would outrank the correct file forever, silently reintroducing #682 on upgrade. `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. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index bcd765bc..773d387a 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -720,7 +720,7 @@ teamai push 每次 pull 都会重新走一遍这个优先级判断，找到当前环境实际会读取的那个文件，然后沿着它对另外四个候选文件名的引用一路查下去——无论要经过多少跳——寻找一个已经带着代码块的候选文件，而不是重复注入。如果链条中间经过的是这五个候选文件名之外的文件（比如某些环境会改用 `~/.config/shell/profile` 这类自定义文件来 source），这条链就不会被继续跟踪。这正是为了不让 Git for Windows 自身的引导逻辑把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 写入 `.bashrc` 之后同样会成立，于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`；如果不沿着这条转发链去找，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是绕得更远了——则会被误报为失效的遗留代码块。同样的道理也适用于一个普通的 `.profile`：它用一条扁平的存在性守卫（`[ -f "$HOME/.bashrc" ] && . "$HOME/.bashrc"`）为交互式 shell source `.bashrc`——这时登录 shell 最先读到的文件，离实际代码块有两跳之遥。 -不过，只有这段扫描逻辑能在不真正运行 shell 的前提下确认的引用才算数——不加引号的 `~/name`，或不加引号/用双引号包裹的 `$HOME/name`（绝不是加了引号的 `~`，也绝不是用单引号包裹的 `$HOME`：shell 不会展开这两种写法，看起来对的引用实际会 source 一个不存在的字面路径）：一条裸的 `source`/`.` 命令，单独出现，或者作为 `&&`/`||` 里总会被尝试的左侧；形如 `test -f X && . X` / `[ -f X ] && . X` 这种被测试路径与被 source 路径完全相同的存在性守卫（这是唯一能靠亲自访问该文件来验证的 `&&` 条件——守卫之后如果还用 `&&` 接了别的命令，并不影响被守卫的 source 是否执行，所以也不影响这里的判断）；或者 `A || B || C || ...` 这种回退链条里的某一项——只有当它前面的每一项都是能识别的 source、且各自指向的文件在磁盘上确实不存在时才算数，因为那是这一项唯一能确定一定会被执行到的情形。嵌在 `if`、`for`/`while`/`until`、`case`、函数体，或者 `(...)` 子 shell 里的内容一律不算数，不管外层条件写的是什么——这些结构要么不保证一定会执行，要么即使一定会执行（比如子 shell），它导出的环境变量也传不到调用它的 shell 里；而不真正运行 shell 就没有办法验证前一种情况，这也意味着 Debian/Ubuntu 标准模板里那种嵌套两层 `if`、沿途还检查 `$BASH_VERSION` 的写法无法被识别，会回退到按优先级选出的文件。位于无条件的顶层 `return` 或 `exit` 之后的内容同样不算数，因为控制流根本不会执行到那里。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 +不过，只有两种字面写法才算真正的引用：单独一行的裸 `source X` / `. X`，或者单独一行、与 Git for Windows 自己生成的写法完全一致的自引用存在性守卫 `test -f X && . X` / `[ -f X ] && . X`（被测试路径与被 source 路径完全相同）——两种情况下 `X` 都必须是不加引号的 `~/name`，或不加引号/用双引号包裹的 `$HOME/name`（绝不是加了引号的 `~`，也绝不是用单引号包裹的 `$HOME`：shell 不会展开这两种写法，看起来对的引用实际会 source 一个不存在的字面路径）。除此之外的写法——source 本身带了重定向或额外参数、`||` 回退、不相关的 `&&` 连接命令、任何这段逻辑无法独立验证的条件——一律不识别，直接回退到按优先级选出的文件，而不是去猜。这是刻意收窄到两种固定写法的封闭集合，而不是尝试解析任意的 shell 条件：真要匹配一个真实 shell 脚本能用来让某一行变成有条件执行（或者把可执行内容伪装成惰性文本）的所有手法，需要一个真正的 shell 解析器，任何固定规模的规则集合都不可能穷尽这件事。嵌在 `if`、`for`/`while`/`until`、`case`、`select`、函数体，或者 `(...)`/`{...}` 分组里的内容一律不算数，不管外层条件写的是什么——这些结构要么不保证一定会执行，要么即使一定会执行（比如子 shell 或大括号分组），它导出的环境变量也传不到调用它的 shell 里，这也意味着 Debian/Ubuntu 标准模板里那种嵌套两层 `if`、沿途还检查 `$BASH_VERSION` 的写法无法被识别，会回退到按优先级选出的文件。位于无条件的顶层 `return` 或 `exit` 之后的内容同样不算数，因为控制流根本不会执行到那里。凡是这套逻辑判断不了的情况，以及当前这条链条根本没触及到的候选文件——哪怕它本身带着代码块——都绝不会因此被优先选中，否则 #682 之前旧版本留下的失效代码块就会永远压过正确的文件，等于在升级后又悄悄把 #682 引入回来。 `doctor`（以及 `pull` 结束后自动运行的检查）还会标记出遗留在*其他*候选文件中的 teamai 环境变量块——例如 #682 之前的旧版本写入 `.bashrc` 的代码块，即便该代码块本身已损坏、从未生效。`teamai uninstall` 会清理它。 diff --git a/src/__tests__/shell-profile.test.ts b/src/__tests__/shell-profile.test.ts index 819b1dcd..947cbc9c 100644 --- a/src/__tests__/shell-profile.test.ts +++ b/src/__tests__/shell-profile.test.ts @@ -229,39 +229,23 @@ describe('resolveActiveShellProfile', () => { expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); }); - // Regression (#693 review round 10): splitting on `||` treated its - // right-hand side as unconditionally reached, but it only runs if the left - // side fails — undetermined here. A stale block behind `||` must not win - // over a working one the left side already reaches. - it('does not treat the right side of || as reachable', async () => { - // Both .profile and .bashrc carry a valid block for this scope; the - // point is which one the resolver *reaches* through the || line, not - // which one has a well-formed block. .bashrc sorts earlier than - // .profile in SHELL_PROFILE_CANDIDATE_NAMES, so a naive "any referenced - // candidate in priority order" search would wrongly land on .bashrc even - // though it only runs if the left side (.profile) fails. + // Regression (#693 review round 15): the scanner no longer recognizes `||` + // fallback chains at all — round 14's N-way generalization, and round 11's + // two-operand version before it, kept needing another regex for another + // adversarial shape (a target that exists but fails to source, mixed + // `&&`/`||`, three-plus operands...). Recognizing only the two literal, + // common forms (bare source, self-referential existence guard) means a + // `||` line of any shape now falls back to the order-based pick — a + // harmless duplicate block, never a wrong one. + it('does not treat either side of a || fallback as reachable', async () => { await fse.writeFile( path.join(homeDir, '.bash_profile'), 'source ~/.profile || source ~/.bashrc\n', ); - await fse.writeFile(path.join(homeDir, '.profile'), teamaiBlock()); - await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); - expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.profile')); - }); - - // Regression (#693 review round 11): the right side of `||` genuinely is - // guaranteed to run when the left side's own target file does not exist — - // the one case this scanner can verify without a real shell. Failing to - // recognize it falls back to injecting a duplicate, which round 10's fix - // was meant to avoid for exactly this shape of line. - it('does treat the right side of || as reachable when the left side\'s target is missing', async () => { - await fse.writeFile( - path.join(homeDir, '.bash_profile'), - 'source ~/.profile || source ~/.bashrc\n', - ); - // .profile is deliberately absent. + // .profile is deliberately absent, so a real shell would reach .bashrc — + // out of scope now regardless. await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); - expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); // Regression (#693 review round 11): `&&` only establishes reachability @@ -312,13 +296,14 @@ describe('resolveActiveShellProfile', () => { expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); }); - // Regression (#693 review round 12): `&&`'s left side is always attempted, - // the same as `||`'s — a trailing unrelated command after it (`&& echo - // ready`) does not make the source itself conditional. - it('treats the left side of && as reachable even when the right side is unrelated', async () => { + // Regression (#693 review round 15): only the self-referential existence + // guard is recognized as an `&&` form now — a bare source followed by an + // unrelated `&&`-chained command is no longer a special case, it is just a + // line that is not one of the two recognized forms, so it falls back. + it('does not treat a bare source followed by an unrelated && command as reachable', async () => { await fse.writeFile(path.join(homeDir, '.bash_profile'), 'source ~/.bashrc && echo ready\n'); await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); - expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); // Regression (#693 review round 12): only `if` nesting was tracked, so a @@ -379,13 +364,19 @@ describe('resolveActiveShellProfile', () => { expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); }); - it('treats an existence-gated source as reachable even with further &&-chained commands', async () => { + // Regression (#693 review round 15): the existence guard is only + // recognized as a complete line now — round 13's tolerance for further + // `&&`-chained commands after it is out of scope again, since it was part + // of the general trailing-content handling that also caused the pipe/ + // background false positive below. A guard with anything appended falls + // back to the order-based pick. + it('does not treat an existence-gated source with further &&-chained commands as reachable', async () => { await fse.writeFile( path.join(homeDir, '.bash_profile'), '[ -f ~/.bashrc ] && . ~/.bashrc && export READY=1\n', ); await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); - expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); it('does not treat a source inside a comment after a semicolon as reachable', async () => { @@ -442,30 +433,42 @@ describe('resolveActiveShellProfile', () => { expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); - it('treats a source with a trailing redirection as reachable', async () => { + // Regression (#693 review round 15): a bare source is only recognized as + // its own complete line now — trailing content of any kind (a redirection, + // a pipe, a background `&`) makes it not one of the two recognized forms, + // so it falls back rather than trying to reason about what the trailing + // content does to reachability. + it('does not treat a source with a trailing redirection as reachable', async () => { await fse.writeFile( path.join(homeDir, '.bash_profile'), 'source ~/.bashrc 2>/dev/null\n', ); await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); - expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); - it('treats the last operand of a three-way || fallback as reachable when the earlier ones are missing', async () => { + // Regression (#693 review round 15): `source ~/.bashrc | cat` and + // `source ~/.bashrc &` both run the source in a subshell — a pipeline + // member and a backgrounded job never propagate exports to the login + // shell — so trusting trailing content indiscriminately (as a prior round + // did, to recognize legitimate extra arguments and redirections) actively + // reintroduced a false "reachable" for these. The strict, line-only match + // rejects all trailing content uniformly, closing this without needing to + // special-case which trailing forms are safe. + it('does not treat a source piped or backgrounded as reachable', async () => { await fse.writeFile( path.join(homeDir, '.bash_profile'), - 'source ~/.profile || source ~/.bash_login || source ~/.bashrc\n', + 'source ~/.bashrc | cat\nsource ~/.bashrc &\n', ); await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); - expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); - it('does not treat the last operand of a three-way || fallback as reachable when an earlier one exists', async () => { + it('does not treat either side of a three-way || fallback as reachable', async () => { await fse.writeFile( path.join(homeDir, '.bash_profile'), 'source ~/.profile || source ~/.bash_login || source ~/.bashrc\n', ); - await fse.writeFile(path.join(homeDir, '.profile'), '# unrelated\n'); await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); @@ -478,6 +481,86 @@ describe('resolveActiveShellProfile', () => { await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); }); + + // Regression (#693 review round 15): `<< { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'cat << { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'cat < { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'cat < { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '[ "$TERM_PROGRAM" = vscode ] &&\nsource ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('recognizes an existence guard split across an operator-end continuation', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '[ -f ~/.bashrc ] &&\n. ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); + + it('does not treat a source inside a select body as reachable', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + 'select opt in a b; do\n . ~/.bashrc\ndone\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bash_profile')); + }); + + it('does not stop recognizing later unconditional sources after a compound closer with a redirection', async () => { + await fse.writeFile( + path.join(homeDir, '.bash_profile'), + '(\n echo hi\n) >/dev/null\nsource ~/.bashrc\n', + ); + await fse.writeFile(path.join(homeDir, '.bashrc'), teamaiBlock()); + expect(await resolveActiveShellProfile(envShPath, 'win32')).toBe(path.join(homeDir, '.bashrc')); + }); }); describe('envBlockSourcesPath', () => { diff --git a/src/utils/shell-profile.ts b/src/utils/shell-profile.ts index a5b1f293..94eb2da2 100644 --- a/src/utils/shell-profile.ts +++ b/src/utils/shell-profile.ts @@ -202,7 +202,6 @@ export function envBlockReferencesDataHome(block: string, envShPath: string): bo return false; } -/** `~/name`, `$HOME/name` or `${HOME}/name`, optionally quoted, as a token this scanner accepts as a reference to `name`. */ /** * `~/name` (never quoted — a shell does not tilde-expand inside any quotes) * or `$HOME/name` / `${HOME}/name` (unquoted or double-quoted — a shell @@ -218,108 +217,76 @@ function homeRelativeRef(name: string): string { return `(?:~/${suffix}|\\$\\{?HOME\\}?/${suffix}|"\\$\\{?HOME\\}?/${suffix}")`; } -/** If `token` is a home-relative reference by the same rule `homeRelativeRef` accepts, the real path it names. */ -function homeRelativePath(token: string, home: string): string | null { - let m = token.match(/^~\/(.+)$/); - if (!m) m = token.match(/^\$\{?HOME\}?\/(.+)$/); - if (!m) m = token.match(/^"\$\{?HOME\}?\/(.+)"$/); - return m ? path.join(home, m[1]) : null; -} - -/** Whether `statement` opens, or closes, a construct whose body is not guaranteed to run — `if`, `for`/`while`/`until`, `case`, a function/brace group, or a `(...)` subshell (whose exports never reach the caller even when its body always runs — #693 review round 14). */ -function opensUnverifiedBlock(statement: string): boolean { - return /^(?:if|for|while|until|case)\b/.test(statement) - || /^function\s+\S/.test(statement) - || /^\S+\s*\(\)\s*\{?\s*$/.test(statement) - || statement === '{' - || statement === '('; -} -function closesUnverifiedBlock(statement: string): boolean { - return /^(?:fi|done|esac)\b/.test(statement) || statement === '}' || statement === ')'; -} - -/** Strips a trailing shell comment (`#` at the start of a word, outside this scanner's quote-naive view) from `line`. */ -function stripComment(line: string): string { - const at = line.search(/(?:^|\s)#/); - if (at === -1) return line; - return line.slice(0, line.indexOf('#', at)).trimEnd(); -} - /** - * Splits `s` on every top-level occurrence of `sep`, skipping any that fall - * inside single or double quotes — the naive `.split(sep)` this replaces - * would otherwise cut a quoted argument in half, e.g. treating the `;` in - * `printf '%s' 'x; source ~/.bashrc; y'` as a real statement separator and - * inventing an executed `source` that was actually just string data (#693 - * review round 14). No escape handling beyond that (matching this scanner's - * existing quote-naive view elsewhere) — good enough to stop a quoted - * separator from being mistaken for a real one, not a full shell lexer. + * Whether `line` opens, or closes, a construct whose body either isn't + * guaranteed to run (`if`/`for`/`while`/`until`/`case`/`select`, a function) + * or runs in a subshell whose exports never reach the caller even when it + * always runs (`(...)`, a brace group) — content inside never counts as + * reaching a candidate, no matter how it looks (#693 review rounds 11-15). */ -function splitTopLevel(s: string, sep: string): string[] { - const parts: string[] = []; - let current = ''; - let quote: string | null = null; - for (let i = 0; i < s.length; ) { - const ch = s[i]; - if (quote) { - current += ch; - if (ch === quote) quote = null; - i += 1; - continue; - } - if (ch === '"' || ch === "'") { - quote = ch; - current += ch; - i += 1; - continue; - } - if (s.startsWith(sep, i)) { - parts.push(current); - current = ''; - i += sep.length; - continue; - } - current += ch; - i += 1; - } - parts.push(current); - return parts; +function opensUnverifiedBlock(line: string): boolean { + return /^(?:if|for|while|until|case|select)\b/.test(line) + || /^function\s+\S/.test(line) + || /^\S+\s*\(\)\s*\{?\s*$/.test(line) + || line === '{' + || line === '('; +} +function closesUnverifiedBlock(line: string): boolean { + return /^(?:fi|done|esac)\b/.test(line) || /^\}(?:\s|$)/.test(line) || /^\)(?:\s|$)/.test(line); } /** - * Splits `content` into logical lines: strips comments, joins `\`-continued - * lines, joins a lone `{` onto the function/construct header it opens (`fn()` - * then `{` on its own line), and drops heredoc bodies entirely (their text is - * data, never executed statements — #693 review round 13), tracking every - * terminator in order when a single command opens more than one heredoc - * (`cat < 0) { - if (rawLines[i].trim() === heredocQueue[0]) heredocQueue.shift(); + const { terminator, stripTabs } = heredocQueue[0]; + const withoutCR = rawLines[i].replace(/\r$/, ''); + const candidate = stripTabs ? withoutCR.replace(/^\t+/, '') : withoutCR; + if (candidate === terminator) heredocQueue.shift(); continue; } - let line = stripComment(rawLines[i]).trim(); - while (line.endsWith('\\') && !line.endsWith('\\\\')) { + let line = rawLines[i].trim(); + while (i + 1 < rawLines.length && ( + (line.endsWith('\\') && !line.endsWith('\\\\')) || line.endsWith('&&') || line.endsWith('||') + )) { i += 1; - line = `${line.slice(0, -1).trimEnd()} ${(i < rawLines.length ? stripComment(rawLines[i]) : '').trim()}`.trim(); + const next = rawLines[i].trim(); + line = line.endsWith('\\') ? `${line.slice(0, -1).trimEnd()} ${next}`.trim() : `${line} ${next}`.trim(); } if (!line) continue; - for (const heredoc of line.matchAll(/<<-?\s*(['"]?)(\w+)\1/g)) { - heredocQueue.push(heredoc[2]); - } + // A self-contained one-liner (`if ...; then ...; fi`, `case ... esac`, + // `for ...; do ...; done`) opens and closes on the same line — net zero + // depth change, not an unclosed open that corrupts tracking for every + // real statement after it (#693 review round 13/15). + if (/^(?:if|for|while|until|case)\b.*;\s*(?:fi|done|esac)\s*$/.test(line)) continue; if (line === '{' && result.length > 0) { result[result.length - 1] += ' {'; continue; } + + for (const heredoc of line.matchAll(/(? { +async function referencesCandidate(content: string, name: string): Promise { const ref = homeRelativeRef(name); - const refOnly = new RegExp(`^${ref}$`); - const existenceGuard = new RegExp(`^(?:test\\s+-f\\s+${ref}|\\[\\s+-f\\s+${ref}\\s*\\])$`); - // The target is the first whitespace-run-delimited argument; anything after - // it (extra positional args passed to the sourced script, a redirection - // like `2>/dev/null`) doesn't change whether the source itself runs (#693 - // review round 14). - const sourceOf = /^(?:\.|source)\s+(\S+)(?:\s+\S.*)?$/; + const bareSource = new RegExp(`^(?:\\.|source)\\s+${ref}$`); + const existenceGuard = new RegExp( + `^(?:test\\s+-f\\s+${ref}|\\[\\s+-f\\s+${ref}\\s*\\])\\s*&&\\s*(?:\\.|source)\\s+${ref}$`, + ); let depth = 0; let halted = false; for (const line of logicalLines(content)) { - for (const statement of splitTopLevel(line, ';').map((s) => s.trim()).filter(Boolean)) { - if (opensUnverifiedBlock(statement)) { depth += 1; continue; } - if (closesUnverifiedBlock(statement)) { depth = Math.max(0, depth - 1); continue; } - if (depth > 0) continue; - - // `return`/`exit`, unconditional and at top level, ends this file's - // control flow right there — nothing textually after it, however it - // looks, ever runs (#693 review round 14). - if (/^(?:return|exit)(?:\s+\S+)?$/.test(statement)) { halted = true; continue; } - if (halted) continue; - - // Existence-gated `&&`: `test -f REF && . REF`, self-referential, with - // any further `&&`-chained commands after it not affecting whether the - // guarded source itself ran (#693 review round 13). - const andParts = splitTopLevel(statement, '&&').map((s) => s.trim()); - if (andParts.length >= 2 && existenceGuard.test(andParts[0])) { - const guarded = andParts[1].match(sourceOf); - if (guarded && refOnly.test(guarded[1])) return true; - } + if (opensUnverifiedBlock(line)) { depth += 1; continue; } + if (closesUnverifiedBlock(line)) { depth = Math.max(0, depth - 1); continue; } + if (depth > 0) continue; - // `A || B || C || ...`: each operand is reached only if every operand - // before it is a recognized source of a target verifiably missing from - // disk (the one way a `||` fallback is guaranteed to run) — the - // leftmost is always attempted regardless. An operand this can't - // resolve one way or the other stops the chain from being trusted any - // further (#693 review round 14 generalized this past two operands). - const orParts = splitTopLevel(statement, '||').map((s) => s.trim()); - if (orParts.length >= 2) { - let reachable = true; - for (const part of orParts) { - const m = part.match(sourceOf); - if (reachable && m && refOnly.test(m[1])) return true; - if (!reachable) break; - const p = m && homeRelativePath(m[1], home); - reachable = !!p && !(await pathExists(p)); - } - continue; - } + if (/^(?:return|exit)(?:\s+\S+)?$/.test(line)) { halted = true; continue; } + if (halted) continue; - // The leftmost command before the first `&&` (if any) is always - // attempted, same as `||`'s left side above — only its right side's - // extra condition is unverifiable in general. - const andLeft = andParts[0].match(sourceOf); - if (andLeft && refOnly.test(andLeft[1])) return true; - } + if (bareSource.test(line) || existenceGuard.test(line)) return true; } return false; } @@ -473,7 +397,7 @@ export async function resolveActiveShellProfile( for (const name of SHELL_PROFILE_CANDIDATE_NAMES) { const candidate = path.join(home, name); - if (candidate !== current && !visited.has(candidate) && await referencesCandidate(content, name, home)) { + if (candidate !== current && !visited.has(candidate) && await referencesCandidate(content, name)) { queue.push(candidate); } }