Bug fix: bazel-bin/library/tests/dtest -f NNNN did not work when invoked from the repo root

[tameware]

Summary

• Make -f NNNN find hands/listNNNN.txt under the current directory first
• Fall back to the repo hands/ directory relative to bazel-bin/library/tests/dtest, without walking through the bazel-bin symlink into the execroot
• Cover cwd preference, binary-relative fallback, and relative argv0 in args_test

Test plan

• bazel test //library/tests:args_test
• bazel build //library/tests:dtest && ./bazel-bin/library/tests/dtest -f 1 -s solve -n 1
• (cd /tmp && \"$REPO/bazel-bin/library/tests/dtest\" -f 1 -s solve -n 1) succeeds
• bazel run //library/tests:dtest -- -f 100

Made with Cursor

[Copilot]

🟡 Not ready to approve

The new path helpers export unnecessary global symbols and the normalization/absolute-path logic has portability pitfalls (notably POSIX \\ handling and mixed separators on Windows).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR updates dtest’s -f/--file handling so numeric inputs (e.g. -f 100) can be resolved reliably from either the current working directory’s hands/ tree or a hands/ directory relative to the dtest binary location, and adds unit tests covering the resolution behavior.

Changes:

• Added resolve_dtest_input_file() and updated read_args() to use it for -f/--file resolution (cwd-first, then binary-relative fallback).
• Updated dtest usage text to document the new lookup behavior.
• Expanded args_test to cover literal-path preference, cwd preference, binary-relative fallback, and relative argv0 cases.

File summaries

File	Description
library/tests/args.hpp	Declares resolve_dtest_input_file() and documents the new lookup order.
library/tests/args.cpp	Implements path resolution helpers and switches -f parsing to use the resolver; updates help text.
library/tests/args_test.cpp	Adds GoogleTest coverage for the new resolver behaviors and edge cases.

Review details

Suppressed comments (2)

library/tests/args.cpp:212

• normalize_logical_path() is only used within args.cpp. Mark it static (or put it in an unnamed namespace) to avoid leaking internal helper symbols into the global link namespace.

string normalize_logical_path(const string& path)

library/tests/args.cpp:289

• absolute_path_logical() is an implementation detail used only by resolve_dtest_input_file(). Marking it static keeps the symbol local to this translation unit.

string absolute_path_logical(const string& path)

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new tests modify Bazel environment variables without restoring prior values, and one updated error message is inconsistent with the actual resolution behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

library/tests/args_test.cpp:216

• This test mutates BUILD_WORKING_DIRECTORY / BUILD_WORKSPACE_DIRECTORY but does not restore any pre-existing values afterwards (it only clears one variable). Preserve and restore the prior environment to avoid cross-test interference when these variables are set externally.

#ifdef _WIN32
  _putenv_s("BUILD_WORKING_DIRECTORY", "");
  _putenv_s("BUILD_WORKSPACE_DIRECTORY", root_.c_str());
#else
  ASSERT_EQ(unsetenv("BUILD_WORKING_DIRECTORY"), 0);

library/tests/args.cpp:436

• The error message after failing to resolve -f omits the BUILD_WORKING_DIRECTORY / BUILD_WORKSPACE_DIRECTORY fallbacks that resolve_dtest_input_file() actually tries, which can mislead users running under bazel run. Update the message to match the resolution order described in usage/docs.

        cout << "Also tried hands/list" << optarg <<
          ".txt under the current directory and relative to the "
          "dtest binary\n";

library/tests/args_test.cpp:203

• This test mutates BUILD_WORKING_DIRECTORY / BUILD_WORKSPACE_DIRECTORY but only clears them at the end, which can break other tests if the variables were already set in the environment. Save the previous values and restore them after the assertion.

This issue also appears on line 212 of the same file.

#ifdef _WIN32
  _putenv_s("BUILD_WORKING_DIRECTORY", root_.c_str());
  _putenv_s("BUILD_WORKSPACE_DIRECTORY", "");
#else
  ASSERT_EQ(setenv("BUILD_WORKING_DIRECTORY", root_.c_str(), 1), 0);

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new Windows absolute-path detection can misclassify UNC and drive-relative paths, which can break argv0-relative fallback resolution on Windows.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

library/tests/args.cpp:212

• On Windows, is_absolute_path() treats any C: prefix as absolute and does not recognize UNC paths (\\server\share\... / //server/share/...). This can cause absolute_path_logical() / resolve_dtest_input_file() to compute the wrong base directory when argv0 is a UNC path or a drive-relative path like C:bin\\dtest (which is not actually absolute on Windows). Consider tightening the drive-letter check to require a root separator and adding UNC handling.

#ifdef _WIN32
  return path.size() >= 2 && std::isalpha(static_cast<unsigned char>(path[0])) &&
    path[1] == ':';
#else
  return path[0] == '/';

library/tests/args_test.cpp:61

• make_dir() ignores errors from _mkdir/mkdir. If directory creation fails for reasons other than already-existing paths (permissions, invalid/mixed separators on Windows, etc.), the tests will continue and later fail in less obvious ways. Consider making make_dir() report failure (e.g., return bool/int and ASSERT_* at each call site) or explicitly handle EEXIST vs other errors.

void make_dir(const std::string& path)
{
#ifdef _WIN32
  _mkdir(path.c_str());
#else
  mkdir(path.c_str(), 0755);
#endif
}

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

Windows root-relative paths like \repo\... are currently misclassified as non-absolute in the new absolute-path logic, which can break the binary-relative fallback for some valid invocation styles.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

library/tests/args.cpp:250

• On Windows, paths rooted at the current drive like \repo\bazel-bin\... (leading single backslash, not UNC) are considered absolute by Win32 path semantics, but is_absolute_path() currently treats them as relative. This can cause absolute_path_logical(argv0) to prepend the current working directory and break the binary-relative fallback when dtest is invoked with a root-relative argv0.

  if (is_unc_path(path))
    return true;
  // Drive-rooted absolute: "C:\..." or "C:/...". "C:foo" is drive-relative.
  return path.size() >= 3 &&
    std::isalpha(static_cast<unsigned char>(path[0])) &&

library/tests/args.hpp:48

• The is_dtest_absolute_path() doc comment describes Windows absolute paths as only C:\... / C:/... or UNC. If is_absolute_path() is updated to also treat rooted current-drive paths like \path\to\file as absolute, the header comment should reflect that so the contract is accurate for callers and tests.

/// True for a rooted absolute path: POSIX `/...`, Windows `C:\...` / `C:/...`,
/// or UNC `\\server\share\...` / `//server/share/...`. Drive-relative forms
/// like `C:bin\dtest` are not absolute.
bool is_dtest_absolute_path(const std::string& path);

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new make_dir() test helper returns success on EEXIST even if the path is a file, which can mask fixture setup errors and should be corrected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

library/tests/args_test.cpp:67

• make_dir() treats errno == EEXIST as success even if path exists as a regular file, which contradicts the comment and can mask fixture setup errors. Verify the existing path is actually a directory before returning true.

  return errno == EEXIST;
}

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

args_test.cpp uses S_ISDIR in new code, which is not reliably available on Windows/MSVC and can cause build failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

library/tests/args_test.cpp:63

• path_is_directory() uses S_ISDIR, which is not guaranteed to be available on Windows/MSVC (it’s a POSIX macro). This can break Windows builds of args_test. Use S_IFMT/S_IFDIR under _WIN32 (both provided by <sys/stat.h>), and keep S_ISDIR for POSIX.

bool path_is_directory(const std::string& path)
{
  struct stat st;
  if (stat(path.c_str(), &st) != 0)
    return false;
  return S_ISDIR(st.st_mode);
}

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

The updated resolution logic matches the stated requirements and is covered by targeted unit tests for the key invocation scenarios.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.