Skip to content

test(devin-cli): anchor the empty-data-dir fallback at the host home - #4384

Draft
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/windows-cli-fixtures-20260912
Draft

test(devin-cli): anchor the empty-data-dir fallback at the host home#4384
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/windows-cli-fixtures-20260912

Conversation

@luvs01

@luvs01 luvs01 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Summary

The Windows lane fails tests/providers/devin-cli-login.test.ts on current dev. The empty-XDG_DATA_HOME case added with #4418 asserts that the resolved credential path starts with /, but the fallback deliberately resolves against the real home directory, and homedir() returns C:\Users\<name> regardless of which platform the resolver is asked about. The assertion therefore only holds when the host is POSIX.

Anchor both branches at homedir() instead. That is the property the test exists to prove — the path is not cwd-relative — and it holds on either host. The win32 branch gains the same anchor assertion, which it previously left implicit. Production behavior is unchanged.

Scope change from the original PR

This PR originally carried the Devin path-separator fixture repair plus a pnpm shim fixture repair. Both are now obsolete:

  • The Devin separator problem was fixed upstream by 293c37d68 (fix(devin-cli): resolve credential paths for the selected platform), which made the POSIX expectations correct.
  • tests/providers/devin-cli-adapter.test.ts no longer exists; 213065e30 retired the ACP adapter.
  • The pnpm shim fixtures landed in test(update): use host-native pnpm shim fixtures #4379.

Rather than close this and open a third PR for the same file, I rebuilt the branch on dev@dcd13b435 with only the one remaining Windows failure fixed. The head is a single commit; the previous two-commit series is gone.

Verification

  • Reproduced first on current dev: 17 pass, 1 fail — an empty XDG_DATA_HOME or APPDATA does not become a cwd-relative path at tests/providers/devin-cli-login.test.ts:155, expected true, received false.
  • After the change: bun test ./tests/providers/devin-cli-login.test.ts18 pass, 0 fail, 34 assertions, Bun 1.4.2 on Windows.
  • bun run typecheck, bun run structure:check, bun run privacy:scan, git diff --check — all passed.
  • The change is portable in the other direction too: on a POSIX host homedir() is /home/<name>, so the anchor assertion holds for both the POSIX and the win32 branch.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. Not needed for a test-only correction.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. The assertion still proves the anti-cwd-relative property the original hardening added.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

Rebase onto dev@dcd13b435 (2026-09-13)

Current head cba37f348ef4d85295bd2f4dbdcc96e5d7b4eda7. The shared baseline failures that kept every authored head red have now landed on dev: Cline inventories via #4390, pnpm shim fixtures via #4379, and the native restore fixtures via #4380. This head is rebased past all three, so its cross-platform result is finally about this change alone.

Fresh checks on this exact head:

  • bun run typecheck — passed.
  • bun run structure:check — passed.
  • bun run privacy:scan — passed.
  • bun test --isolate --timeout 60000 ./tests/providers/devin-cli-login.test.ts — passed.
18 pass
 0 fail
Ran 18 tests across 1 file. [1485.00ms]

A full cross-platform matrix is running for this head at run 34724811731. The readiness boxes below are ticked only against its actual conclusion, so they are updated after it finishes rather than from these focused passes.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 54 / 80

설명

이 PR은 Windows에서 깨지는 테스트 픽스처만 고칩니다. 프로덕션 코드는 건드리지 않고 tests/providers/devin-cli-adapter.test.ts, tests/providers/devin-cli-login.test.ts, tests/update/update-pnpm.test.ts 세 파일만 바꿉니다. 지금 dev HEAD는 d42a1363d (#4368 Codex 거절 계정이 다음 픽을 이기지 못하게 한 직후, package.json 2.52.0)이고, 이 PR 본문 base는 그보다 한 칸 뒤인 489af939b입니다. 캐시/ Hermes/ accounts 레인과는 겹치지 않는 CI 수리 조각입니다.

무엇이 깨졌냐면, Devin 경로 기대값이 리눅스식 슬래시 문자열(/home/u/.local/bin/devin, /d/devin/credentials.toml)로 박혀 있어서 Windows 호스트의 path.join 결과와 달라지고, pnpm 글로벌 shim 테스트는 POSIX 셸 스크립트에 실행 비트를 심은 뒤 platform: "linux"로 검증해서 NTFS/Windows CI에서 실패합니다. 이 PR은 Devin 쪽을 join(...)로 맞추고, pnpm 쪽은 writeHostShims로 win32면 .cmd/.ps1, 그 외면 실행 비트 있는 shell shim을 만든 뒤 process.platform으로 검증합니다. 절대 credential override, discovery 우선순위, stale shim 거부, group alias는 유지한다고 본문에 적혀 있고, Spark 은퇴(#4334)는 가져오지 않는다고 선을 긋습니다.

같은 날 열린 #4379(test(update): use host-native pnpm shim fixtures)도 tests/update/update-pnpm.test.ts의 host-native shim 헬퍼를 고칩니다. 두 PR이 pnpm 픽스처를 동시에 고치면 나중에 한쪽이 conflict나 중복 머지가 됩니다. Devin 경로 join은 #4379 범위 밖이라 #4384만의 가치가 있고, pnpm 부분은 사실상 형제 수리입니다. draft이고 mergeable은 MERGEABLE이지만 mergeStateStatus는 BLOCKED(체크 대기)입니다. types.ts/config.ts 분리 캠페인과는 무관합니다.

라인 - 이게 무슨 문제다

tests/update/update-pnpm.test.ts / writeHostShims - #4379와 같은 파일·같은 헬퍼 계열을 고칩니다. 둘 다 머지하면 충돌하거나 한쪽이 중복이 됩니다. pnpm 수리는 한쪽으로 모으는 편이 낫습니다.

tests/providers/devin-cli-adapter.test.ts / join("/home/u", ".local", "bin", "devin" - home 인자는 여전히 POSIX 형태 문자열이지만, 기대값만 host separator로 맞춥니다. Windows에서 home 자체가 다른 형태여도 exists 콜백이 같은 문자열을 받으니 테스트 의도는 유지됩니다. 다만 “진짜 Windows home 레이아웃”까지 검증하진 않습니다.

tests/providers/devin-cli-login.test.ts 절대 override /tmp/creds.toml - 리눅스 플랫폼 인자로 돌릴 때 절대 경로 문자열을 그대로 기대합니다. Windows 전용 절대 경로 케이스는 이 diff에 없어서, Devin Windows APPDATA 케이스와 역할이 나뉩니다. 범위는 괜찮습니다.

본문 Verification - 로컬에서 Devin/pnpm 55 passed를 적었지만 hosted Cross-platform CI는 pending입니다. draft 체크리스트도 아직 ready가 아닙니다.

메인테이너의 판단이 필요한 지점

  • pnpm shim 수리를 #4379에 맡기고 이 PR은 Devin join만 남길지, 반대로 이 PR로 pnpm까지 흡수하고 #4379를 닫을지
  • Windows CI 초록을 이 draft 단독으로 볼지, Combo tip에 묶을지
  • Spark/#4334 픽스처를 이 PR에 더 실을지(본문은 의도적으로 제외)

너의 추천
Devin 경로 join은 바로 가져갈 만합니다. pnpm 부분은 #4379와 겹치니 하나를 owner로 정하고 다른 쪽은 close-or-trim 하세요. Cross-platform(특히 Windows) CI가 초록이면 draft 해제 후 머지, 그 전에는 draft 유지. 프로덕션 미변경이라 위험은 낮습니다.

이 댓글은 grok-bot이 작성했습니다

@luvs01
luvs01 force-pushed the agent/windows-cli-fixtures-20260912 branch from 75c7c16 to 4283b95 Compare September 12, 2026 07:28
@luvs01 luvs01 changed the title test: make Devin and pnpm fixtures portable on Windows test: make Devin path fixtures portable on Windows Sep 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@luvs01

luvs01 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head CI for 4283b95 completed with failure: https://github.com/luvs01/opencodex/actions/runs/34681186783

The three Devin fixture failures are absent from this run; both retained files also passed locally (29 tests, 95 assertions). Remaining recurring assertion groups correspond to Cline inventories (#4386), injection/journal fixtures (#4380), and pnpm shim fixtures (#4379).

One additional failure is explicitly unresolved: cli-start-journal-order.test.ts, startup preserves only a client journal matching the final committed api key id, exceeded its 30,000 ms budget in Windows job 103520318533. Slow adjacent cases make load sensitivity plausible, but do not establish the cause. My earlier categorization as runner timing was too strong. This is outside the two modified Devin test files; it remains a follow-up observation, not a passing or resolved test. Draft and CI checkboxes remain unchanged.

@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (2/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 2/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

2/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@luvs01
luvs01 force-pushed the agent/windows-cli-fixtures-20260912 branch from 4283b95 to 3314465 Compare September 12, 2026 09:53
The fallback resolves against the real home directory, so asserting a leading slash only held on a POSIX host and failed the Windows lane. Anchor both branches at homedir(), which is what proves the path is not cwd-relative on either host.
@luvs01
luvs01 force-pushed the agent/windows-cli-fixtures-20260912 branch from 3314465 to cba37f3 Compare September 12, 2026 23:12
@luvs01 luvs01 changed the title test: make Devin path fixtures portable on Windows test(devin-cli): anchor the empty-data-dir fallback at the host home Sep 12, 2026
@luvs01

luvs01 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

This PR was rebuilt on dev@dcd13b435 and its scope is now a single test-only commit. The original two-commit series is gone, so the two questions from the earlier review are resolved by construction rather than by argument:

  • The pnpm shim fixtures are no longer touched here. test(update): use host-native pnpm shim fixtures #4379 owns that repair and has landed, so there is nothing left to deduplicate.
  • tests/providers/devin-cli-adapter.test.ts no longer exists. 213065e30 retired the ACP adapter, and 293c37d68 fixed the credential path resolution that the old separator expectations were working around.

What remains is one real Windows failure on current dev. In tests/providers/devin-cli-login.test.ts:155, the empty-XDG_DATA_HOME case asserts resolved.startsWith("/"), but the fallback resolves against the real home directory, and homedir() returns C:\Users\<name> no matter which platform the resolver is asked about. The assertion only holds when the host itself is POSIX. Anchoring both branches at homedir() keeps the property the test exists to prove — the path never becomes cwd-relative — and it holds on either host.

This is worth knowing beyond this PR: the same failure is currently red on the Windows lane of every head rebased onto dcd13b435. A cross-platform matrix on this fork shows (fail) devin-cli credential path and read bounds > an empty XDG_DATA_HOME or APPDATA does not become a cwd-relative path in the windows 1/6 shard of otherwise unrelated heads, for example run 34724696111. A baseline matrix on unmodified dcd13b435 is running at run 34725771988 to record that the failure belongs to dev and not to any individual change.

Verification on this head: reproduced 17 pass / 1 fail on dev, then 18 pass / 0 fail after the change, with bun run typecheck, bun run structure:check and bun run privacy:scan also passing. Production code is untouched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants