Repository navigation
fix: match each **/ independently, and decode git-quoted paths in diffs - #82
Merged
Merged
Conversation
…iffs `patternVariants` only tested a pattern with every `**/` kept or every one removed, so `**/ios/**/*.swift` missed `ios/src/A.swift` and `packages/ios/A.swift`. It now tests every keep/remove combination, up to six `**/` (64 variants). Git quotes a path with special characters, and with the default `core.quotePath` every non-ASCII byte as octal (`"b/src/caf\303\251.ts"`). GitHub's diffs quote the same way. The parser kept the quotes and the `b/` prefix, so the path matched no glob (noise, routing) and pointed at a file that doesn't exist. A pure rename of such a file has no `---`/`+++` lines, so the header regex didn't match either, and the file dropped out of the review entirely. `decodeGitPath` now decodes every path from the `---`/`+++` lines and from the header.
…d-paths # Conflicts: # llp/0004-diff-noise-and-prompts.explainer.md # src/__tests__/diff.test.ts # src/config/routing.ts
🤖 AI code reviewDecision: Ready for human review Overall PR risk: Medium. This change modifies how diffs parse paths and match patterns against scopes. It decodes git-quoted paths (which may change their effective filenames) and expands
No findings. This review is advisory — it never blocks a merge and never auto-approves. |
Review of #82 found that decoding quoted paths into raw characters opened new problems, and that the parser had two older ones in the same function. - `decodeGitPath` now keeps a quoted path as is when its bytes are not valid UTF-8 or the decoded name has a control, format or line-separator character. A decoded newline in a PR-chosen file name could forge an `----- END DIFF` line in reviewer prompts (`sanitizeUntrusted` keeps newlines). A newline or U+2028 also stopped `**` from matching. Lenient UTF-8 turned distinct invalid names into one U+FFFD name. - Header lines are read only before the first `@@`. An added line `++ b/package-lock.json` read as `+++`, so a PR could rename its own entry to a lockfile, which noise filtering then dropped unreviewed. - A pure rename takes its path from `rename to`, not the header, which is ambiguous when a bare path contains " b/". - `ecr ref-check` checks scope globs with `matchesPathGlobs`, the matcher scope assignment uses, instead of its own copy of the old all-or-nothing variants.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two path-matching bugs that predate #81 and that #81's agent
pathsfilter inherits. A multi-model review of #81 found them.**/a/**/bmisses some depths.patternVariantsonly tested a pattern with every**/kept or every one removed. So**/ios/**/*.swiftmissedios/src/A.swiftandpackages/ios/A.swift. This affectsrouting.jsoncscopes today, and will affect agentpathsonce feat: add a per-agentpathsfilter to agent frontmatter #81 lands.core.quotePathevery non-ASCII byte as octal, e.g."b/src/caf\303\251.ts". GitHub's diffs quote the same way; I checked a real public commit diff. The parser kept the quotes and theb/prefix, so:paths);---/+++lines, and the header regex didn't match a quoted header, so the file dropped out of the review entirely.How
patternVariantstests every keep/remove combination of each**/. Patterns with more than six fall back to the old all-or-nothing pair, so the variants stop at 64.decodeGitPathdecodes C-quoted paths: octal escapes become UTF-8 bytes, plus\",\\,\tand the other C escapes. A path stays quoted, as before, when its bytes aren't valid UTF-8 or the decoded name has a control, format or line-separator character. Otherwise a PR-chosen file name containing a newline could forge an----- END DIFFline in reviewer prompts, defeat**matching, or make distinct invalid names collide. It runs on every path from the---/+++lines and from thediff --githeader. The header parser accepts quoted sides, and a rename can mix a quoted side with a bare one.---,+++,rename) are read only before the first@@. Before, an added line++ b/package-lock.jsonread as+++, so a PR could rename its own entry to a lockfile, which noise filtering dropped unreviewed. This bug predates feat: add a per-agentpathsfilter to agent frontmatter #81.rename to, not from the header, which is ambiguous when a bare path containsb/.ecr ref-checkchecks scope globs with the same matcher as scope assignment.Merging with #81
#81 has landed; this branch merges
mainand resolves the conflicts. #81's newrename fromvalue now also goes throughdecodeGitPath, so a quoted rename's old path matches agentpaths.Test Plan
bun test(793 pass),bun run typecheck,bun run lint,bun run fmt:checkandbun run llp:checkall pass. New tests cover:**/matching inresolveScopes; this one fails on the old matcher;decodeGitPathescapes;+++/---lines inside a hunk;🤖 Generated with Claude Code