test(devin-cli): anchor the empty-data-dir fallback at the host home - #4448
Conversation
Carry #4384 from fdba29b onto the current dev tip. The empty-XDG_DATA_HOME case asserted the resolved path starts with "/", which is false on a Windows runner: devinCliCredentialsPath picks path.win32 or path.posix from its platform ARGUMENT, but the fallback joins the HOST homedir(), so asking about "linux" from Windows yields C:\Users\<name>/.local/share/devin/credentials.toml. Anchor both crossings at that home directory instead, normalising through win32.join for the win32 branch so a POSIX host home compares against the same separators. Anchoring, not a leading slash, is what proves the path is not cwd-relative, which is the defect the test exists to pin. The surviving endsWith assertions and the startsWith("devin") === false check still prove it. Production behaviour is unchanged; this is test-only. Local product tests, typecheck, build and install: NOT RUN. Hosted exact-head CI on this PR is the merge proof. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe test updates fallback credential-path assertions in ChangesDevin CLI path test
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Other Merge Risk: ⚪ Minimal · up to This test-only update improves cross-platform fallback-path coverage without changing production behavior, so it is mergeable after normal CI checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
리뷰 · 우선순위 58 / 80설명 이 PR은 제품 코드를 바꾸지 않고, Devin CLI 자격 증명 경로 테스트만 고칩니다. 지금 그래서 Windows CI에서 본문은 #4384(@luvs01)를 현재 참고로 오늘 웨이크는 #4446 머지 알림이었지만, 그 커밋은 이미 이전 tests/providers/devin-cli-login.test.ts:155 근처 - 예전 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
Maintainer integration into
Supersedes #4384, whose author is credited by the |
Summary
Carries #4384 (@luvs01, head
fdba29bc1ae1cf262430764221312d729476a551) onto the currentdevtip.devinCliCredentialsPathpickspath.win32orpath.posixfrom its platform argument, but the empty-value fallback joins the hosthomedir(). On a Windows runner, asking about"linux"therefore returnsC:\Users\<name>/.local/share/devin/credentials.toml, and the test asserted a leading/:Anchor both crossings at the host home instead, normalising through
win32.joinfor the win32 branch so a POSIX host home is compared with matching separators. Anchoring is what actually proves the path is not cwd-relative, which is the defect this test pins; a leading slash only ever held because the host happened to be POSIX.Production behaviour is unchanged. Test-only.
Verification
src/oauth/devin-cli.ts:71whilehomedir()reflects the host; walked both host/platform crossings; confirmed the untouchedendsWithon the next line does not leave a second Windows failure, because the posix branch joins with forward slashes; and confirmedwin32.join(homedir())is a real normalisation rather than a single-argument no-op.homedir()were empty or/. The original cwd-relative defect stays proven by the survivingendsWithassertions and thestartsWith("devin") === falsecheck.Checklist
Maintainer integration under
MAINTAINERS.md:devonly, with exact-head CI evidence recorded before merge.Supersedes #4384.
Summary by CodeRabbit