Skip to content

fix(cli): name the launch variable in invalid launch-default startup errors - #236

Closed
devin-ai-integration[bot] wants to merge 1 commit into
maintenance/2.8.9from
devin/e2e-fix/235-1c468b5
Closed

devin-ai-integration[bot] wants to merge 1 commit into
maintenance/2.8.9from
devin/e2e-fix/235-1c468b5

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

What and why

What: This is a fix for #235, which targets maintenance/2.8.9 at reviewed head 1c468b5. The CLI startup preflight now prints invalid launch-default environment errors under the variable that is actually wrong. Real profile errors keep the BETTERWRIGHT_PROFILE: / --profile: label.

Why: In #235, cliProfile() now calls launchOptionsFromEnv(), which validates BETTERWRIGHT_HEADLESS and BETTERWRIGHT_DOWNLOAD_POLICY. The preflight catch in runCli then puts the profile-source label on every error, so a bad display/download variable points the user at the wrong setting:

$ BETTERWRIGHT_HEADLESS=maybe betterwright run -c 'return 1'
BETTERWRIGHT_PROFILE: BETTERWRIGHT_HEADLESS must be auto, true, or false.      # before
BETTERWRIGHT_HEADLESS must be auto, true, or false.                            # after
$ BETTERWRIGHT_HEADLESS=maybe betterwright run --profile work -c 'return 1'
--profile: BETTERWRIGHT_HEADLESS must be auto, true, or false.                 # before

Every command that runs the preflight hits this, including doctor and cookies browsers. The fix leaves the label off any error message that already starts with a BETTERWRIGHT_* variable name. It is a small change: 1 source hunk plus 1 regression test.

Checklist

  • bun run release:check was not run in full. On this Linux VM, test:unit has 2 failures that come from the environment and are unrelated to this change: the chromium-package packager tests need Python ≥3.11 hashlib.file_digest, and the VM has 3.10. These tests fail the same way on the reviewed head. lint, typecheck, and build:harness pass.
  • Guard proxy: unchanged.
  • No secret output: unchanged; only the stderr label changes.
  • Dependency pins: unchanged.
  • Public API / types/*.d.ts: no change.
  • CI job names / actions: unchanged.
  • No unit test imports the worker; the new test spawns dist/bin/betterwright.js.
  • CHANGELOG: not updated, because this fixes behavior that 2.8.9 itself introduces and has not shipped yet.
  • Nothing private committed.

How it was verified

  • New test invalid launch environment errors name their own variable, not the profile source in tests/node/cli-startup.test.ts. On reviewed head 1c468b5 it fails (0 pass / 1 fail). On this branch ce29ba3 it passes, and the whole file passes (5/5).
  • I ran the real built CLI, before and after, with BETTERWRIGHT_HEADLESS=maybe (with and without --profile work), BETTERWRIGHT_DOWNLOAD_POLICY=sometimes and BETTERWRIGHT_PROFILE=../escape. The last case still prints BETTERWRIGHT_PROFILE: invalid profile name ….
  • Environment: Ubuntu 22.04 x64, Bun 1.4.0, BetterChromium 153.0.8010.36, playwright-core 1.61.1.

Recording of the before/after run and the regression test (MP4): https://app.devin.ai/api/presigned_proxy?token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX2lkIjpudWxsLCJidWNrZXRfbmFtZSI6ImRldmluYXR0YWNobWVudHMiLCJidWNrZXRfa2V5IjoiYXR0YWNobWVudHNfcHJpdmF0ZS9vcmctYmQ0NTliYTUyOTRiNDEyMGJiYWRhNmE1Y2JmODIyZTUvNDJlOWZhZTEtMTllZS00NjQyLWJmNDMtMmU3OTRiMTUxNzA0IiwiaWF0IjoxNzkxODU3NjM1LCJvcmdfaWQiOiJvcmctYmQ0NTliYTUyOTRiNDEyMGJiYWRhNmE1Y2JmODIyZTUiLCJmaWxlbmFtZSI6InByMjM1LWNsaS1sYWJlbC1maXgtY2UyOWJhMy5tcDQifQ.LyhQ7_wNl-g6MbxqOidy7--kqyPUSo9qYyYje4W3Fgw

CLI label before/after on 1c468b5 vs ce29ba3

Full E2E report: on #235.

Written by Devin

Open in Devin Desktop: https://app.devin.ai/desktop/session/4e6d7e2ecdb94f9c87e8ed1a4ea75614?variant=devin

…errors

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Note: since this fix was opened, maintenance/2.8.9 has moved to 867468b. The two new commits don't touch bin/cli-main.ts or tests/node/cli-startup.test.ts, so this change applies unchanged to the new head. The full E2E report for the reviewed SHA 1c468b5 is here: #235 (comment). This PR stays a draft until a maintainer approves it.

@SSHdotCodes

Copy link
Copy Markdown
Collaborator

Superseded by #239, merged for 2.9.0. The original fix and regression test were cherry-picked with attribution, then adjusted for the final main branch's decoupled profile parsing. Launch validation now reports the actual environment variable while preserving explicit daemon stdin overrides and management commands. Closing this outdated maintenance-branch draft after the replacement landed.

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.

1 participant