Repository navigation
Conversation
|
Findings
|
STiFLeR7
left a comment
There was a problem hiding this comment.
Thanks, the HOME vs os.homedir() split is a real problem and the fix is mechanical and low-risk. I read the full diff against src/utils/home.ts and checked out the branch (Windows, Git Bash, npm ci, vitest).
-
The PR says "
os.homedir()now appears only insidehome.ts". That is not quite true on the branch head.src/hermes-home.ts:2,16still doesimport { homedir } from 'node:os'andpath.join(homedir(), '.hermes'). A grep foros.homedirmisses it because it is a named import (grep -rn homedir src --include=*.tsshows it). It is the same bug class: with onlyUSERPROFILEset, or Git Bash'sHOMEdiffering fromos.homedir(), teamai would look for~/.hermesin a different home than the rest of~/.teamai. Suggestpath.join(getUserHome(), '.hermes')there too.src/__tests__/hermes-config.test.tsmocksos.homedir, so it will need the sameHOMEhandling you added togitcode-provider.test.ts. -
Test teardown nit in
src/__tests__/user-home-resolution.test.ts(afterEach):process.env.HOME = savedHome;assigns the string"undefined"when HOME was unset at start (Node coerces; verified withprocess.env.X = undefined). HOME unset is exactly the native-PowerShell case this PR is about.savedCacheDiralready uses the=== undefined ? delete : assignpattern, so HOME should too. It only leaks within that test file's worker, so low severity. -
Coverage: the new tests exercise HOME vs os.homedir() only. The
USERPROFILE-only path (the Windows PowerShell case in the description) is not asserted, and nothing coverspush --skill ~/...(push.tsaround L1371 and L1394) orgfGetOAuthToken.push-skill-flag.test.ts"expands ~ in skill path" builds expectations fromos.homedir(). Optional follow-up, not a blocker.
Test run notes, for transparency: push-skill-flag.test.ts has 4 failures on this branch on Windows and 5 on current main (same cause, POSIX-style paths in mocks), so they are not caused by this PR. path-safety.test.ts passes (26/26).
Once 1 is addressed I would be happy to approve; the rest is optional.
|
Triage pass (Triage access only, no merge authority on my end) — this PR has 3 open P1-blocking findings from the bot with no response yet:
Also showing |
`getUserHome()` (HOME -> USERPROFILE -> os.homedir()) is the resolver the rest of teamai already uses for ~/.teamai state and credentials, but several call sites still expanded ~ with os.homedir() directly. On any machine where the two disagree -- a native Windows PowerShell session that exposes only USERPROFILE, or Git Bash on Windows, which sets HOME to a path os.homedir() never reports -- teamai wrote and read the same state in two different homes: - the GitCode/TGit netrc token was written to os.homedir()/.netrc while git reads $HOME/.netrc, so the stored credential was not the one git uses, and the provider reported unauthenticated; - the repo cache landed under os.homedir()/.teamai/cache/repos while the rest of ~/.teamai stayed under $HOME; - defaultAllowedRoots() allowed os.homedir(), so a path under $HOME -- where teamai had just written -- was judged outside every allowed root. Route every remaining expansion through getUserHome() so one machine has one home, and add regression tests that hold HOME and the platform home apart.
7c5b0b4 to
603e959
Compare
|
Findings
None of the three earlier findings is resolved in the current diff. No PR code was run, built, or installed. |
Summary
getUserHome()(HOME→USERPROFILE→os.homedir()) is the resolver the rest of teamai already uses for~/.teamaistate and credentials, but several call sites still expanded~withos.homedir()directly. On any machine where the two disagree — a native Windows PowerShell session that exposes onlyUSERPROFILE, or Git Bash on Windows, which setsHOMEto a pathos.homedir()never reports — the same state was written and read in two different homes. This routes every remaining expansion throughgetUserHome().Background
src/utils/home.tsdefines the canonical home —HOME→USERPROFILE→os.homedir(), throwing when none resolve — because a native Windows PowerShell session commonly exposes onlyUSERPROFILE, and Git Bash on Windows setsHOMEto a pathos.homedir()never reports. The rest of teamai already resolves~/.teamaistate and credentials throughgetUserHome(), but several call sites still expanded~withos.homedir()directly, so on any machine where the two disagree the same state is written and read in two different homes.The change
Every remaining
os.homedir()expansion now goes throughgetUserHome():src/providers/gitcode/gitcode-api.tsnetrcPath()—~/.netrcsrc/providers/tgit/gf-cli.tsgfGetOAuthToken()—~/.netrcsrc/utils/path-safety.tsresolveReal()anddefaultAllowedRoots()src/utils/repo-cache.ts,src/utils/cache-index.tsgetCacheRoot()src/push.tspush --skill ~/…(resolveRequestedSkill())os.homedir()now appears only insidehome.ts, where it is the documented last-resort fallback.Why it matters
writeNetrcToken()wroteos.homedir()/.netrcwhilegitreads$HOME/.netrcon POSIX — and%HOME%before%USERPROFILE%on Windows, the same ordergetUserHome()uses. The credential teamai stored was not the one git authenticated with, andreadNetrcToken()could return a different account's token from the other file.~/.teamai. Credentials and team-repo state live under$HOME/.teamai; the cache went toos.homedir()/.teamai/cache/repos.defaultAllowedRoots()returnedos.homedir(), so a path under$HOME— where teamai had just written — resolved outside every allowed root.Type of Change
Test Plan
New
src/__tests__/user-home-resolution.test.tsholds$HOMEand the platform home apart (a realHOMEwithos.homedir()mocked elsewhere) and asserts the netrc token, the cache root and the default allowed roots all land in$HOME. It fails onmainand passes with this change:src/__tests__/gitcode-provider.test.tsalso neededHOMEset in itsbeforeEach, becausegetUserHome()readsHOMEbefore theos.homedir()spy it already installed.npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpassesnpm run lintandnpx vitest runare left unticked because both hit host-specific problems on the machine this was prepared on, not findings in the change:npm run lintisoxlint --type-aware --deny-warnings --report-unused-disable-directives, and itstsgolintsubprocess could not spawn there (Failed to spawn tsgolint … os error 231). The non-type-aware pass was run instead and is clean:npx oxlint --deny-warnings --report-unused-disable-directives src/→ 0 warnings, 0 errors (895 files).vitestsuite has pre-existing host-specific failures on that machine (safe-delete shim and Windows path separators). For the files this PR touches, the failure set is byte-identical before and after —src/__tests__/push-skill-flag.test.tsreports the same 5 failures / 3 passes onmainand on this branch — with the 3 new tests inuser-home-resolution.test.tsas the only delta. On Linux CI, wherepath.joinuses/andHOMEagrees withos.homedir(), those cases are not affected.Related Issues
Fixes #1045
Notes for Reviewers
Rebased onto
main.mainmoved this code while the branch was open:push --skillnow resolves its path in a newresolveRequestedSkill()helper that still calledos.homedir(). The rebase keeps main's structure and moves the~expansion ontogetUserHome()there; the helper is now synchronous, since dropping the dynamicimport('node:os')removed its onlyawait.One deliberate non-change.
getUserHome()still returns$HOMEverbatim, so a Git Bash session on Windows that setsHOME=/c/Users/…gets a POSIX-style path. That behaviour is pre-existing and shared with every currentgetUserHome()caller — normalising MSYS paths would be a separate change. This PR only makes the remaining call sites agree with the resolver the project already standardised on, rather than introducing a third rule.Follow-ups worth a decision, not hidden here:
os.homedir()/.netrcare not migrated and not read as a fallback, so on a machine where the two homes differ a previously stored token becomes invisible until the user authenticates again. If a migration is wanted, it belongs innetrcPath()'s read path.getUserHome()'s POSIX-style value is passed straight intopath.join()on Windows, soHOME=/c/Users/aliceresolves to\c\Users\alice. The new tests setHOMEto a native path, so that combination is not yet pinned by a test.