[build] update py:local_dev task to ensure all files are generated and symlinks are always resolved

[titusfortner]

🔗 Related Issues

💥 What does this PR do?

Fixes py:local_dev so it stages real generated content into the source tree instead of leaving broken links behind.

It built //py:selenium, a py_library with srcs = [], which materializes nothing into bazel-bin — the task then copied whatever residue happened to be there. It now builds //py:selenium-wheel, whose packaging action forces the generated sources onto disk.

It also copied with cp_r, which preserves the symlinks bazel-bin leaves into the Bazel cache — including the three selenium-manager binaries. A bazel clean turned those into dangling links that failed at runtime, looking like a Selenium bug. The task now copies each generated file individually and resolves every symlink to real content.

🔧 Implementation Notes

Staging happens in two passes. Fully generated directories (common/bidi, common/devtools) are cleared and repopulated from bazel-bin, resolving the cache symlinks; any git-tracked files in them are restored afterward, since the generated paths are all git-ignored and the tracked files are exactly the hand-written ones that share the directory. Individual generated files that live among hand-written ones — the remote atoms, the mutation-listener scripts, the three selenium-manager binaries, and firefox/webdriver_prefs.json — are copied in place. all runs the directory pass over every generated top-level directory instead of the curated set.

This task is run manually by committers, so it does not guard against or abort on a dirty working tree.

🤖 AI assistance

• AI assisted (complete below)
  • Tool(s): Claude Code
  • What was generated: rake task changes
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• Bug fix (backwards compatible)

[qodo-code-review]

PR Summary by Qodo

Fix py:local_dev staging by building wheel outputs and dereferencing symlinks

🐞 Bug fix ⚙️ Configuration changes 🕐 10-20 Minutes

AI Description

• Build the wheel target to force generated Selenium assets onto disk.
• Copy generated directories by dereferencing Bazel cache symlinks to real files.
• Refuse to run when generated dirs have local changes; restore tracked files after copy.

Diagram

graph TD
  A(["rake py:local_dev"]) --> B[["Bazel build //py:selenium-wheel"]] --> C[("bazel-bin outputs")] --> D["Copy dirs (realpath)"] --> E{"Git changes under dest?"}
  E -- "yes" --> F["Abort (protect local edits)"]
  E -- "no" --> G["Restore tracked files (git checkout)"] --> H["Working tree: py/selenium/webdriver"]

  subgraph Legend
    direction LR
    _task(["Task"]) ~~~ _build[["Build step"]] ~~~ _out[("Build outputs")] ~~~ _proc["Process"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use rsync with copy-links (e.g., rsync -aL)

• ➕ Naturally dereferences symlinks without a per-file Ruby loop
• ➕ Good performance and fewer Ruby filesystem edge cases
• ➖ Adds a dependency on rsync behavior/availability across environments (macOS/Linux/CI images)
• ➖ More escaping/quoting complexity and less portable than pure Ruby

2. Use FileUtils.cp_r with explicit symlink-dereference options

• ➕ Keeps implementation in Ruby with a simpler copy operation
• ➕ Potentially faster than globbing every file
• ➖ Ruby/FileUtils symlink handling is subtle and may not dereference nested cache symlinks as intended
• ➖ Harder to reason about correctness than explicit realpath-per-file copying

Recommendation: The PR’s approach (build a wheel-producing target, then copy files via File.realpath with a safety abort and tracked-file restore) is the most robust and portable way to prevent dangling Bazel cache symlinks from entering the working tree. Alternatives like rsync or cp_r dereference options could reduce code, but introduce portability risk or unclear symlink semantics.

Files changed (1) +16 / -31

Bug fix (1) +16 / -31

python.rakeFix py:local_dev to stage real Bazel outputs with safety checks +16/-31

Fix py:local_dev to stage real Bazel outputs with safety checks

• Switches the Bazel build target from //py:selenium to //py:selenium-wheel so generated packaging artifacts are actually materialized in bazel-bin. Replaces cp_r-based directory copying with a per-file copy that dereferences symlinks via realpath, aborts on uncommitted destination changes, and restores git-tracked files after regenerating mixed directories.

rake_tasks/python.rake

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 11 rules

1. Local edits can be lost ✗ Dismissed 🐞 Bug ☼ Reliability ⭐ New

Description

py:local_dev deletes destination paths and (when tracked files exist) restores them from HEAD
via checkout_file without checking for local modifications, so running it can silently discard
in-progress work. In all mode, this applies to generated output directories like common/ and
remote/, which are tracked source trees in py/selenium/webdriver/.

Code

rake_tasks/python.rake[R75-111]

+  if arguments[:all] == 'all'
+    dirs = Dir.children(bazel_bin)
+    files = []
  else
-    bidi_src = "#{bazel_bin}/common/bidi"
-    bidi_dest = "#{lib_path}/common/bidi"
-    if Dir.exist?(bidi_src)
-      FileUtils.mkdir_p(bidi_dest)
-      Dir.children(bidi_src).sort.each do |entry|
-        src = File.join(bidi_src, entry)
-        dest = File.join(bidi_dest, entry)
-        next unless File.file?(src) || File.symlink?(src)
-
-        resolved_src = File.symlink?(src) ? File.realpath(src) : src
-        FileUtils.rm_f(dest)
-        FileUtils.cp(resolved_src, dest)
-      end
-    end
+    dirs = %w[common/bidi common/devtools]
+    files = %w[
+      remote/getAttribute.js remote/isDisplayed.js remote/findElements.js
+      common/mutation-listener.js common/bidi-mutation-listener.js
+      common/linux/selenium-manager common/macos/selenium-manager common/windows/selenium-manager.exe
+      firefox/webdriver_prefs.json
+    ]
+  end

-    %w[common/devtools common/linux common/macos common/windows].each do |dir|
-      src = "#{bazel_bin}/#{dir}"
-      dest = "#{lib_path}/#{dir}"
-      next unless Dir.exist?(src)
+  dirs.each do |dir|
+    src_dir = "#{bazel_bin}/#{dir}"
+    dest_dir = "#{lib_path}/#{dir}"

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end
+    FileUtils.rm_rf(dest_dir)
+    # Restore any git tracked files in the directory we just deleted
+    SeleniumRake.git.checkout_file('HEAD', dest_dir) unless SeleniumRake.git.ls_files(dest_dir).empty?

-    %w[getAttribute.js isDisplayed.js findElements.js].each do |atom|
-      dest = "#{lib_path}/remote/#{atom}"
-      FileUtils.rm_f(dest)
-      FileUtils.cp("#{bazel_bin}/remote/#{atom}", dest)
+    # Copy each file individually to resolve Bazel's cache symlinks
+    Dir.glob(File.join(src_dir, '**', '*')).each do |src|
+      next unless File.file?(src)
+
+      dest = File.join(dest_dir, src.delete_prefix("#{src_dir}/"))
+      FileUtils.mkdir_p(File.dirname(dest))
+      FileUtils.cp(File.realpath(src), dest)
    end
  end
+
+  files.each do |file|
+    dest = "#{lib_path}/#{file}"
+    FileUtils.mkdir_p(File.dirname(dest))
+    FileUtils.rm_f(dest)
+    FileUtils.cp(File.realpath("#{bazel_bin}/#{file}"), dest)
+  end

Evidence

The task unconditionally deletes destination directories/files and may explicitly reset tracked
content to HEAD via checkout_file. Bazel-generated outputs are defined under
selenium/webdriver/common/* and selenium/webdriver/remote/*, so all mode can include common
and remote; those destination directories contain tracked Python sources, meaning local edits
there can be discarded.

rake_tasks/python.rake[68-111]
py/BUILD.bazel[105-157]
py/selenium/webdriver/common/action_chains.py[17-28]
py/selenium/webdriver/remote/webdriver.py[18-30]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`py:local_dev` removes and overwrites paths under `py/selenium/webdriver` (and in `all` mode, potentially large tracked source directories) without verifying those paths have no uncommitted or untracked changes. This can result in silent loss of developer changes.

## Issue Context
The task now uses `rm_rf`/`rm_f` plus `SeleniumRake.git.checkout_file('HEAD', ...)` and `cp` to repopulate from `bazel-bin`. This should refuse to run when any destination path it will touch is dirty.

## Fix Focus Areas
- Add a preflight “dirty path” guard that checks **both tracked modifications and untracked files** for every destination path that will be removed/overwritten; abort with a clear message listing offending paths.
- Prefer using the existing `git` gem instance (`SeleniumRake.git`) to query status, rather than shelling out.

### Suggested implementation outline
1. Build `dest_paths`:
  - For dirs: `dest_dir = "#{lib_path}/#{dir}"`
  - For files: `dest = "#{lib_path}/#{file}"`
2. Compute dirtiness:
  - Tracked changes under path
  - Untracked files under path
3. `abort(...)` before performing any `rm_rf`/`rm_f`.

## Fix Focus Areas (code pointers)
- rake_tasks/python.rake[68-111]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Shell-injected git status ✗ Dismissed 🐞 Bug ⛨ Security

Description

py:local_dev runs git status via backticks with an unescaped interpolated path built from
filesystem-derived names (Dir.children in all mode), which allows shell metacharacters in an
entry name to change the executed command. The check also ignores git exit status/stderr, so a
failing git status can be treated as “clean” and the task proceeds to delete/copy anyway.

Code

rake_tasks/python.rake[R88-92]

+  dirs.each do |dir|
+    src_dir = "#{bazel_bin}/#{dir}"
+    dest_dir = "#{lib_path}/#{dir}"
+    abort("Commit or stash your changes under #{dest_dir} first") unless `git status --porcelain #{dest_dir}`.empty?

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end

Evidence

The rake task derives dirs from Dir.children(bazel_bin) in all mode, then interpolates the
resulting dest_dir into a backtick shell command. Backticks execute via a shell and the code does
not check the command’s exit status, making this both injection-prone and unreliable as a safety
guard.

rake_tasks/python.rake[75-95]
rake_tasks/bazel.rb[14-52]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`py:local_dev` uses backticks to run `git status --porcelain #{dest_dir}`. In `all` mode, `dest_dir` incorporates directory names from `Dir.children(bazel_bin)`; if any entry name contains shell metacharacters, the backtick invocation can execute unintended shell syntax. Separately, the current code does not validate the command exit status and does not capture stderr, so failures can be misinterpreted as “no changes” and the task proceeds.

### Issue Context
This is a safety guard intended to prevent destructive deletes/copies when the destination has local changes.

### Fix Focus Areas
- rake_tasks/python.rake[75-105]

### Suggested fix
Replace the backtick call with an argv-based subprocess invocation and verify success, e.g.:

```rb
require "open3"

out, status = Open3.capture2e("git", "status", "--porcelain", "--", dest_dir)
abort("git status failed for #{dest_dir}: #{out}") unless status.success?
abort("Commit or stash your changes under #{dest_dir} first") unless out.empty?
```

(Alternatively, if you want to keep backticks, at minimum use `Shellwords.escape(dest_dir)` and check `$CHILD_STATUS.success?`, but argv-based execution is preferred.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. py:local_dev lacks regression tests 📘 Rule violation ▣ Testability

Description

This PR changes the py:local_dev behavior (build target, symlink resolution, and safety abort on
dirty working tree) but the provided diff shows no accompanying automated tests to prevent
regressions. This increases the risk of breaking local-dev staging behavior again without detection.

Code

rake_tasks/python.rake[R68-94]

desc 'Copy known generated files for local development (use `./go py:local_dev all` to copy everything)'
task :local_dev, [:all] do |_task, arguments|
-  Bazel.execute('build', [], '//py:selenium')
+  Bazel.execute('build', [], '//py:selenium-wheel')

  bazel_bin = 'bazel-bin/py/selenium/webdriver'
  lib_path = 'py/selenium/webdriver'

-  copy_all = arguments[:all] == 'all'
-  if copy_all
-    FileUtils.rm_rf("#{lib_path}/common/devtools")
-    FileUtils.cp_r("#{bazel_bin}/.", lib_path, remove_destination: true)
-  else
-    bidi_src = "#{bazel_bin}/common/bidi"
-    bidi_dest = "#{lib_path}/common/bidi"
-    if Dir.exist?(bidi_src)
-      FileUtils.mkdir_p(bidi_dest)
-      Dir.children(bidi_src).sort.each do |entry|
-        src = File.join(bidi_src, entry)
-        dest = File.join(bidi_dest, entry)
-        next unless File.file?(src) || File.symlink?(src)
-
-        resolved_src = File.symlink?(src) ? File.realpath(src) : src
-        FileUtils.rm_f(dest)
-        FileUtils.cp(resolved_src, dest)
-      end
-    end
+  dirs = arguments[:all] ? Dir.children(bazel_bin) : %w[common/bidi common/devtools common/linux common/macos common/windows remote]

-    %w[common/devtools common/linux common/macos common/windows].each do |dir|
-      src = "#{bazel_bin}/#{dir}"
-      dest = "#{lib_path}/#{dir}"
-      next unless Dir.exist?(src)
+  dirs.each do |dir|
+    src_dir = "#{bazel_bin}/#{dir}"
+    dest_dir = "#{lib_path}/#{dir}"
+    abort("Commit or stash your changes under #{dest_dir} first") if SeleniumRake.git.diff('HEAD').path(dest_dir).any?

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end
+    FileUtils.rm_rf(dest_dir)
+    # Copy each file individually to resolve Bazel's cache symlinks
+    Dir.glob(File.join(src_dir, '**', '*')).each do |src|
+      next unless File.file?(src)

-    %w[getAttribute.js isDisplayed.js findElements.js].each do |atom|
-      dest = "#{lib_path}/remote/#{atom}"
-      FileUtils.rm_f(dest)
-      FileUtils.cp("#{bazel_bin}/remote/#{atom}", dest)
+      dest = File.join(dest_dir, src.delete_prefix("#{src_dir}/"))
+      FileUtils.mkdir_p(File.dirname(dest))
+      FileUtils.cp(File.realpath(src), dest)
    end
+
+    # Restore any git tracked files in the directories
+    SeleniumRake.git.checkout_file('HEAD', dest_dir) unless SeleniumRake.git.ls_files(dest_dir).empty?
  end

Evidence

PR Compliance ID 389273 requires tests for bug fixes/new behavior. The diff adds new logic in
py:local_dev (building //py:selenium-wheel, resolving symlinks via File.realpath, and aborting
on uncommitted destination changes), but no test changes are present in the provided diff.

Rule 389273: Require tests for all new functionality and bug fixes
rake_tasks/python.rake[68-94]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `py:local_dev` rake task behavior was changed, but no automated regression test was added/updated alongside it.

## Issue Context
The updated task now builds `//py:selenium-wheel`, clears and repopulates generated directories while resolving symlinks, and aborts if there are uncommitted changes under the destination directory. A focused test should exercise at least one previously-broken scenario (dangling symlink / symlink resolution) so the change is protected from future regressions.

## Fix Focus Areas
- rake_tasks/python.rake[68-94]
- (add a new test file under an existing test framework directory for Ruby/Rake tooling, asserting the new behavior)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. Missing runtime resource copies ✓ Resolved 🐞 Bug ≡ Correctness

Description

py:local_dev only copies a directory allowlist, so generated single-file outputs (e.g.
common/mutation-listener.js, common/bidi-mutation-listener.js, firefox/webdriver_prefs.json) are not
staged into py/selenium/webdriver in the default mode. Python code loads these resources at runtime
and will raise (ValueError/FileNotFoundError) when they’re missing.

Code

rake_tasks/python.rake[R72-90]

  bazel_bin = 'bazel-bin/py/selenium/webdriver'
  lib_path = 'py/selenium/webdriver'

-  copy_all = arguments[:all] == 'all'
-  if copy_all
-    FileUtils.rm_rf("#{lib_path}/common/devtools")
-    FileUtils.cp_r("#{bazel_bin}/.", lib_path, remove_destination: true)
-  else
-    bidi_src = "#{bazel_bin}/common/bidi"
-    bidi_dest = "#{lib_path}/common/bidi"
-    if Dir.exist?(bidi_src)
-      FileUtils.mkdir_p(bidi_dest)
-      Dir.children(bidi_src).sort.each do |entry|
-        src = File.join(bidi_src, entry)
-        dest = File.join(bidi_dest, entry)
-        next unless File.file?(src) || File.symlink?(src)
-
-        resolved_src = File.symlink?(src) ? File.realpath(src) : src
-        FileUtils.rm_f(dest)
-        FileUtils.cp(resolved_src, dest)
-      end
-    end
+  dirs = arguments[:all] ? Dir.children(bazel_bin) : %w[common/bidi common/devtools common/linux common/macos common/windows remote]

-    %w[common/devtools common/linux common/macos common/windows].each do |dir|
-      src = "#{bazel_bin}/#{dir}"
-      dest = "#{lib_path}/#{dir}"
-      next unless Dir.exist?(src)
+  dirs.each do |dir|
+    src_dir = "#{bazel_bin}/#{dir}"
+    dest_dir = "#{lib_path}/#{dir}"
+    abort("Commit or stash your changes under #{dest_dir} first") if SeleniumRake.git.diff('HEAD').path(dest_dir).any?

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end
+    FileUtils.rm_rf(dest_dir)
+    # Copy each file individually to resolve Bazel's cache symlinks
+    Dir.glob(File.join(src_dir, '**', '*')).each do |src|
+      next unless File.file?(src)

-    %w[getAttribute.js isDisplayed.js findElements.js].each do |atom|
-      dest = "#{lib_path}/remote/#{atom}"
-      FileUtils.rm_f(dest)
-      FileUtils.cp("#{bazel_bin}/remote/#{atom}", dest)
+      dest = File.join(dest_dir, src.delete_prefix("#{src_dir}/"))
+      FileUtils.mkdir_p(File.dirname(dest))
+      FileUtils.cp(File.realpath(src), dest)
    end

Evidence

The rake task’s default allowlist contains only directories under common/ and remote/, so it can’t
stage Bazel outputs that are files under common/ (root) or under firefox/. Those files are
explicitly produced by Bazel and are required by runtime code paths that load them from package data
/ filesystem.

rake_tasks/python.rake[68-90]
py/BUILD.bazel[105-157]
py/selenium/webdriver/common/log.py[44-54]
py/selenium/webdriver/firefox/firefox_profile.py[35-71]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`py:local_dev` only iterates over a list of *directories* and copies `**/*` beneath them. Several Bazel-generated resources are *files* that live outside those directories (e.g., `common/mutation-listener.js`, `common/bidi-mutation-listener.js`, `firefox/webdriver_prefs.json`), so they never get staged in the default `./go py:local_dev` path, causing runtime failures when Selenium tries to load them.

### Issue Context
- These resources are produced by `copy_file(...)` outputs in `py/BUILD.bazel`.
- Runtime Python code expects them to exist in the source tree when running from source (the purpose of `py:local_dev`).

### Fix Focus Areas
- rake_tasks/python.rake[68-94]
 - Extend the staging logic to handle both directories and individual files.
 - Add explicit handling (in the default mode) to copy:
   - `common/mutation-listener.js`
   - `common/bidi-mutation-listener.js`
   - `firefox/webdriver_prefs.json`
 - Optionally, in `all` mode, detect whether `Dir.children(bazel_bin)` entries are files vs directories and copy root files correctly (copy the file itself, not only `**/*`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Any arg triggers full copy ✓ Resolved 🐞 Bug ≡ Correctness

Description

The task now treats any non-nil value for the :all argument as enabling the “copy everything” mode,
so ./go py:local_dev foo behaves like all despite the help text. This makes the task’s behavior
unpredictable and can cause much broader deletion/copying than intended.

Code

rake_tasks/python.rake[75]

+  dirs = arguments[:all] ? Dir.children(bazel_bin) : %w[common/bidi common/devtools common/linux common/macos common/windows remote]

Evidence

The go wrapper forwards arbitrary task args into the rake argument slot, and the new condition only
checks presence rather than equality to the documented string.

rake_tasks/python.rake[68-76]
go[21-40]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`dirs = arguments[:all] ? ...` treats any provided argument value as truthy, not just the documented `all` flag, so arbitrary arguments unexpectedly enable the full-copy path.

### Issue Context
The `./go` wrapper forwards any extra CLI args into the rake task’s bracket args (`task[args]`). With the current truthiness check, any arg value will switch behavior.

### Fix Focus Areas
- rake_tasks/python.rake[75-75]
 - Change the condition to `arguments[:all] == 'all'` (or explicitly validate accepted values and abort on unknown values).
- go[21-40]
 - (No change required, but this shows why arbitrary args are easy to pass.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Untracked files may be deleted ✓ Resolved 🐞 Bug ☼ Reliability

Description

The safety check only inspects diffs vs HEAD before rm_rf, so untracked files under the
destination directories can be removed without triggering the abort. After deletion, the checkout
restores tracked files from HEAD but cannot restore deleted untracked files.

Code

rake_tasks/python.rake[R80-83]

+    abort("Commit or stash your changes under #{dest_dir} first") if SeleniumRake.git.diff('HEAD').path(dest_dir).any?

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end
+    FileUtils.rm_rf(dest_dir)
+    # Copy each file individually to resolve Bazel's cache symlinks

Evidence

The code deletes the directory tree after only checking diff('HEAD'), which by construction can’t
protect files that don’t exist in HEAD; the later checkout is explicitly limited to tracked files.

rake_tasks/python.rake[77-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Before deleting `dest_dir`, the task only checks `git.diff('HEAD')` scoped to `dest_dir`. Untracked files are not part of `HEAD`, so they can be deleted by `FileUtils.rm_rf(dest_dir)` without warning.

### Issue Context
This task is intentionally destructive (it clears and repopulates directories). The current guard protects modified tracked files but can still lose untracked work (scratch files, local artifacts, etc.) inside these directories.

### Fix Focus Areas
- rake_tasks/python.rake[77-93]
 - Add a guard that also detects untracked files under `dest_dir` (e.g., via `SeleniumRake.git.status` if available, or by invoking `git status --porcelain --untracked-files=all -- <path>` and checking for output).
 - Abort with a clear message listing the offending paths.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Restore comment restates code ✓ Resolved 📘 Rule violation ✧ Quality

Description

The newly added comment # Restore any git tracked files in the directories restates what the
immediately following line does without adding rationale or constraints. This reduces
signal-to-noise in comments and conflicts with the project guideline to prefer rationale-focused
comments.

Code

rake_tasks/python.rake[R92-93]

+    # Restore any git tracked files in the directories
+    SeleniumRake.git.checkout_file('HEAD', dest_dir) unless SeleniumRake.git.ls_files(dest_dir).empty?

Evidence

PR Compliance ID 389275 discourages comments that merely restate code behavior. The comment at
rake_tasks/python.rake is a direct natural-language repetition of the following checkout_file
call, without providing rationale.

Rule 389275: Prefer rationale-focused comments over restating code behavior
rake_tasks/python.rake[92-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A newly added comment restates the code behavior without explaining why the behavior is needed.

## Issue Context
The comment sits directly above a `checkout_file` call whose intent is already clear from the method name and arguments.

## Fix Focus Areas
- rake_tasks/python.rake[92-93]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 6603661

Results up to commit 70f3e69

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. py:local_dev lacks regression tests 📘 Rule violation ▣ Testability

Description

This PR changes the py:local_dev behavior (build target, symlink resolution, and safety abort on
dirty working tree) but the provided diff shows no accompanying automated tests to prevent
regressions. This increases the risk of breaking local-dev staging behavior again without detection.

Code

rake_tasks/python.rake[R68-94]

desc 'Copy known generated files for local development (use `./go py:local_dev all` to copy everything)'
task :local_dev, [:all] do |_task, arguments|
-  Bazel.execute('build', [], '//py:selenium')
+  Bazel.execute('build', [], '//py:selenium-wheel')

  bazel_bin = 'bazel-bin/py/selenium/webdriver'
  lib_path = 'py/selenium/webdriver'

-  copy_all = arguments[:all] == 'all'
-  if copy_all
-    FileUtils.rm_rf("#{lib_path}/common/devtools")
-    FileUtils.cp_r("#{bazel_bin}/.", lib_path, remove_destination: true)
-  else
-    bidi_src = "#{bazel_bin}/common/bidi"
-    bidi_dest = "#{lib_path}/common/bidi"
-    if Dir.exist?(bidi_src)
-      FileUtils.mkdir_p(bidi_dest)
-      Dir.children(bidi_src).sort.each do |entry|
-        src = File.join(bidi_src, entry)
-        dest = File.join(bidi_dest, entry)
-        next unless File.file?(src) || File.symlink?(src)
-
-        resolved_src = File.symlink?(src) ? File.realpath(src) : src
-        FileUtils.rm_f(dest)
-        FileUtils.cp(resolved_src, dest)
-      end
-    end
+  dirs = arguments[:all] ? Dir.children(bazel_bin) : %w[common/bidi common/devtools common/linux common/macos common/windows remote]

-    %w[common/devtools common/linux common/macos common/windows].each do |dir|
-      src = "#{bazel_bin}/#{dir}"
-      dest = "#{lib_path}/#{dir}"
-      next unless Dir.exist?(src)
+  dirs.each do |dir|
+    src_dir = "#{bazel_bin}/#{dir}"
+    dest_dir = "#{lib_path}/#{dir}"
+    abort("Commit or stash your changes under #{dest_dir} first") if SeleniumRake.git.diff('HEAD').path(dest_dir).any?

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end
+    FileUtils.rm_rf(dest_dir)
+    # Copy each file individually to resolve Bazel's cache symlinks
+    Dir.glob(File.join(src_dir, '**', '*')).each do |src|
+      next unless File.file?(src)

-    %w[getAttribute.js isDisplayed.js findElements.js].each do |atom|
-      dest = "#{lib_path}/remote/#{atom}"
-      FileUtils.rm_f(dest)
-      FileUtils.cp("#{bazel_bin}/remote/#{atom}", dest)
+      dest = File.join(dest_dir, src.delete_prefix("#{src_dir}/"))
+      FileUtils.mkdir_p(File.dirname(dest))
+      FileUtils.cp(File.realpath(src), dest)
    end
+
+    # Restore any git tracked files in the directories
+    SeleniumRake.git.checkout_file('HEAD', dest_dir) unless SeleniumRake.git.ls_files(dest_dir).empty?
  end

Evidence

PR Compliance ID 389273 requires tests for bug fixes/new behavior. The diff adds new logic in
py:local_dev (building //py:selenium-wheel, resolving symlinks via File.realpath, and aborting
on uncommitted destination changes), but no test changes are present in the provided diff.

Rule 389273: Require tests for all new functionality and bug fixes
rake_tasks/python.rake[68-94]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `py:local_dev` rake task behavior was changed, but no automated regression test was added/updated alongside it.

## Issue Context
The updated task now builds `//py:selenium-wheel`, clears and repopulates generated directories while resolving symlinks, and aborts if there are uncommitted changes under the destination directory. A focused test should exercise at least one previously-broken scenario (dangling symlink / symlink resolution) so the change is protected from future regressions.

## Fix Focus Areas
- rake_tasks/python.rake[68-94]
- (add a new test file under an existing test framework directory for Ruby/Rake tooling, asserting the new behavior)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Missing runtime resource copies ✓ Resolved 🐞 Bug ≡ Correctness

Description

py:local_dev only copies a directory allowlist, so generated single-file outputs (e.g.
common/mutation-listener.js, common/bidi-mutation-listener.js, firefox/webdriver_prefs.json) are not
staged into py/selenium/webdriver in the default mode. Python code loads these resources at runtime
and will raise (ValueError/FileNotFoundError) when they’re missing.

Code

rake_tasks/python.rake[R72-90]

  bazel_bin = 'bazel-bin/py/selenium/webdriver'
  lib_path = 'py/selenium/webdriver'

-  copy_all = arguments[:all] == 'all'
-  if copy_all
-    FileUtils.rm_rf("#{lib_path}/common/devtools")
-    FileUtils.cp_r("#{bazel_bin}/.", lib_path, remove_destination: true)
-  else
-    bidi_src = "#{bazel_bin}/common/bidi"
-    bidi_dest = "#{lib_path}/common/bidi"
-    if Dir.exist?(bidi_src)
-      FileUtils.mkdir_p(bidi_dest)
-      Dir.children(bidi_src).sort.each do |entry|
-        src = File.join(bidi_src, entry)
-        dest = File.join(bidi_dest, entry)
-        next unless File.file?(src) || File.symlink?(src)
-
-        resolved_src = File.symlink?(src) ? File.realpath(src) : src
-        FileUtils.rm_f(dest)
-        FileUtils.cp(resolved_src, dest)
-      end
-    end
+  dirs = arguments[:all] ? Dir.children(bazel_bin) : %w[common/bidi common/devtools common/linux common/macos common/windows remote]

-    %w[common/devtools common/linux common/macos common/windows].each do |dir|
-      src = "#{bazel_bin}/#{dir}"
-      dest = "#{lib_path}/#{dir}"
-      next unless Dir.exist?(src)
+  dirs.each do |dir|
+    src_dir = "#{bazel_bin}/#{dir}"
+    dest_dir = "#{lib_path}/#{dir}"
+    abort("Commit or stash your changes under #{dest_dir} first") if SeleniumRake.git.diff('HEAD').path(dest_dir).any?

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end
+    FileUtils.rm_rf(dest_dir)
+    # Copy each file individually to resolve Bazel's cache symlinks
+    Dir.glob(File.join(src_dir, '**', '*')).each do |src|
+      next unless File.file?(src)

-    %w[getAttribute.js isDisplayed.js findElements.js].each do |atom|
-      dest = "#{lib_path}/remote/#{atom}"
-      FileUtils.rm_f(dest)
-      FileUtils.cp("#{bazel_bin}/remote/#{atom}", dest)
+      dest = File.join(dest_dir, src.delete_prefix("#{src_dir}/"))
+      FileUtils.mkdir_p(File.dirname(dest))
+      FileUtils.cp(File.realpath(src), dest)
    end

Evidence

The rake task’s default allowlist contains only directories under common/ and remote/, so it can’t
stage Bazel outputs that are files under common/ (root) or under firefox/. Those files are
explicitly produced by Bazel and are required by runtime code paths that load them from package data
/ filesystem.

rake_tasks/python.rake[68-90]
py/BUILD.bazel[105-157]
py/selenium/webdriver/common/log.py[44-54]
py/selenium/webdriver/firefox/firefox_profile.py[35-71]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`py:local_dev` only iterates over a list of *directories* and copies `**/*` beneath them. Several Bazel-generated resources are *files* that live outside those directories (e.g., `common/mutation-listener.js`, `common/bidi-mutation-listener.js`, `firefox/webdriver_prefs.json`), so they never get staged in the default `./go py:local_dev` path, causing runtime failures when Selenium tries to load them.

### Issue Context
- These resources are produced by `copy_file(...)` outputs in `py/BUILD.bazel`.
- Runtime Python code expects them to exist in the source tree when running from source (the purpose of `py:local_dev`).

### Fix Focus Areas
- rake_tasks/python.rake[68-94]
 - Extend the staging logic to handle both directories and individual files.
 - Add explicit handling (in the default mode) to copy:
   - `common/mutation-listener.js`
   - `common/bidi-mutation-listener.js`
   - `firefox/webdriver_prefs.json`
 - Optionally, in `all` mode, detect whether `Dir.children(bazel_bin)` entries are files vs directories and copy root files correctly (copy the file itself, not only `**/*`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Untracked files may be deleted ✓ Resolved 🐞 Bug ☼ Reliability

Description

The safety check only inspects diffs vs HEAD before rm_rf, so untracked files under the
destination directories can be removed without triggering the abort. After deletion, the checkout
restores tracked files from HEAD but cannot restore deleted untracked files.

Code

rake_tasks/python.rake[R80-83]

+    abort("Commit or stash your changes under #{dest_dir} first") if SeleniumRake.git.diff('HEAD').path(dest_dir).any?

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end
+    FileUtils.rm_rf(dest_dir)
+    # Copy each file individually to resolve Bazel's cache symlinks

Evidence

The code deletes the directory tree after only checking diff('HEAD'), which by construction can’t
protect files that don’t exist in HEAD; the later checkout is explicitly limited to tracked files.

rake_tasks/python.rake[77-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Before deleting `dest_dir`, the task only checks `git.diff('HEAD')` scoped to `dest_dir`. Untracked files are not part of `HEAD`, so they can be deleted by `FileUtils.rm_rf(dest_dir)` without warning.

### Issue Context
This task is intentionally destructive (it clears and repopulates directories). The current guard protects modified tracked files but can still lose untracked work (scratch files, local artifacts, etc.) inside these directories.

### Fix Focus Areas
- rake_tasks/python.rake[77-93]
 - Add a guard that also detects untracked files under `dest_dir` (e.g., via `SeleniumRake.git.status` if available, or by invoking `git status --porcelain --untracked-files=all -- <path>` and checking for output).
 - Abort with a clear message listing the offending paths.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Restore comment restates code ✓ Resolved 📘 Rule violation ✧ Quality

Description

The newly added comment # Restore any git tracked files in the directories restates what the
immediately following line does without adding rationale or constraints. This reduces
signal-to-noise in comments and conflicts with the project guideline to prefer rationale-focused
comments.

Code

rake_tasks/python.rake[R92-93]

+    # Restore any git tracked files in the directories
+    SeleniumRake.git.checkout_file('HEAD', dest_dir) unless SeleniumRake.git.ls_files(dest_dir).empty?

Evidence

PR Compliance ID 389275 discourages comments that merely restate code behavior. The comment at
rake_tasks/python.rake is a direct natural-language repetition of the following checkout_file
call, without providing rationale.

Rule 389275: Prefer rationale-focused comments over restating code behavior
rake_tasks/python.rake[92-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A newly added comment restates the code behavior without explaining why the behavior is needed.

## Issue Context
The comment sits directly above a `checkout_file` call whose intent is already clear from the method name and arguments.

## Fix Focus Areas
- rake_tasks/python.rake[92-93]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Any arg triggers full copy ✓ Resolved 🐞 Bug ≡ Correctness

Description

The task now treats any non-nil value for the :all argument as enabling the “copy everything” mode,
so ./go py:local_dev foo behaves like all despite the help text. This makes the task’s behavior
unpredictable and can cause much broader deletion/copying than intended.

Code

rake_tasks/python.rake[75]

+  dirs = arguments[:all] ? Dir.children(bazel_bin) : %w[common/bidi common/devtools common/linux common/macos common/windows remote]

Evidence

The go wrapper forwards arbitrary task args into the rake argument slot, and the new condition only
checks presence rather than equality to the documented string.

rake_tasks/python.rake[68-76]
go[21-40]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`dirs = arguments[:all] ? ...` treats any provided argument value as truthy, not just the documented `all` flag, so arbitrary arguments unexpectedly enable the full-copy path.

### Issue Context
The `./go` wrapper forwards any extra CLI args into the rake task’s bracket args (`task[args]`). With the current truthiness check, any arg value will switch behavior.

### Fix Focus Areas
- rake_tasks/python.rake[75-75]
 - Change the condition to `arguments[:all] == 'all'` (or explicitly validate accepted values and abort on unknown values).
- go[21-40]
 - (No change required, but this shows why arbitrary args are easy to pass.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit ba77862

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Shell-injected git status ✗ Dismissed 🐞 Bug ⛨ Security

Description

py:local_dev runs git status via backticks with an unescaped interpolated path built from
filesystem-derived names (Dir.children in all mode), which allows shell metacharacters in an
entry name to change the executed command. The check also ignores git exit status/stderr, so a
failing git status can be treated as “clean” and the task proceeds to delete/copy anyway.

Code

rake_tasks/python.rake[R88-92]

+  dirs.each do |dir|
+    src_dir = "#{bazel_bin}/#{dir}"
+    dest_dir = "#{lib_path}/#{dir}"
+    abort("Commit or stash your changes under #{dest_dir} first") unless `git status --porcelain #{dest_dir}`.empty?

-      FileUtils.rm_rf(dest)
-      FileUtils.cp_r(src, dest)
-    end

Evidence

The rake task derives dirs from Dir.children(bazel_bin) in all mode, then interpolates the
resulting dest_dir into a backtick shell command. Backticks execute via a shell and the code does
not check the command’s exit status, making this both injection-prone and unreliable as a safety
guard.

rake_tasks/python.rake[75-95]
rake_tasks/bazel.rb[14-52]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`py:local_dev` uses backticks to run `git status --porcelain #{dest_dir}`. In `all` mode, `dest_dir` incorporates directory names from `Dir.children(bazel_bin)`; if any entry name contains shell metacharacters, the backtick invocation can execute unintended shell syntax. Separately, the current code does not validate the command exit status and does not capture stderr, so failures can be misinterpreted as “no changes” and the task proceeds.

### Issue Context
This is a safety guard intended to prevent destructive deletes/copies when the destination has local changes.

### Fix Focus Areas
- rake_tasks/python.rake[75-105]

### Suggested fix
Replace the backtick call with an argv-based subprocess invocation and verify success, e.g.:

```rb
require "open3"

out, status = Open3.capture2e("git", "status", "--porcelain", "--", dest_dir)
abort("git status failed for #{dest_dir}: #{out}") unless status.success?
abort("Commit or stash your changes under #{dest_dir} first") unless out.empty?
```

(Alternatively, if you want to keep backticks, at minimum use `Shellwords.escape(dest_dir)` and check `$CHILD_STATUS.success?`, but argv-based execution is preferred.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit ba77862

[qodo-code-review]

Code review by qodo was updated up to the latest commit 6603661

[Copilot]

Pull request overview

Updates the py:local_dev Rake task to stage Python-generated artifacts for local development by building the wheel target and copying Bazel outputs into the source tree while resolving Bazel cache symlinks.

Changes:

• Build //py:selenium-wheel instead of //py:selenium to ensure generated sources are materialized on disk.
• Replace bulk copies with per-file copies using File.realpath to avoid leaving dangling symlinks after bazel clean.
• Unify “all” vs default behavior around a directory loop plus an explicit file list.

[titusfortner]

I'm merging this without the added guards so users beware we can add them later if there's a problem