Fix dtest -f relative paths under bazel run

[tameware]

Summary

• Resolves literal relative -f paths (e.g. hands/list1.txt) via BUILD_WORKING_DIRECTORY / BUILD_WORKSPACE_DIRECTORY and relative to the dtest binary, so bazelisk run //library/tests:dtest -- -f hands/list1.txt works.
• Numeric list shorthand (-f 1 → hands/list1.txt) behavior is unchanged; docs and error text updated to match.
• Adds unit coverage for literal relative resolution under bazel env dirs and binary-relative fallback.

Fixes #328

Test plan

• bazelisk test //library/tests:args_test
• bazelisk run //library/tests:dtest -- -f hands/list1.txt -s calc
• Confirm bazel-bin/library/tests/dtest -f hands/list1.txt still works from the repo root

Made with Cursor

[Copilot]

Pull request overview

Updates dtest argument handling so literal relative -f paths resolve correctly when running under bazelisk run, using Bazel-provided environment directories and an argv0-based fallback to locate the workspace hands/ files.

Changes:

• Extends -f/--file resolution to retry literal relative paths under BUILD_WORKING_DIRECTORY / BUILD_WORKSPACE_DIRECTORY, then relative to the inferred workspace root from argv0.
• Updates usage/docs/error output to describe the new resolution behavior.
• Adds unit tests covering Bazel env-dir resolution for literal relative paths and binary-relative fallback.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
library/tests/args.hpp	Updates documentation for -f input-file resolution order.
library/tests/args.cpp	Implements Bazel env-dir + binary-relative fallback for literal relative -f paths; updates help/error text.
library/tests/args_test.cpp	Adds tests verifying literal relative path resolution via Bazel env dirs and argv0 fallback.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[tameware]

Addressed both Copilot review notes:

• Clarified resolve_dtest_input_file docs: list shorthand is for numeric ids; absolute paths only try the literal path.
• Reworded the missing-file hint so it no longer concatenates a path arg into hands/list<path>.txt.

[Copilot]

Pull request overview

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

Suppressed comments (1)

library/tests/args.cpp:333

• Absolute -f values that don't exist still fall through to the list-shorthand/env/binary-relative search (e.g., potentially resolving to an unrelated hands/list*.txt file). This contradicts the updated contract in args.hpp (“Absolute paths only attempt the literal path as given.”) and can produce surprising results.

    return cwd_candidate;

[tameware]

Addressed the follow-up Copilot note on absolute -f values: a missing absolute path now returns empty immediately instead of falling through to hands/list{arg}.txt lookup. Added ResolveAbsoluteMissingDoesNotUseListShorthand.

[Copilot]

Pull request overview

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

Suppressed comments (2)

library/tests/args.hpp:41

• The header comment says the final fallback is "relative to the directory of argv0", but the implementation actually infers the workspace root by climbing four parents from argv0 (bazel-bin/.../dtest) and then resolves relative paths from that root. This is a documentation mismatch that could confuse callers.

/// `bazel run`); then those same two forms relative to the directory of
/// `argv0` (the usual `bazel-bin/library/tests/dtest` layout). Absolute
/// paths only attempt the literal path as given.

library/tests/args_test.cpp:402

• This test assumes the OS path /no_such_dtest_abs does not exist. If it happens to exist on a developer machine or CI image, the test will fail even though the resolver logic is correct. Consider choosing a missing absolute path dynamically and creating the corresponding "trap" file based on that chosen path.

  EXPECT_TRUE(
    resolve_dtest_input_file("/no_such_dtest_abs", binary_path_).empty());

[tameware]

Addressed the latest Copilot notes:

• Docs/usage now say the argv0 fallback uses the workspace root inferred by climbing four parents from the binary (not “relative to the directory of argv0”).
• ResolveAbsoluteMissingDoesNotUseListShorthand now chooses a unique missing absolute path and builds the list-shorthand trap from that path.

[Copilot]

Pull request overview

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

Suppressed comments (1)

library/tests/args_test.cpp:272

• The comment references -f hands/list1.txt, but this test resolves hands/list42.txt. This is misleading when debugging bazel path resolution behavior; update the comment to match the test input.

  // bazelisk run //library/tests:dtest -- -f hands/list1.txt must find the
  // path relative to the invoke-time shell cwd, not the runfiles tree.

[tameware]

Addressed the latest Copilot note: the ResolveLiteralRelativeUsesBazelWorkingDirectory comment now refers to hands/list42.txt, matching the path the test resolves.

[Copilot]

Pull request overview

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

Suppressed comments (2)

library/tests/args.cpp:399

• Similarly for the binary-relative fallback: root/arg is tried before root/hands/list{arg}.txt. This can cause numeric -f 42 to resolve to a workspace-root file named 42 instead of the documented hands/list42.txt if both exist. To keep numeric shorthand semantics stable, prefer the list-shorthand candidate under the inferred workspace root before the literal candidate.

  const string bin_literal =
    normalize_logical_path((root / arg).string());
  if (path_exists(bin_literal))
    return bin_literal;

library/tests/args.cpp:389

• resolve_dtest_input_file() now checks BUILD_WORKING_DIRECTORY/arg and BUILD_WORKSPACE_DIRECTORY/arg before the numeric shorthand hands/list{arg}.txt under those env dirs. This changes numeric -f 42 resolution if a file named 42 exists in the workspace (contradicting the PR description that numeric shorthand behavior is unchanged). Consider trying the list-shorthand candidate under the Bazel env dirs before the literal arg candidate.

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

  if (const string found =
        from_env_dir("BUILD_WORKING_DIRECTORY", arg); !found.empty())
  {
    return found;
  }

[tameware]

Addressed the latest Copilot notes on resolution order:

• Under BUILD_WORKING_DIRECTORY / BUILD_WORKSPACE_DIRECTORY and the argv0 workspace root, list shorthand (hands/listN.txt) is tried before a bare relative name.
• Added ResolveNumericPrefersListOverLiteralUnderBazelWorking and ResolveNumericPrefersListOverLiteralRelativeToBinary.
• Updated the resolve_dtest_input_file docs to match.

[Copilot]

Pull request overview

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

Suppressed comments (2)

library/tests/args.cpp:345

• from_env_dir() joins dir / rel even when rel has a root-name (e.g. Windows drive-relative C:foo). In that case std::filesystem::operator/ can discard dir, so BUILD_* / argv0-root fallback can be bypassed unexpectedly. Consider rejecting such inputs (treat them as non-relative) so the join behaves consistently.

    const char* dir = std::getenv(env_name);
    if (dir == nullptr || dir[0] == '\0')
      return string();
    const string candidate =
      normalize_logical_path((fs::path(dir) / rel).string());

library/tests/args.cpp:473

• The “Also tried …” message is misleading for absolute -f arguments: resolve_dtest_input_file() returns early for missing absolute paths and does not try BUILD_* or argv0-root fallbacks. Consider wording this as conditional (only for relative paths).

        cout << "Also tried that path under the current directory, "
          "BUILD_WORKING_DIRECTORY, BUILD_WORKSPACE_DIRECTORY, "
          "and under the workspace root inferred from the dtest binary; "
          "for numeric -f N, also hands/listN.txt\n";

[tameware]

Addressed the latest Copilot notes:

• Drive-relative -f values (Windows C:foo) are no longer joined under BUILD_* / argv0; only the literal path is tried. Added ResolveDriveRelativeMissingDoesNotJoinUnderBazelDirs (Windows).
• The missing-file “Also tried …” hint is only printed for portable relative paths, not absolute/drive-relative ones.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

library/tests/args.cpp:338

• resolve_dtest_input_file() still applies the list-shorthand transformation (list{arg}.txt) even when arg is already a relative path containing directory separators (e.g. hands/list42.txt), producing nonsense candidates like hands/listhands/list42.txt.txt. This can lead to surprising resolution if such a path happens to exist, and it also means the resolver is trying paths that the updated docs/error text no longer describe. Consider disabling list-shorthand when the argument already looks like a path (contains / or \\), and (optionally) only enabling shorthand for purely numeric tokens.

  const string list_name = "list" + arg + ".txt";
  // Keep generic separators so cwd hits match the documented hands/listN.txt form.
  const string cwd_candidate =
    (fs::path("hands") / list_name).generic_string();
  if (path_exists(cwd_candidate))

[zzcgumn]

I just wish that there was an easier way to handle file paths :(.

[tameware]

Addressed the latest Copilot note on list shorthand for path-like -f args:

• List shorthand (hands/listN.txt) is now only applied for purely numeric tokens.
• Path-like args (e.g. hands/list42.txt) no longer probe concatenated shorthand candidates.
• Added ResolvePathLikeArgDoesNotUseListShorthand; updated resolve_dtest_input_file docs.

[Copilot]

Pull request overview

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

Suppressed comments (2)

library/tests/args.cpp:321

• is_dtest_list_shorthand_arg() is a TU-local helper but is currently emitted with external linkage, unlike the other helper functions in this file (kept in the unnamed namespace). Making it static avoids exporting an unnecessary symbol and prevents potential name collisions at link time.

bool is_dtest_list_shorthand_arg(const string& arg)

library/tests/args_test.cpp:303

• This test uses the throwing overload of std::filesystem::remove(). If the remove fails unexpectedly (permissions, transient FS issues), it will throw and abort the test binary instead of producing a clear assertion failure. Prefer the error_code overload here (and assert success).

  std::filesystem::remove(root_ + "hands/list42.txt");