bug(unic-pr-review): re-review delta is unspecified, so the Fetcher improvises a pathspec that drops every path with a space

Summary

agents/ado-fetcher.md Step 4 specifies the re-review delta as an unscoped diff:

git diff "$PRIOR_COMMIT" "$CURRENT_COMMIT" --unified=3

There is no instruction to restrict it to changedFiles. On a branch that has merged its target since the reviewed revision, that diff carries the whole merge, so the Fetcher improvises the scoping — and the improvisation shell-splits the path list. Every path containing a space is dropped from the delta, silently. The Fetcher reports the reduced count as fact, and the re-review proceeds against a diff that is missing most of what the author changed.

Reproduction

ADO PR 5812 (DXP-Website), re-review at iteration 2. Revisions 6 and 7 merged develop into the branch, so the raw revision-1-to-HEAD diff spans 65 files of mostly unrelated churn.

The Fetcher reported:

deltaRawDiff/rawDiff scoped to changedFiles. The raw git diff e1655bc2..73bb2fec spans 65 files and 3368 insertions because iteration 7 merged develop … Restricted to the PR changedFiles list it is 5 files and 86 insertions.

Five files. The real figure is 17. Reproduce the failure directly:

$ git diff --name-only f0d4c208 73bb2fec > pr-files.txt      # 85 PR files

$ git diff --name-only e1655bc2 73bb2fec -- $(cat pr-files.txt | tr '\n' ' ') | wc -l
5

$ node -e "
const fs=require('node:fs'),{execFileSync}=require('node:child_process')
const files=fs.readFileSync('pr-files.txt','utf8').split('\n').filter(Boolean)
console.log(execFileSync('git',['diff','--name-only','e1655bc2','73bb2fec','--',...files],
  {encoding:'utf8'}).split('\n').filter(Boolean).length)
"
17

The twelve lost files are every .yml whose path contains a space, for example:

be/src/Foundation/JavaScriptServices/serialization/Templates/JavaScriptServices/_Prefetching/Dxp Technical Fields/Disable Prefetching.yml
be/src/Project/PortalZRH/serialization/Templates/PortalZRH/Page Templates/Search Page/__Standard Values.yml

git diff does not error on a pathspec that matches nothing, so the loss is silent in both directions: no non-zero exit, no warning, and a plausible-looking file count in its place.

Impact

Those twelve files were the substance of the iteration under review. Revision 4 is described by its own commit message as [42895] make the Disable Prefetching field shared and update Inventory — the field definition flipping Shared: 0 to 1, and every serialised item's value moving out of per-language Versions into SharedFields. None of it reached the aspect agents.

Two of the four Findings this review produced come from files the Fetcher had dropped. Had the run trusted the Fetcher's delta, the re-review would have reported a near-empty iteration and been wrong about it, with nothing in the output to suggest the coverage gap.

git diff succeeding on a partial pathspec also defeats the existing empty-diff guard at ado-fetcher.md:256-262. That guard catches an empty diff against a non-empty changedFiles; it does not catch a diff that is merely incomplete.

Where it goes wrong

agents/ado-fetcher.md:167 — the re-review delta command, unscoped. The scoping the Fetcher adds is unspecified behaviour, so its shape varies per run and nothing constrains it to be correct.

The same gap exists on the first-review path at ado-fetcher.md:247 (git diff "$COMMON_REF_COMMIT" "$SOURCE_REF_COMMIT"), which is unscoped by design. It is unaffected today because a merge-base diff has no unrelated churn to prompt the improvisation — but a Fetcher that decides to scope it there would lose the same files the same way.

Suggested fix

Specify the scoping rather than leaving it to the agent, and specify it space-safe. git diff has no --pathspec-from-file, so the path list cannot go through the shell as a word-split string. Either drive it through an argv array:

node -e "
const fs=require('node:fs'),{execFileSync}=require('node:child_process')
const files=JSON.parse(process.env.CHANGED_FILES)
process.stdout.write(execFileSync('git',['diff',process.env.PRIOR,process.env.CURRENT,'--unified=3','--',...files],
  {encoding:'utf8',maxBuffer:1e9}))
" CHANGED_FILES="<changedFiles JSON>" PRIOR="$PRIOR_COMMIT" CURRENT="$CURRENT_COMMIT"

or use git diff-tree -r -z and reassemble, if a pure-shell form is wanted.

Worth adding alongside it: cross-check the scoped file count against the unscoped one restricted to changedFiles, and warn on a mismatch. That turns this class of silent truncation into a visible one.

Note

Unrelated to #275, though both surfaced in the same run. #275 is wrong resource names on the ADO REST calls; this is diff construction on the git side.

Start with agents/ado-fetcher.md Step 4 at line 167, then compare the first-review path at line 247 and the empty-diff guard at lines 256-262. Reproduce the failure with the issue's git and Node commands, including paths containing spaces. Done means the Fetcher explicitly scopes diffs to changedFiles without shell word splitting and makes incomplete coverage visible.