Skip to content

Fix Gemini browser profile dir resolution#124

Open
blackopsrepl wants to merge 2 commits intosteipete:mainfrom
blackopsrepl:fix/gemini-browser-profile-dir
Open

Fix Gemini browser profile dir resolution#124
blackopsrepl wants to merge 2 commits intosteipete:mainfrom
blackopsrepl:fix/gemini-browser-profile-dir

Conversation

@blackopsrepl
Copy link
Copy Markdown

Summary

This PR fixes a narrow Gemini browser-session bug: openGeminiBrowserSession() was choosing the profile directory before browser config resolution, so Gemini manual-login / Deep Think runs could ignore configured browser profile defaults.

Intent

The intent of this PR is strictly to make Gemini browser sessions honor the same resolved profile-dir precedence that the rest of browser config uses:

  1. explicit manualLoginProfileDir
  2. ORACLE_BROWSER_PROFILE_DIR
  3. default ~/.oracle/browser-profile

This PR is intentionally separate from the Grok browser/provider refactor work. It does not change Grok, ChatGPT, or shared browser runtime behavior.

How we got into this bug

We found this while reviewing a separate browser-provider refactor branch. Review surfaced that Gemini browser runs were not respecting configured persistent profile directories unless browserConfig.manualLoginProfileDir was passed explicitly.

The root cause was local to src/gemini-web/browserSessionManager.ts:

  • it picked profileDir up front from browserConfig?.manualLoginProfileDir ?? ~/.oracle/browser-profile
  • only after that did it call resolveBrowserConfig()
  • that meant resolved defaults like ORACLE_BROWSER_PROFILE_DIR never got a chance to influence the actual Gemini Chrome profile path

In practice, users with a configured browser profile dir could silently launch the wrong Gemini profile and miss their signed-in session.

Why this solution is right

This fix keeps the scope intentionally small:

  • openGeminiBrowserSession() now resolves browser config first
  • it then derives profileDir from resolvedConfig.manualLoginProfileDir
  • a defensive fallback remains for explicit config / env / default path so existing mocked tests and unexpected call sites stay safe

Why this is the correct behavior:

  • resolveBrowserConfig() is already the source of truth for browser config precedence
  • Gemini should not reimplement its own earlier, narrower profile-dir resolution path
  • the change is isolated to Gemini session setup and does not alter shared provider/runtime logic

Tests

Added focused coverage for Gemini session manager profile resolution:

  • explicit manualLoginProfileDir wins over ORACLE_BROWSER_PROFILE_DIR
  • ORACLE_BROWSER_PROFILE_DIR is honored when explicit config is absent

Validation run on this branch:

  • pnpm test
  • pnpm run typecheck
  • pnpm run build

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aacc0185c8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown

@JiwaniZakir JiwaniZakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new resolveGeminiProfileDir function correctly handles blank/whitespace env values via .trim(), and the test coverage for that edge case in browserSessionManager.test.ts is solid. However, there's a potential inconsistency worth examining: after computing profileDir (which may come from ORACLE_BROWSER_PROFILE_DIR), resolvedConfig.manualLoginProfileDir is never updated to reflect the final resolved path. In the env-var scenario, resolvedConfig was built without manualLoginProfileDir (since browserConfig?.manualLoginProfileDir would be unset), so if the downstream launchChrome call still uses resolvedConfig for its first argument, the manualLoginProfileDir field in the config object would be undefined even though profileDir correctly points to the env dir. The test at line ~109 asserts expect.objectContaining({ manualLoginProfileDir: envDir }) on the launchChrome call — worth verifying that assertion actually passes rather than silently becoming a no-op match. A missing test case: manualLoginProfileDir set to an empty string "" directly (not just whitespace), which would also be filtered by the if (normalized) check and might surprise callers who explicitly passed "" to opt out of a path.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants