[rb] resolve spec runfiles via Bazel::Runfiles

[titusfortner]

🔗 Related Issues

Adopts the runfiles library from bazel-contrib/rules_ruby#374 ahead of it merging/releasing.

💥 What does this PR do?

Consolidates the Ruby spec suite's runfiles path resolution onto a single Bazel::Runfiles helper, replacing the hand-rolled rlocation method in test_environment.rb. The pinned browser, driver, and JDK paths now flow through $(rlocationpath ...) in the BUILD files and resolve via the shared library, which additionally understands the runfiles manifest — so resolution works on Windows and RBE where only a manifest exists and RUNFILES_DIR is unset.

🔧 Implementation Notes

The Bazel::Runfiles helper is vendored at rb/lib/bazel/runfiles.rb and loaded with require 'bazel/runfiles'. It sits under rb/lib (the Ruby load-path root, so the require resolves for free) rather than the spec tree, so non-spec code can require it too, and because that path mirrors exactly what rules_ruby#374 exposes. It is not packaged in the gem — the gemspec globs only lib/selenium/webdriver/** — and it is excluded from Steep as build/test tooling. This is an interim vendor: when a rules_ruby release ships Bazel::Runfiles, delete rb/lib/bazel/runfiles.rb, drop the Steep ignore, and add the @rules_ruby//ruby/runfiles dep — every require 'bazel/runfiles' line stays and silently resolves against the ruleset's built-in instead.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude Code (Opus 4.8)
  • What was generated: the vendored runfiles library, the test_environment.rb refactor, and the tests.bzl/BUILD wiring
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

Placed in rb/lib/bazel (not the spec tree) so the BiDi/CDP generators can adopt it next; migrating them (bidi_generate.rb, check_generated.rb#schema_path) needs a small load-path shim under stock rules_ruby and is deferred to a follow-up.

🔄 Types of changes

• Cleanup (formatting, renaming)

[qodo-code-review]

PR Summary by Qodo

Resolve Ruby spec runfiles via Bazel::Runfiles (manifest + dir support)

🐞 Bug fix ⚙️ Configuration changes ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Vendor a Bazel::Runfiles helper to resolve runfiles via directory or manifest.
• Route browser/driver/JDK paths through Runfiles#rlocation in the spec test environment.
• Switch Bazel test env wiring to $(rlocationpath ...) for canonical runfiles paths.

Diagram

graph TD
  T["Bazel rb_test"] --> E["Env: $(rlocationpath ...)"] --> S["test_environment.rb"] --> R["Bazel::Runfiles"] --> D{"Resolve via"}
  D --> M[("RUNFILES_MANIFEST_FILE")] --> P["Resolved abs paths"] --> U["Selenium + JDK"]
  D --> F[("RUNFILES_DIR")] --> P
  subgraph Legend
    direction LR
    _code["Code"] ~~~ _dec{"Decision"} ~~~ _store[("Runfiles store")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Depend directly on @rules_ruby//ruby/runfiles

• ➕ Avoids vendoring/duplication and future drift from upstream implementation
• ➕ Centralizes runfiles behavior across all Ruby targets
• ➖ Requires rules_ruby launcher/RUBYLIB changes (the still-unreleased rules_ruby#374 behavior)
• ➖ Harder to roll out if the repo can’t upgrade rules_ruby immediately

2. Extend the existing hand-rolled rlocation helper

• ➕ Minimal code churn and no new vendored library file
• ➕ Could be narrowly tailored to current spec needs
• ➖ Easy to miss edge cases (manifest parsing, escaping, validation)
• ➖ Continues divergence from Bazel community conventions and upstream fixes

3. Move resolution entirely into Bazel (pass absolute paths)

• ➕ Ruby runtime becomes simpler (no runfiles logic needed)
• ➕ Can be consistent across languages if standardized
• ➖ Absolute paths are less portable across execution environments and can break with sandboxing/RBE
• ➖ Requires careful handling of platform-specific layouts and symlinks

Recommendation: Current approach is the best interim step: it standardizes on Bazel::Runfiles semantics (including manifest support for Windows/RBE) without waiting on a rules_ruby release. Keep the vendored helper clearly scoped (as done) and plan a follow-up to delete it once rules_ruby ships the library on RUBYLIB; consider adding a small unit test for manifest parsing/escaping to reduce regression risk.

Files changed (4) +166 / -44

Bug fix (1) +16 / -23

test_environment.rbRoute spec binary/JDK resolution through Bazel::Runfiles +16/-23

Route spec binary/JDK resolution through Bazel::Runfiles

• Replaces the custom runfiles path resolver with a memoized Bazel::Runfiles instance. Updates all browser/driver/JDK lookups to call runfiles.rlocation and keeps the Windows JDK realpath workaround.

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb

Other (3) +150 / -21

BUILD.bazelNormalize Java toolchain path to canonical rlocation form +3/-1

Normalize Java toolchain path to canonical rlocation form

• Updates the java-location genrule to strip the leading "external/" prefix from $(JAVA). This makes the generated path a canonical rlocation that Bazel::Runfiles#rlocation can resolve directly.

rb/spec/BUILD.bazel

runfiles.rbVendor Bazel::Runfiles resolver (manifest + directory strategies) +127/-0

Vendor Bazel::Runfiles resolver (manifest + directory strategies)

• Adds a vendored Bazel::Runfiles implementation mirrored from rules_ruby#374. Supports both manifest-based and directory-based runfiles resolution and validates rlocation inputs.

rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb

tests.bzlUse $(rlocationpath ...) for pinned browser/driver env vars +20/-20

Use $(rlocationpath ...) for pinned browser/driver env vars

• Switches pinned browser and driver environment variables from $(location ...) to $(rlocationpath ...). This ensures the Ruby resolver receives canonical rlocation paths that work with runfiles manifests as well as runfiles directories.

rb/spec/tests.bzl

[qodo-code-review]

Code Review by Qodo

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

1. Non-hermetic sed genrule ✓ Resolved 🐞 Bug ☼ Reliability

Description

//rb/spec:java-location now depends on a host-provided sed in its cmd, which is not declared as
a tool input and can fail on exec platforms/environments where sed is unavailable. This also
increases platform sensitivity precisely for the JDK path used by grid/remote spec targets.

Code

rb/spec/BUILD.bazel[R60-62]

+    # Strip the "external/" exec-path prefix so the result is a canonical rlocation
+    # path that Bazel::Runfiles#rlocation resolves directly.
+    cmd = "echo $(JAVA) | sed 's|^external/||' > $@",

Evidence

The genrule’s cmd directly invokes sed (not provided via declared tools), and the generated file
is wired into the rb_test remote/grid environment via WD_BAZEL_JAVA_LOCATION, so failures here
prevent the remote spec target from starting.

rb/spec/BUILD.bazel[56-65]
rb/spec/tests.bzl[229-243]

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 `java-location` genrule shells out to `sed` to strip an `external/` prefix. This introduces an undeclared host-tool dependency (non-hermetic) and risks failures on exec platforms without `sed`.

## Issue Context
`java-location` is pulled into remote/grid ruby spec targets as data and used to locate Java at runtime.

## Fix Focus Areas
- rb/spec/BUILD.bazel[56-65]
- rb/spec/tests.bzl[229-243]

## Suggested fix options
Choose one:
1) Remove the `sed` dependency entirely and do normalization in Ruby before calling `runfiles.rlocation`, e.g. strip `external/` or `external\\` and normalize separators.
2) If you must transform in Bazel, make it platform-aware:
  - use `cmd_bash` for Unix and `cmd_ps` (PowerShell) for Windows, avoiding `sed`.
3) Replace the genrule with a small Starlark rule that writes the content without shelling out.

Also consider emitting a clear failure if Java path cannot be resolved rather than producing an empty/incorrect entry.

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

---

2. Nil runfiles breaks Java ✓ Resolved 🐞 Bug ≡ Correctness

Description

TestEnvironment#bazel_java can call File.exist?(resolved) with resolved=nil because
Bazel::Runfiles#rlocation returns nil when runfiles metadata is missing or the entry cannot be
found, which raises and aborts spec startup on Windows. This is a behavioral regression from the old
helper that always returned the original path on failure.

Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[R150-156]

          return unless ENV.key?('WD_BAZEL_JAVA_LOCATION')

          java_path = File.read(File.expand_path(ENV.fetch('WD_BAZEL_JAVA_LOCATION'))).chomp
-          resolved = rlocation(java_path)
+          resolved = runfiles.rlocation(java_path)
+
+          # Resolve the JDK symlink to its real path to dodge a Windows JVM bug mapping lib\modules.
          Platform.windows? && File.exist?(resolved) ? File.realpath(resolved) : resolved

Evidence

Bazel::Runfiles#rlocation delegates to a strategy that can return nil when it can’t resolve the
path; bazel_java then passes that potentially-nil value into File.exist? on Windows, causing a
runtime exception during test environment setup.

rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[53-62]
rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[70-82]
rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[120-124]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[149-157]

Agent prompt

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

## Issue description
`TestEnvironment#bazel_java` assumes `runfiles.rlocation(java_path)` always returns a String, but `Bazel::Runfiles#rlocation` can return `nil` (missing manifest entry, empty RUNFILES_DIR, etc.). On Windows the next line calls `File.exist?(resolved)`, which will raise when `resolved` is `nil`.

## Issue Context
This is most likely to surface when `WD_BAZEL_JAVA_LOCATION` is set (it is for grid/remote tests), but runfiles cannot be resolved (e.g., missing/incorrect manifest, unexpected path format).

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[149-157]
- rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[53-62]
- rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[70-82]
- rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[120-124]

## Suggested fix
- Make `bazel_java` nil-safe, e.g.:
 - `resolved = runfiles.rlocation(java_path) || java_path` (fallback), or
 - raise a clear error if `resolved.nil?` that includes `java_path` and which runfiles mode is active.
- Additionally, guard the Windows realpath branch:
 - `Platform.windows? && resolved && File.exist?(resolved) ? File.realpath(resolved) : resolved`

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

---

3. runfiles_path override untested 📘 Rule violation ☼ Reliability ⭐ New

Description

The new runfiles_path branch that returns an existing on-disk override (File.exist?(value)) is a
behavior change but has no dedicated unit coverage. This makes regressions in local/non-Bazel
execution paths more likely to slip through CI.

Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[407]

+          return value if File.exist?(value) # honor an on-disk override for local runs without runfiles

Evidence

PR Compliance ID 3 requires appropriate tests for behavior changes when feasible. The change at
test_environment.rb:407 adds a new resolution branch for local overrides, but no corresponding
unit test was added to lock in the expected behavior.

AGENTS.md: Add/Update Tests for Implemented Fixes and Prefer Small Unit Tests
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[404-410]

Agent prompt

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

## Issue description
`TestEnvironment#runfiles_path` now returns the raw env value when it points to an existing file (`File.exist?(value)`), but this new behavior is not covered by a small/unit spec.

## Issue Context
This branch is primarily exercised in local/non-Bazel runs (where runfiles metadata may not exist), so it can be missed by Bazel-driven integration CI.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[404-410]

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

---

4. Relative path overrides fail ✓ Resolved 🐞 Bug ☼ Reliability

Description

TestEnvironment#runfiles_path always resolves env values via Bazel::Runfiles and raises on
lookup miss, so when no runfiles metadata exists (e.g., local/IDE execution) even an existing
relative path like ./chromedriver cannot be used for CHROMEDRIVER_BINARY/CHROME_BINARY. This
breaks the documented “paths to a specific browser/driver” override behavior for relative paths and
makes local runs fail deterministically when those vars are set.

Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[R404-409]

+        def runfiles_path(env_key)
+          value = ENV.fetch(env_key, nil)
+          return if value.nil? || value.empty? # a cleared --test_env passes "", which rlocation rejects

-          path
+          runfiles.rlocation(value) || raise("runfiles could not resolve #{env_key}=#{value.inspect}")
        end

Evidence

runfiles_path unconditionally calls runfiles.rlocation(value) and raises if it returns nil.
Bazel::Runfiles.create can legally construct a directory-based strategy with an empty root (when
no runfiles env vars and no *.runfiles artifacts exist), and that strategy’s rlocation returns
nil; this makes any relative, non-runfiles override fail even if it exists on disk. The testing
guide documents these env vars as user-provided “paths”, and the spec environment contains logic
implying non-Bazel execution is supported (e.g., adding bazel-bin outputs to $LOAD_PATH).

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[39-42]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[404-409]
rb/lib/bazel/runfiles.rb[29-47]
rb/lib/bazel/runfiles.rb[117-126]
rb/TESTING.md[233-250]

Agent prompt

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

### Issue description
`runfiles_path` currently raises when `Bazel::Runfiles` can’t resolve an env var value. When specs are run outside Bazel (no `RUNFILES_DIR`, no manifest, no adjacent `*.runfiles` tree), `Bazel::Runfiles.create` falls back to a strategy that returns `nil`, causing `runfiles_path` to raise even if the env var points to an existing relative filesystem path.

### Issue Context
The test suite documentation describes `CHROME_BINARY` / `CHROMEDRIVER_BINARY` (and equivalents) as “Paths to a specific … browser/driver”, which includes common relative-path usage in local/IDE runs. The current implementation only succeeds for absolute paths (which `Bazel::Runfiles#rlocation` returns unchanged) or for values resolvable via runfiles.

### Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[404-409]
- rb/lib/bazel/runfiles.rb[29-47]
- rb/lib/bazel/runfiles.rb[122-126]

### Suggested fix
Update `runfiles_path` to accept already-existing filesystem paths before attempting runfiles resolution, e.g.:

- If `value` is present and `File.exist?(File.expand_path(value))`, return that expanded path (this also avoids `./` being rejected by `Bazel::Runfiles#rlocation`).
- Otherwise, attempt `runfiles.rlocation(value)`.
- Keep the current raise on lookup miss (so Bazel misconfiguration still fails loudly).

Optionally normalize `external/` prefix here as a convenience for manual overrides (similar to the existing `bazel_java` normalization), but only after the `File.exist?` fast-path.

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

---

5. rlocation returns nil silently ✓ Resolved 📘 Rule violation ◔ Observability

Description

Bazel::Runfiles returns nil on lookup misses (or when no runfiles metadata exists) without any
diagnostic logging, and multiple callers directly assign that result to executable/browser paths.
This can make spec failures hard to diagnose when runfiles are misconfigured or incomplete (e.g.,
missing data deps/manifest entries).

Code

rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[R72-84]

+      def rlocation(path)
+        return @entries[path] if @entries.key?(path)
+
+        prefix = File.dirname(path)
+        while prefix != '.' && prefix != '/'
+          base = @entries[prefix]
+          return "#{base}#{path[prefix.length..]}" if base && !base.empty?
+
+          prefix = File.dirname(prefix)
+        end
+
+        nil
+      end

Evidence

PR Compliance ID 8 expects adding diagnostic logging where it improves user insight. The new
runfiles resolver returns nil when it cannot resolve a path, and the refactor assigns that return
value to driver/browser binaries without any logging, which reduces debuggability when resolution
fails.

AGENTS.md: Add User-Relevant Logging Where It Improves Insight
rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[72-84]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[317-333]

Agent prompt

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

## Issue description
`Bazel::Runfiles#rlocation` can return `nil` on lookup miss and does so silently, which can later surface as confusing downstream errors when `nil` is used as a binary/executable path.

## Issue Context
The PR rewires multiple spec paths (drivers/browsers) to use `runfiles.rlocation(...)`. When resolution fails due to missing runfiles metadata or a missing manifest entry, there is currently no log message indicating what path failed to resolve or how to fix it.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[72-84]
- rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[122-126]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[133-138]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[317-333]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[351-373]

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

---

View more (3)

6. Empty env crashes rlocation ✓ Resolved 🐞 Bug ≡ Correctness

Description

Bazel::Runfiles#rlocation raises ArgumentError for empty strings, but TestEnvironment only checks
ENV.key? before calling it. If a user clears a Bazel-set variable by passing an empty value via
--test_env, spec startup/configuration will now raise instead of treating it as unset.

Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[R131-139]

        def driver_path
          env = {chrome: 'CHROMEDRIVER_BINARY', edge: 'MSEDGEDRIVER_BINARY', firefox: 'GECKODRIVER_BINARY'}[browser]
-          rlocation(ENV.fetch(env)) if env && ENV.key?(env)
+          runfiles.rlocation(ENV.fetch(env)) if env && ENV.key?(env)
        end

        def browser_path
          env = "#{browser.to_s.upcase}_BINARY"
-          rlocation(ENV.fetch(env)) if ENV.key?(env)
+          runfiles.rlocation(ENV.fetch(env)) if ENV.key?(env)
        end

Evidence

Runfiles#rlocation explicitly raises when given an empty string; TestEnvironment’s new call sites
only check ENV.key? and then pass ENV.fetch/ENV[...] directly, so an empty-but-present env var will
raise. The repo docs encourage overriding these via --test_env, which can produce empty-but-present
values.

rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[53-64]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[131-139]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[313-334]
rb/TESTING.md[233-250]

Agent prompt

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

### Issue description
`Bazel::Runfiles#rlocation` raises on empty strings, but `TestEnvironment` calls it as long as `ENV.key?(...)` is true. In Ruby, an environment variable can be present with an empty value, making `ENV.key?` true and causing an immediate `ArgumentError`.

### Issue Context
`rb/TESTING.md` instructs users to override variables using `--test_env`, and clearing a variable is commonly done by setting it to an empty string in Bazel invocations.

### Fix Focus Areas
- Guard on non-empty values before calling `runfiles.rlocation` (e.g., `val = ENV[VAR]; return if val.nil? || val.empty?`).
- Apply consistently across `driver_path`, `browser_path`, and the per-browser option/service assignments.

### Fix Focus Areas (code pointers)
- rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[53-64]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[131-139]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[313-334]
- rb/TESTING.md[233-250]

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

---

7. Bazel::Runfiles lacks unit specs ✓ Resolved 📘 Rule violation ☼ Reliability

Description

This PR introduces new runfiles resolution logic (manifest- and directory-based) and rewires spec
runtime path lookup to depend on it, but adds no automated unit coverage for the new behavior.
Without tests, regressions in Windows/RBE manifest resolution and path validation are more likely to
slip through CI unnoticed.

Code

rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[R25-125]

+module Bazel
+  # Resolves runtime paths to data dependencies using either a
+  # manifest file or a runfiles directory.
+  class Runfiles
+    def self.create(env = ENV)
+      manifest_file = env['RUNFILES_MANIFEST_FILE']
+      runfiles_dir = env['RUNFILES_DIR']
+
+      return new(ManifestBased.new(manifest_file)) if manifest_file && !manifest_file.empty?
+      return new(DirectoryBased.new(runfiles_dir)) if runfiles_dir && !runfiles_dir.empty?
+
+      create_from_program_name($PROGRAM_NAME)
+    end
+
+    def self.create_from_program_name(program_name)
+      if File.exist?("#{program_name}.runfiles_manifest")
+        new(ManifestBased.new("#{program_name}.runfiles_manifest"))
+      elsif File.exist?("#{program_name}.runfiles")
+        new(DirectoryBased.new("#{program_name}.runfiles"))
+      else
+        new(DirectoryBased.new(''))
+      end
+    end
+
+    def initialize(strategy)
+      @strategy = strategy
+    end
+
+    def rlocation(path)
+      raise ArgumentError, 'path must not be empty' if path.to_s.empty?
+
+      invalid_path = %r{\A\.\.[/\\]|[/\\]\.\.[/\\]|\A\.[/\\]|[/\\]\.[/\\]|[/\\]\.\z|[/\\][/\\]}
+      raise ArgumentError, "path is not valid: #{path.inspect}" if path.match?(invalid_path)
+
+      return path if Pathname.new(path).absolute?
+
+      @strategy.rlocation(path)
+    end
+
+    # Resolves paths by looking them up in a runfiles MANIFEST file.
+    class ManifestBased
+      def initialize(manifest_path)
+        @entries = parse_manifest(manifest_path)
+      end
+
+      def rlocation(path)
+        return @entries[path] if @entries.key?(path)
+
+        prefix = File.dirname(path)
+        while prefix != '.' && prefix != '/'
+          base = @entries[prefix]
+          return "#{base}#{path[prefix.length..]}" if base && !base.empty?
+
+          prefix = File.dirname(prefix)
+        end
+
+        nil
+      end
+
+      private
+
+      def parse_manifest(path)
+        entries = {}
+        return entries unless File.exist?(path)
+
+        File.foreach(path) do |line|
+          line.chomp!
+          next if line.empty?
+
+          key, value = parse_entry(line)
+          entries[key] = value
+        end
+
+        entries
+      end
+
+      def parse_entry(line)
+        escaped = line.delete_prefix!(' ')
+        key, _, value = line.partition(' ')
+        return [key, value] unless escaped
+
+        [unescape(key), unescape(value)]
+      end
+
+      def unescape(str)
+        str.gsub(/\\[snb]/, '\s' => ' ', '\n' => "\n", '\b' => '\\')
+      end
+    end
+
+    # Resolves paths by joining them onto a runfiles directory root.
+    class DirectoryBased
+      def initialize(runfiles_dir)
+        @runfiles_dir = runfiles_dir
+      end
+
+      def rlocation(path)
+        return nil if @runfiles_dir.empty?
+
+        File.join(@runfiles_dir, path)
+      end
+    end

Evidence

PR Compliance ID 5 requires adding/updating automated tests for behavioral changes when feasible.
The PR adds a new runfiles resolver (Bazel::Runfiles) and updates the spec environment to use it,
but does not introduce any unit spec coverage for the new resolution behavior (manifest parsing,
directory fallback, and path validation).

AGENTS.md: Add or Update Tests for Implemented Fixes/Features (Prefer Small Unit Tests)
rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[25-125]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[131-403]

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 new `Bazel::Runfiles` implementation was added and is now relied on by `test_environment.rb`, but there are no accompanying unit tests validating manifest parsing, directory-based resolution, and invalid path handling.

## Issue Context
The PR’s stated goal is correct resolution in environments where only a runfiles manifest exists (e.g., Windows/RBE). These branches are easy to break without focused unit tests.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[25-125]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[131-403]
- rb/spec/unit/selenium/webdriver/spec_support/bazel/runfiles_spec.rb[1-200]

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

---

8. UNC absolute paths rejected ✓ Resolved 🐞 Bug ≡ Correctness

Description

Bazel::Runfiles#rlocation validates the path before checking if it is absolute, and the invalid-path
regex rejects double backslashes, so valid Windows UNC absolute paths (e.g. \\server\share\file)
raise ArgumentError instead of being returned unchanged. This undermines the method’s own “return
absolute paths as-is” behavior.

Code

rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[R53-60]

+    def rlocation(path)
+      raise ArgumentError, 'path must not be empty' if path.to_s.empty?
+
+      invalid_path = %r{\A\.\.[/\\]|[/\\]\.\.[/\\]|\A\.[/\\]|[/\\]\.[/\\]|[/\\]\.\z|[/\\][/\\]}
+      raise ArgumentError, "path is not valid: #{path.inspect}" if path.match?(invalid_path)
+
+      return path if Pathname.new(path).absolute?
+

Evidence

The implementation checks invalid_path and raises before it checks Pathname.new(path).absolute?,
and the test environment code calls out UNC behavior on Windows—making this ordering a practical
edge-case risk.

rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[53-61]
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[212-216]

Agent prompt

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

## Issue description
`Runfiles#rlocation` applies an `invalid_path` regex check before the absolute-path early return. The regex rejects `[/\\][/\\]`, which matches the leading `\\` in UNC paths. As a result, UNC absolute paths raise even though absolute paths should be returned unchanged.

## Issue Context
The surrounding test codebase is Windows-aware and explicitly mentions UNC path issues, so UNC paths are plausible in real executions.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb[53-62]
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[212-216]

## Suggested fix
- Move the absolute-path check before the invalid-path validation, or
- Adjust validation so it only applies to relative runfiles paths (and/or explicitly exempt UNC prefixes like `\\`), while still rejecting dangerous relative patterns (`..`, `./`, `//` within relative paths).

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

---

9. Runfiles comment restates behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The new comment above runfiles explains what the method does rather than why this approach is
needed, which reduces long-term maintainability when future readers revisit the Bazel/runfiles
integration decisions. Prefer capturing rationale (e.g., manifest support on Windows/RBE) instead of
restating obvious behavior.

Code

rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[R401-403]

+        # Resolves Bazel $(rlocationpath ...) values to absolute paths in the runfiles tree.
+        def runfiles
+          @runfiles ||= Bazel::Runfiles.create

Evidence

PR Compliance ID 9 requires comments to focus on rationale (“why”) rather than restating behavior
(“what”). The added comment above def runfiles describes behavior without documenting the
underlying constraints motivating the helper.

AGENTS.md: Comments Should Explain Why, Not What
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[401-403]

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 explains what the `runfiles` helper does instead of the rationale/constraints for why it exists.

## Issue Context
Per repo guidance, comments should capture intent and non-obvious reasoning so future maintainers understand why this resolution strategy is necessary.

## Fix Focus Areas
- rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb[401-403]

ⓘ 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 aad21e1

[Copilot]

Pull request overview

This PR consolidates Ruby integration spec runfiles path resolution by introducing a shared Bazel::Runfiles helper (vendored from rules_ruby#374) and switching spec configuration to use Bazel $(rlocationpath ...) values, enabling manifest-based resolution for environments where RUNFILES_DIR is not set (e.g., Windows/RBE).

Changes:

• Switches pinned browser/driver env vars in rb/spec/tests.bzl from $(location ...) to $(rlocationpath ...).
• Replaces the hand-rolled rlocation helper in test_environment.rb with Bazel::Runfiles.create and runfiles.rlocation(...).
• Adds a vendored Bazel::Runfiles implementation under spec_support/bazel/runfiles.rb.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
rb/spec/tests.bzl	Updates spec env var wiring to use $(rlocationpath ...) for runfiles-friendly paths.
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb	Uses Bazel::Runfiles for resolving Bazel-provided paths (including manifest-based resolution).
rb/spec/integration/selenium/webdriver/spec_support/bazel/runfiles.rb	Adds vendored Bazel::Runfiles helper used by the Ruby spec suite.

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 37898fa

[qodo-code-review]

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

[qodo-code-review]

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

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3f2938b

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.