Skip to content

fix: warn when process env shadows dotenv keys in browse doctor#2360

Open
pirate wants to merge 3 commits into
mainfrom
stg-2555-doctor-env-shadow-warning
Open

fix: warn when process env shadows dotenv keys in browse doctor#2360
pirate wants to merge 3 commits into
mainfrom
stg-2555-doctor-env-shadow-warning

Conversation

@pirate

@pirate pirate commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • warn in browse doctor when a process-level Browserbase API key differs from .env or .env.local
  • parse dotenv files for diagnostics without loading them or exposing credential values
  • document process-environment precedence in the CLI setup README

Why

An inherited API key takes precedence over dotenv values and can silently route Browserbase sessions to a different account. The doctor warning makes that conflict visible while preserving the CLI's existing .env behavior and continuing not to load .env.local.

Validation

  • pnpm exec prettier --check packages/cli/src/lib/driver/doctor.ts packages/cli/tests/doctor.test.ts
  • pnpm exec eslint packages/cli/src/lib/driver/doctor.ts packages/cli/tests/doctor.test.ts
  • TypeScript syntax transpilation for the modified source and test
  • git diff --check
  • confirmed the generic doctor source remains free of a hard-coded API-key variable name

The focused Vitest suite could not run locally because the checkout's CLI dependencies were incomplete and the configured package proxy failed DNS resolution with ENOTFOUND socket-firewall.tail929132.ts.net.


Summary by cubic

Warns when a process-level BROWSERBASE_API_KEY overrides .env and flags .env.local differences as ignored, making config conflicts visible without exposing secrets. Updates the CLI README to clarify environment precedence and recommend running browse doctor.

  • New Features
    • browse doctor compares process env with .env and .env.local; warns when .env is overridden and notes .env.local mismatches are ignored since it isn’t auto-loaded, parsing with dotenv without loading files or printing values.
    • README clarifies that the process environment takes precedence, browse auto-loads only .env (not .env.local), and adds a quick browse doctor check.

Written for commit d826810. Summary will update on new commits.

Review in cubic

@changeset-bot

changeset-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d826810

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 3 files

Confidence score: 5/5

  • Safe to merge after the addressed issues were fixed.
Architecture diagram
sequenceDiagram
    participant User
    participant CLI as Doctor Command
    participant Builder as buildDoctorReport
    participant Warning as dotenvShadowingWarning
    participant Remote as remote configuration
    participant FS as File System
    participant Dotenv as dotenv.parse

    Note over User,Builder: User runs "browse doctor"
    User->>CLI: browse doctor
    CLI->>Builder: buildDoctorReport({ flags, session }, deps)
    Note over Builder,Warning: New environment‑shadowing check

    Builder->>Warning: dotenvShadowingWarning(env, cwd)

    Warning->>Remote: forwardedEnvKeys()
    Remote-->>Warning: ["BROWSERBASE_API_KEY"]

    loop Over [".env", ".env.local"]
        Warning->>FS: readFile(join(cwd, file))

        alt file exists
            FS-->>Warning: content
            Warning->>Dotenv: parse(content)
            Dotenv-->>Warning: parsed object

            loop For each env key
                alt processValue != fileValue
                    Warning->>Warning: record { file, variable }
                end
            end

        else file not found / error
            FS-->>Warning: error
            Warning->>Warning: skip file
        end
    end

    alt at least one conflict recorded
        Warning-->>Builder: DoctorCheck (name: "environment", status: "warn")
        Builder->>Builder: push check into checks[]
    else no conflicts
        Warning-->>Builder: null
    end

    Builder-->>CLI: DoctorReport (checks include environment warning if present)
    CLI-->>User: rendered report (warn message, never shows secret values)
Loading

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/cli/src/lib/driver/doctor.ts Outdated
shrey150 added a commit that referenced this pull request Jul 14, 2026
## Summary

`browse` has auto-loaded a `.env` file from the current directory on
startup since it moved into this monorepo, with no way to turn it off. A
user reported `.env` values in one project being silently overridden by
a stale shell-level key from another project -- the shell-level
`BROWSERBASE_API_KEY` from an earlier project took precedence over the
correct key in a different project's `.env`, so session replays went to
the wrong Browserbase account.

The agreed direction (per internal eng review) is that CLI tools used
across many unrelated projects/directories shouldn't auto-load `.env` at
all -- only `process.env` should matter, and it's on the user to
`export` vars or source `.env` themselves. Ripping that out in one PR
would break the existing install base that relies on the implicit
behavior, so this PR is the non-breaking first step:

- Default behavior is unchanged: `browse` still loads `.env` by default.
- New `BROWSE_LOAD_DOTENV` env var makes that behavior explicitly
toggleable: `0`/`false`/`no` (case-insensitive) skips `.env` loading
entirely today; any other explicit value (e.g. `1`) keeps loading with
no nagging.
- When the toggle is left unset (the implicit default) and loading a
`.env` file actually applies a variable not already in `process.env`, we
print a one-time deprecation warning to stderr naming the variable(s),
so people relying on the implicit default get advance notice.

A future PR (not this one) will flip the default of `BROWSE_LOAD_DOTENV`
to off, once the warning has had time to reach users.

This complements #2360, which adds a `browse doctor` warning when
`process.env` and `.env`/`.env.local` values *disagree* -- a useful
on-demand diagnostic, but it doesn't touch the auto-load behavior
itself. This PR does not depend on #2360.

## E2E Test Matrix

Built the CLI locally (`pnpm turbo run build --filter=browse`) and ran
the built `bin/run.js` directly in a scratch temp dir containing a
`.env` with a fake `BROWSERBASE_API_KEY`.

| Command / flow | Observed output | Confidence / sufficiency |
| --- | --- | --- |
| `node bin/run.js doctor --json` in scratch dir with `.env` containing
`BROWSERBASE_API_KEY=<fake>`, `BROWSE_LOAD_DOTENV` unset | stderr:
`[browse] Loaded BROWSERBASE_API_KEY from .env. Auto-loading .env is
deprecated and will be disabled by default in a future release -- export
these variables in your shell instead, or set BROWSE_LOAD_DOTENV=1 to
keep this behavior explicitly once that happens. Set
BROWSE_LOAD_DOTENV=0 to opt out today. Run \`browse doctor\` to check
for conflicts with your shell environment.` / stdout `verdict: "ok"` /
exit 0 | Proves default (implicit) path still loads `.env` and now warns
exactly once, naming the applied var. |
| `node bin/run.js doctor --remote` in same dir with
`BROWSE_LOAD_DOTENV=0` | stdout: `[fail] browserbase BROWSERBASE_API_KEY
is not set` / no stderr / exit 1 | Proves opting out today skips `.env`
entirely -- the key never reaches `process.env`, no warning printed
(explicit choice). |
| `node bin/run.js doctor --remote` in same dir with
`BROWSE_LOAD_DOTENV=1` | stdout: `[ok] browserbase BROWSERBASE_API_KEY
is set` / no stderr / exit 0 | Proves explicit opt-in keeps loading
`.env` with zero nagging. |
| `pnpm exec vitest run tests/cli-cloud-contract.test.ts
tests/doctor.test.ts` | `Test Files 2 passed (2)` / `Tests 69 passed
(69)` | Covers the existing `.env`-loads-config contract (unchanged
default) plus: warns when unset+applied, silent when
`BROWSE_LOAD_DOTENV=1`, and fails auth (key never loaded, no warning)
for each of `0`/`false`/`no`/`FALSE`/`NO` as `BROWSE_LOAD_DOTENV` --
added the extra opt-out aliases per cubic review feedback below. |
| `pnpm exec eslint bin/run.js tests/cli-cloud-contract.test.ts` + `pnpm
exec prettier --check bin/run.js README.md
tests/cli-cloud-contract.test.ts` + `pnpm exec tsc --noEmit -p
tsconfig.json` | All clean, no output/errors | Static checks on touched
files; supporting evidence, not a substitute for the live runs above. |

## Linear

STG-2579 (linked via branch name).

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
pirate and others added 3 commits July 14, 2026 15:18
.env.local is never auto-loaded by browse, so a mismatch there was
never going to take effect -- report it as ignored, not overridden,
and point users at moving the value to .env instead. Addresses cubic's
review comment on this PR.
@shrey150
shrey150 force-pushed the stg-2555-doctor-env-shadow-warning branch from 3f82331 to d826810 Compare July 14, 2026 22:19
@shrey150 shrey150 changed the title [STG-2555] Warn when process env shadows dotenv keys fix: warn when process env shadows dotenv keys in browse doctor Jul 14, 2026
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