Skip to content

code-coach review: PATH/CLI debuggability rollout — 3 advisory suggestions #2

@psmon

Description

@psmon

Source

Verdict

Commit approved — 0 must-fix, 0 should-fix, 3 advisory suggestions. The
3 items below are all micro-scale (defensive coverage of corner cases that
do not occur on any current invocation path) and did not block the merge.
Filed per harness v1.1.4 issue-handoff policy (any finding ≥ Suggestion).

Suggestions

S-A — workDir double-quote corner case

File: Project/AgentZeroWpf/UI/APP/MainWindow.xaml.cs:1581

The new pushd "{workDir}" form quotes the directory path so paths with
spaces work. If workDir itself happens to contain a " character (e.g.
the user typed a literal quote in a workspace directory entry), the cmd
line breaks. _cliGroups[].DirectoryPath is filesystem-derived in normal
flow so this is mostly hypothetical, but a Replace("\"", "") or an
explicit reject in CliGroupInfo would close the gap.

S-B — rawCmd nested-quote depth

File: Project/AgentZeroWpf/UI/APP/MainWindow.xaml.cs:1581

Today the four shells (CMD / PW5 / PW7 / Claude) all expand to plain
commands without their own outer quotes, so the new wrapper's quoting
works. If a future CliDefinition ships with a quoted argument string
(e.g. pwsh.exe -NoExit -Command "claude --resume"), the cmd's outer
/c "..." plus the inner "..." plus rawCmd's "..." reach a depth
that cmd's parser won't reliably handle. A small unit test that builds
the cmdLine for each seeded CliDefinition would catch regressions
when new ones are added.

S-C — AppVersionProvider exception narrowing

File: Project/ZeroCommon/Module/AppVersionProvider.cs:24

try { ... }
catch { /* fall through to assembly attribute */ }

Catches every exception, including OutOfMemoryException /
StackOverflowException etc., to fall back to the assembly attribute.
Narrowing to the realistic set keeps the contract explicit:

catch (Exception ex) when (ex is IOException
                        || ex is UnauthorizedAccessException
                        || ex is System.Security.SecurityException)
{ /* fall through */ }

Pure documentation/contract win — the catch covers exactly what the
File operations can throw, no more.

Recommendation

All three are defensive-coverage upgrades with no observed failure on
today's workflows. Deferring as a single cleanup PR is reasonable; none
of them block the immediate goal (debuggable PATH/CLI). S-C is a
one-line change and the fastest to land if a contributor wants a small
warmup task.

Closes-when

  • S-A: workDir sanitation in place (sanitize on entry or reject at edit time)
  • S-B: cmdLine build covered by a unit test that walks every seeded CliDefinition
  • S-C: exception filter narrowed in AppVersionProvider

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions