Evaluate Wayfinder-shaped Bash command canonicalization#78
Conversation
Reviewer's GuideIntroduces a new Wardwright.BashCanonicalizer module that deterministically canonicalizes certain bash git commands (including splitting command chains and removing redundant git -C uses) and returns structured status/diagnostics, along with focused tests and documentation for this experimental evaluation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="app/lib/wardwright/bash_canonicalizer.ex" line_range="126-135" />
<code_context>
+ end
+ end
+
+ defp equivalent_context_path?(_path, nil, nil), do: false
+
+ defp equivalent_context_path?(path, cwd, repo_root) do
+ expanded = expand_path(path, cwd)
+
+ [cwd, repo_root]
+ |> Enum.reject(&is_nil/1)
+ |> Enum.map(&Path.expand/1)
+ |> Enum.any?(&(&1 == expanded))
+ end
+
+ defp expand_path(path, cwd) do
+ if Path.type(path) == :absolute do
+ Path.expand(path)
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid using File.cwd!/0 when :cwd is nil but :repo_root is provided to prevent mis-detecting redundant `git -C`.
In `equivalent_context_path?/3`, when `cwd` is nil but `repo_root` is set, `expand_path/2` still falls back to `File.cwd!/0`. This makes the OS working directory affect whether `git -C` is seen as redundant and can cause us to strip a `-C` that should change context from the agent’s perspective.
Instead of using `File.cwd!/0` here, either treat `path` as non-equivalent when `cwd` is nil, or require explicit `cwd`/`repo_root` values for these checks so equivalence is based only on explicit context, not ambient process state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| defp equivalent_context_path?(_path, nil, nil), do: false | ||
|
|
||
| defp equivalent_context_path?(path, cwd, repo_root) do | ||
| expanded = expand_path(path, cwd) | ||
|
|
||
| [cwd, repo_root] | ||
| |> Enum.reject(&is_nil/1) | ||
| |> Enum.map(&Path.expand/1) | ||
| |> Enum.any?(&(&1 == expanded)) | ||
| end |
There was a problem hiding this comment.
issue (bug_risk): Avoid using File.cwd!/0 when :cwd is nil but :repo_root is provided to prevent mis-detecting redundant git -C.
In equivalent_context_path?/3, when cwd is nil but repo_root is set, expand_path/2 still falls back to File.cwd!/0. This makes the OS working directory affect whether git -C is seen as redundant and can cause us to strip a -C that should change context from the agent’s perspective.
Instead of using File.cwd!/0 here, either treat path as non-equivalent when cwd is nil, or require explicit cwd/repo_root values for these checks so equivalence is based only on explicit context, not ambient process state.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Wardwright.BashCanonicalizer module to canonicalize Bash commands for agent tool calls, along with tests and documentation. The review highlights several critical and high-severity issues: the tokenizer incorrectly strips quotes, which alters command semantics upon reconstruction; the dynamic shell variable check can be bypassed using ${VAR} syntax; splitting && chains discards conditional execution semantics, potentially leading to dangerous behavior; File.cwd!() may raise unhandled exceptions; and backslashes inside single quotes are incorrectly treated as escape characters during splitting and tokenization.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| defp do_tokenize(<<"'", rest::binary>>, current, tokens, :normal), | ||
| do: do_tokenize(rest, current, tokens, :single_quote) | ||
|
|
||
| defp do_tokenize(<<"'", rest::binary>>, current, tokens, :single_quote), | ||
| do: do_tokenize(rest, current, tokens, :normal) | ||
|
|
||
| defp do_tokenize(<<"\"", rest::binary>>, current, tokens, :normal), | ||
| do: do_tokenize(rest, current, tokens, :double_quote) | ||
|
|
||
| defp do_tokenize(<<"\"", rest::binary>>, current, tokens, :double_quote), | ||
| do: do_tokenize(rest, current, tokens, :normal) |
There was a problem hiding this comment.
The tokenizer currently strips single and double quotes from all tokens during the tokenization phase. When a command is reconstructed (e.g., in canonicalize_git_c/5 using Enum.join(["git" | rest], " ")), the original quotes are completely lost.
For example, git -C /workspace/example-repo commit -m 'hello world' will be canonicalized to git commit -m hello world. In Bash, this is a major semantic change because world is no longer part of the commit message and is instead treated as a separate argument.
To fix this, the tokenizer should preserve quotes for all tokens, and the path comparison logic should strip quotes only from the path argument before checking for equivalence.
defp do_tokenize(<<"'", rest::binary>>, current, tokens, :normal),
do: do_tokenize(rest, ["'" | current], tokens, :single_quote)
defp do_tokenize(<<"'", rest::binary>>, current, tokens, :single_quote),
do: do_tokenize(rest, ["'" | current], tokens, :normal)
defp do_tokenize(<<"\"", rest::binary>>, current, tokens, :normal),
do: do_tokenize(rest, ["\"" | current], tokens, :double_quote)
defp do_tokenize(<<"\"", rest::binary>>, current, tokens, :double_quote),
do: do_tokenize(rest, ["\"" | current], tokens, :normal)
| defp dynamic_shell_variable?(command) do | ||
| Regex.match?(~r/(^|[\s;])[_A-Za-z][_A-Za-z0-9]*=\$\(/, command) and | ||
| Regex.match?(~r/(^|[^\$])\$[_A-Za-z][_A-Za-z0-9]*/, command) | ||
| end |
There was a problem hiding this comment.
The dynamic_shell_variable?/1 check can be bypassed by using the standard Bash syntax ${VAR} instead of $VAR. Since the regex ~r/(^|[^\$])\$[_A-Za-z][_A-Za-z0-9]*/ expects a letter or underscore immediately after the $, it fails to match ${VAR}.
If bypassed, a command like FILES=$(rg --files); wc -l ${FILES} will be split into ["FILES=$(rg --files)", "wc -l ${FILES}"] and marked as "rewritten". When executed as separate commands, the variable state is lost, completely breaking the command.
Additionally, the regex matches escaped variables like \$VAR because [^\$] matches the backslash \.
Consider using a lookbehind to avoid matching escaped dollar signs, and support both $VAR and ${VAR} formats.
defp dynamic_shell_variable?(command) do
Regex.match?(~r/(^|[\s;])[_A-Za-z][_A-Za-z0-9]*=\$\(/, command) and
Regex.match?(~r/(?<!\\)\$([_A-Za-z][_A-Za-z0-9]*|\{[\s]*[_A-Za-z][_A-Za-z0-9]*[\s]*\})/, command)
end
| defp do_split_top_level(<<"&&", rest::binary>>, current, commands, :normal) do | ||
| command = current |> Enum.reverse() |> IO.iodata_to_binary() | ||
| do_split_top_level(rest, [], [command | commands], :normal) | ||
| end |
There was a problem hiding this comment.
Splitting && chains into a list of independent commands loses the conditional execution semantics of Bash. In Bash, cmd1 && cmd2 ensures cmd2 only runs if cmd1 succeeds. If these are split and executed sequentially by the caller without checking the exit status of the previous command, it can lead to dangerous behavior (for example, cd /safe/dir && rm -rf * would run rm -rf * in the current directory if the cd failed).
Consider either keeping && chains as a single command (or marking them as repair if they cannot be safely canonicalized together), or returning structured metadata indicating that the commands must be executed conditionally.
| defp expand_path(path, cwd) do | ||
| if Path.type(path) == :absolute do | ||
| Path.expand(path) | ||
| else | ||
| Path.expand(path, cwd || File.cwd!()) | ||
| end | ||
| end |
There was a problem hiding this comment.
Using File.cwd!() can raise a File.Error exception if the current working directory is inaccessible or has been deleted. Since canonicalize/2 is used for preflight tool-call canonicalization, an unhandled exception here could crash the mediation pipeline.
Additionally, if we preserve quotes in do_tokenize/4, we need to strip quotes from the path before checking for equivalence. We can add a helper strip_quotes/1 here.
defp expand_path(path, cwd) do
path = strip_quotes(path)
if Path.type(path) == :absolute do
Path.expand(path)
else
fallback_cwd =
case File.cwd() do
{:ok, dir} -> dir
{:error, _} -> "."
end
Path.expand(path, cwd || fallback_cwd)
end
end
defp strip_quotes(path) do
cond do
String.starts_with?(path, "'") and String.ends_with?(path, "'") ->
String.slice(path, 1..-2)
String.starts_with?(path, "\"") and String.ends_with?(path, "\"") ->
String.slice(path, 1..-2)
true ->
path
end
end
| defp do_split_top_level(<<"\\", char::binary-size(1), rest::binary>>, current, commands, mode) do | ||
| do_split_top_level(rest, [char, "\\" | current], commands, mode) | ||
| end |
There was a problem hiding this comment.
In Bash, backslashes inside single quotes ('...') have no special meaning and do not act as escape characters. However, do_split_top_level/4 handles backslashes regardless of the current mode. This causes backslashes inside single quotes to incorrectly escape the following character (e.g., escaping a single quote, which is impossible in standard Bash single quotes).
We should restrict the backslash clause to only match when mode is :normal or :double_quote.
defp do_split_top_level(<<"\\", char::binary-size(1), rest::binary>>, current, commands, mode) when mode in [:normal, :double_quote] do
do_split_top_level(rest, [char, "\\" | current], commands, mode)
end
| defp do_tokenize(<<"\\", char::binary-size(1), rest::binary>>, current, tokens, mode) do | ||
| do_tokenize(rest, [char | current], tokens, mode) | ||
| end |
There was a problem hiding this comment.
In Bash, backslashes inside single quotes ('...') have no special meaning and do not act as escape characters. However, do_tokenize/4 handles backslashes regardless of the current mode. This causes backslashes inside single quotes to incorrectly escape the following character (e.g., escaping a single quote, which is impossible in standard Bash single quotes).
We should restrict the backslash clause to only match when mode is :normal or :double_quote.
defp do_tokenize(<<"\\", char::binary-size(1), rest::binary>>, current, tokens, mode) when mode in [:normal, :double_quote] do
do_tokenize(rest, [char | current], tokens, mode)
end
There was a problem hiding this comment.
Pull request overview
This PR introduces an experimental Wardwright.BashCanonicalizer to deterministically normalize certain Bash tool-call shapes (e.g., redundant git -C, simple top-level chaining) into forms that are more allowlist/permission-analyzer friendly, along with focused ExUnit coverage and an eval document describing intent and scope.
Changes:
- Added
Wardwright.BashCanonicalizer.canonicalize/2returning a JSON-serializable result (status,commands,diagnostics) and implementing initial rewrite/repair rules. - Added ExUnit tests covering redundant
git -Cremoval, top-level command-chain splitting, quote-aware separator handling, and repair diagnostics. - Added a short evaluation document explaining the experiment and how to run the focused tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| docs/bash-command-canonicalization-eval.md | Documents the experiment’s goals, statuses, and initial covered cases. |
| app/lib/wardwright/bash_canonicalizer.ex | Implements the canonicalization/repair logic and supporting parsing helpers. |
| app/test/bash_canonicalizer_test.exs | Adds targeted ExUnit tests for the new canonicalizer behaviors. |
| defp canonicalize_git_c(original, path, rest, cwd, repo_root) do | ||
| if equivalent_context_path?(path, cwd, repo_root) do | ||
| %{ | ||
| command: Enum.join(["git" | rest], " "), | ||
| status: @rewritten, | ||
| diagnostics: [ | ||
| %{ | ||
| "kind" => "removed_redundant_git_c", | ||
| "message" => "Removed redundant `git -C` because it targeted the active cwd or repo root." | ||
| } | ||
| ] | ||
| } |
| defp dynamic_shell_variable?(command) do | ||
| Regex.match?(~r/(^|[\s;])[_A-Za-z][_A-Za-z0-9]*=\$\(/, command) and | ||
| Regex.match?(~r/(^|[^\$])\$[_A-Za-z][_A-Za-z0-9]*/, command) | ||
| end |
| defp do_split_top_level(<<"&&", rest::binary>>, current, commands, :normal) do | ||
| command = current |> Enum.reverse() |> IO.iodata_to_binary() | ||
| do_split_top_level(rest, [], [command | commands], :normal) | ||
| end |
| test "removes redundant quoted git -C targeting the current cwd" do | ||
| assert %{"commands" => ["git diff --stat"], "status" => "rewritten"} = | ||
| BashCanonicalizer.canonicalize(~s(git -C "/workspace/example-repo" diff --stat), | ||
| cwd: @repo | ||
| ) | ||
| end | ||
|
|
| - `status: "rewritten"` when the command can be safely converted to one or more | ||
| atomic commands. |
Summary
Wardwright.BashCanonicalizerexperiment for deterministic Bash tool-call preflight canonicalization.git -Ccleanup, top-level command-chain splitting, quoted separator preservation, external repo repair, and dynamic shell expansion repair.Verification
git diff --checkpassed locally.cd app && mise exec -- mix test test/bash_canonicalizer_test.exsbecause this worker context has nomix,elixir, ormisebinary available.Notes
This is intentionally a small first slice. Follow-up comparisons can evaluate prompt-complexity route facts, deterministic request classification before Dune/WASM policy evaluation, and receipt-visible router explanations.
Summary by Sourcery
Introduce a Wardwright Bash command canonicalizer for deterministic preflight normalization of agent tool calls and add focused tests and documentation for its initial behaviors.
New Features:
Enhancements:
Documentation:
Tests: