Skip to content

fix: resolve CLI file paths before daemon IPC - #2821

Open
achideal wants to merge 1 commit into
ChromeDevTools:mainfrom
achideal:fix/cli-file-path-resolution
Open

achideal wants to merge 1 commit into
ChromeDevTools:mainfrom
achideal:fix/cli-file-path-resolution

Conversation

@achideal

Copy link
Copy Markdown
Contributor

Summary

  • derive CLI file-path metadata from each tool's verifyFilesSchema
  • resolve relative scalar and array file arguments in the CLI client before daemon IPC
  • preserve absolute paths and supported URLs
  • add unit coverage and an end-to-end test with different client and daemon working directories

Why

The CLI client and the long-running daemon can have different current working directories. Sending a relative file argument unchanged makes the daemon resolve it from the wrong directory, so commands such as take_screenshot, take_snapshot, upload_file, and other file-accepting commands can read or write the wrong location, or fail.

This follow-up was requested during the review of #2606 and keeps the generic CLI behavior independent of the evaluate_script feature.

Testing

  • npm run gen
  • npm run test -- tests/cli.test.ts
  • npm run test -- tests/e2e/chrome-devtools-commands.test.ts
  • npm run test
    • the changed and related tests pass
    • the local full suite retains two unrelated existing failures: an extension-page listing timeout and Chrome rejecting an excessively large full-page screenshot

@OrKoN OrKoN left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR:

  • Return filePathOrUrl unchanged when filePathOrUrl.trim().length === 0 in resolveFilePath (src/config/cli-commands.ts). Calling path.resolve(cwd, filePathOrUrl) on empty or whitespace-only strings turns them into cwd (or ${cwd}/...) instead of letting validateAndResolvePathOrUrl (src/ToolHandler.ts) clear them to undefined.
  • Rebase onto main, re-run npm run gen to update src/config/cli-options.ts, and replace fs.mkdtemp / fs.rm in tests/e2e/chrome-devtools-commands.test.ts with using rootDirectory = createTempDir('cli-file-path-') from tests/utils.ts (ensuring runCli(['stop'], sessionId) runs before rootDirectory is disposed). main now enforces the local/enforce-using ESLint rule against direct mkdtemp calls in tests and includes changes in src/config/cli-options.ts, tests/cli.test.ts, and tests/utils.ts that conflict with this branch.

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