test: isolate E2E LLM profiles store#1015
Conversation
Co-authored-by: smolpaws <engel@enyst.org>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds environment-variable driven LLM profiles directory resolution and E2E test setup: the SDK reads Changes
Sequence Diagram(s)(Skipped — changes are small and self-contained; no multi-component control-flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/suite/defaultProfilesSeeding.ts (1)
19-31: Environment variable priority differs from SDK'sresolveRootDir.The test helper checks
E2E_LLM_PROFILES_DIRfirst, thenOPENHANDS_LLM_PROFILES_DIR. However, the SDK'sresolveRootDirinpackages/agent-sdk/src/sdk/llm/profiles.ts(lines 44-48) checks them in the opposite order:OPENHANDS_LLM_PROFILES_DIRfirst, thenE2E_LLM_PROFILES_DIR.In practice this works because
extension.tssets both variables to the same value, but the inconsistent priority could cause confusion or subtle bugs if someone sets only one variable in the future.Consider aligning the priority order with the SDK:
♻️ Suggested alignment
const getProfilesDir = (): string => { - const fromEnv = typeof process.env.E2E_LLM_PROFILES_DIR === 'string' - ? process.env.E2E_LLM_PROFILES_DIR.trim() + const fromSdkEnv = typeof process.env.OPENHANDS_LLM_PROFILES_DIR === 'string' + ? process.env.OPENHANDS_LLM_PROFILES_DIR.trim() : ''; - if (fromEnv) return path.resolve(fromEnv); + if (fromSdkEnv) return path.resolve(fromSdkEnv); - const fromSdkEnv = typeof process.env.OPENHANDS_LLM_PROFILES_DIR === 'string' - ? process.env.OPENHANDS_LLM_PROFILES_DIR.trim() + const fromEnv = typeof process.env.E2E_LLM_PROFILES_DIR === 'string' + ? process.env.E2E_LLM_PROFILES_DIR.trim() : ''; - if (fromSdkEnv) return path.resolve(fromSdkEnv); + if (fromEnv) return path.resolve(fromEnv); return path.join(os.homedir(), '.openhands', 'llm-profiles'); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/suite/defaultProfilesSeeding.ts` around lines 19 - 31, The getProfilesDir helper currently prefers E2E_LLM_PROFILES_DIR over OPENHANDS_LLM_PROFILES_DIR, which is the opposite order used by the SDK's resolveRootDir; update getProfilesDir so it checks OPENHANDS_LLM_PROFILES_DIR first and falls back to E2E_LLM_PROFILES_DIR to match resolveRootDir's priority (refer to getProfilesDir and resolveRootDir and the env vars OPENHANDS_LLM_PROFILES_DIR and E2E_LLM_PROFILES_DIR).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/suite/defaultProfilesSeeding.ts`:
- Around line 19-31: The getProfilesDir helper currently prefers
E2E_LLM_PROFILES_DIR over OPENHANDS_LLM_PROFILES_DIR, which is the opposite
order used by the SDK's resolveRootDir; update getProfilesDir so it checks
OPENHANDS_LLM_PROFILES_DIR first and falls back to E2E_LLM_PROFILES_DIR to match
resolveRootDir's priority (refer to getProfilesDir and resolveRootDir and the
env vars OPENHANDS_LLM_PROFILES_DIR and E2E_LLM_PROFILES_DIR).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b7e3c06-07f8-4bef-a039-fb1676885204
📒 Files selected for processing (4)
packages/agent-sdk/src/sdk/llm/profiles.tssrc/extension.tstests/e2e/suite/defaultProfilesSeeding.tstests/e2e/suite/llmProfiles.ts
Co-authored-by: smolpaws <engel@enyst.org>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/extension.ts (1)
228-230: Align env-var precedence with SDK root resolution for consistency.When both env vars are set, this picks
E2E_LLM_PROFILES_DIRfirst, butpackages/agent-sdk/src/sdk/llm/profiles.tsresolvesOPENHANDS_LLM_PROFILES_DIRfirst. Matching the same order avoids surprising overrides in mixed setups.Suggested diff
- const explicitE2e = process.env.E2E_LLM_PROFILES_DIR?.trim() ?? ''; - const explicitSdk = process.env.OPENHANDS_LLM_PROFILES_DIR?.trim() ?? ''; - const rootDir = path.resolve(explicitE2e || explicitSdk || path.join(context.globalStorageUri.fsPath, 'llm-profiles')); + const explicitSdk = process.env.OPENHANDS_LLM_PROFILES_DIR?.trim() ?? ''; + const explicitE2e = process.env.E2E_LLM_PROFILES_DIR?.trim() ?? ''; + const rootDir = path.resolve(explicitSdk || explicitE2e || path.join(context.globalStorageUri.fsPath, 'llm-profiles'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/extension.ts` around lines 228 - 230, The env-var precedence is reversed compared to the SDK: change the rootDir resolution to prefer OPENHANDS_LLM_PROFILES_DIR over E2E_LLM_PROFILES_DIR so behavior matches packages/agent-sdk/src/sdk/llm/profiles.ts; specifically update the rootDir expression that currently uses explicitE2e || explicitSdk || ... to use explicitSdk || explicitE2e || ..., keeping explicitE2e and explicitSdk variables and the fallback path.resolve(path.join(context.globalStorageUri.fsPath, 'llm-profiles')) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/extension.ts`:
- Around line 228-230: The env-var precedence is reversed compared to the SDK:
change the rootDir resolution to prefer OPENHANDS_LLM_PROFILES_DIR over
E2E_LLM_PROFILES_DIR so behavior matches
packages/agent-sdk/src/sdk/llm/profiles.ts; specifically update the rootDir
expression that currently uses explicitE2e || explicitSdk || ... to use
explicitSdk || explicitE2e || ..., keeping explicitE2e and explicitSdk variables
and the fallback path.resolve(path.join(context.globalStorageUri.fsPath,
'llm-profiles')) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fc91e36-da13-4855-ba7b-f1a492283e15
📒 Files selected for processing (3)
packages/agent-sdk/src/sdk/llm/profiles.tssrc/extension.tstests/e2e/suite/defaultProfilesSeeding.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/agent-sdk/src/sdk/llm/profiles.ts
Co-authored-by: smolpaws <engel@enyst.org>
🔧 VSCode Extension Built Successfully• File: openhands-tab-0.9.3.vsix (548 KB) To install:
Built with Node 22. Commit 52c61f3. |
|
Addressed Gemini/Devin/CodeRabbit notes:
All checks green. |
1 participant
Ensure E2E runs never touch a developer's real
~/.openhands/llm-profiles.What changed
TEST_NAME), setE2E_LLM_PROFILES_DIR/OPENHANDS_LLM_PROFILES_DIRto a per-run directory under the VS Code user-data-dir globalStorage.OPENHANDS_LLM_PROFILES_DIR(andE2E_LLM_PROFILES_DIR) as the default root.Verification
npm run typecheck✅npm test✅npm run lint✅npm run e2e✅Summary by CodeRabbit
New Features
Bug Fixes
Tests