fix(review): resolve escaped finding paths and drop findings on an excluded conventions file

[aliasunder]

Summary

When a file path contains a double quote, the prompt writes the path attribute as path="docs/a"b.md". The system prompt tells the model to copy that attribute into a finding's file, and a model that does so writes docs/a"b.md. The unknown-file filter compared that spelling against the raw paths, which hold ", and dropped the finding as "file not in prompt context". Reviewers lost valid findings on related files, related docs, priority docs, and the conventions file whenever the path contained a quote.

The filter now keeps a finding whose file matches a known path only after each " is read as ", and rewrites file to that decoded spelling. A file that matches as written is never rewritten, so a path that literally contains " stays as the model wrote it.

A PR that changes the conventions file can also remove it from the review diff through diff_exclude_paths or linguist rules. The excluded-files note tells the model that file's content was not provided, and the system prompt says not to report findings on excluded paths, yet findings on the conventions file were still kept. The filter now accepts the conventions path only when diff exclusion did not remove it, so those findings are dropped like findings on any other excluded file. The conventions section is still sent unchanged as the review's rubric.

A finding on any diff-excluded file, the conventions file included, is now reported apart from unknown-file drops. It was logged as dropping finding: file not in prompt context and counted as an unknown file, which pointed an operator at context assembly although the path is listed in the excluded-files note. It now logs dropping finding: file excluded from the review diff and has its own count. Which findings are dropped is unchanged.

Changes

• src/review/prompt.ts: adds unescapeAttributeValue beside escapeAttributeValue, decoding only the one entity the encoder writes. Each function's comment names the other so they change together.
• src/review/filter-unknown-file-findings.ts: resolves each finding as known, unescaped, excluded, or unknown, with the written spelling checked first. Takes the diff-excluded paths, normalized like the known paths, and returns findings on them as droppedAsExcludedFile apart from droppedAsUnknownFile. The known check runs first, so the set of dropped findings does not change. Returns the rewrites as unescapedFileRewrites. The JSDoc states the new contract.
• src/orchestrate.ts:
  • Logs each rewrite at debug level (resolved escaped finding file to a prompt path) with the phase, both spellings, and the line. The dropped count and the job summary still count only dropped findings.
  • The per-phase filter summary log is renamed from non-finding filter applied to model output to per-phase finding filters applied to model output, since it also reports unknown-file drops and cross-phase duplicates.
  • The pipeline builds one closure over the prompt paths and the diff-excluded paths and passes it to the per-phase filter step, which no longer forwards path lists. Each excluded-file drop logs dropping finding: file excluded from the review diff at warn with the same fields as the unknown-file warning (phase, file, line, category). The per-phase filter summary log and the job summary carry droppedAsExcludedFile.
  • The conventions path joins the filter's accepted paths only when it is not among the diff-excluded paths, compared after posix.normalize on both sides like the neighbouring priority-doc check. The conventions section of the prompt is unchanged.
• src/review/review-summary.ts: the findings-pipeline table gains a Dropped as excluded file row after Dropped as unknown file.
• action.yml and README findings_count: the removed-findings list names findings on files diff exclusion removed.
• README.md:
  • Unknown-file filter: findings on excluded files are reported as excluded-file drops, and the drop bullet names both warnings and both job-summary rows. How it works step 6 names both drop kinds.
  • Unknown-file filter: lists what the model was given (diff-header paths, changed files, import-traced related files, mention-matched and priority docs, the conventions file), states that excluded files are listed by path at the end of the diff with their content withheld, so findings on them are dropped (an excluded conventions file is still sent as the review's conventions, but findings on it are dropped too), says findings on renamed-from and deleted paths post as standalone comments, states what normalization ignores and that matching is case-sensitive, and adds the decoding rule with its log line.
  • The job-summary bullet and the Non-finding filter section say the job summary shows each filter step's drop count, and use the renamed log message.
  • Both filter sections use one order phrase: each filter runs before cross-phase and cross-run deduplication, the severity threshold, and the cap.
  • How it works: the prompt-tag step, the within-run overlap rule (including which finding a same-severity tie keeps), and the review body are described in plain terms.
  • The beyond-diff bullet says those findings are on files the model was given.
  • The conventions_budget_tokens row splits its reporting conditions into two sentences.
• Tests: new filter cases for the rewrite, a decode that keeps the rest of the written spelling, raw-wins, and neither-spelling-known. A seam test renders a path holding & and " through buildUserPrompt and feeds the attribute back as file. Each matching-rule filter test seeds a finding the rule keeps and one it drops, and new cases cover a leading or trailing / on either side of the match. One orchestrate test asserts the posted path, the debug log, the drop warning, and the filter counts. A second shows a finding filed on the escaped spelling is deduplicated against a prior anchor on the decoded path. Two more run the real prompt builder on a PR that changes the conventions file: when the file is diff-excluded, a finding on it is dropped with the warning, nothing posts, and the prompt still carries the conventions section; the same holds when conventions_file is spelled ./AGENTS.md; when it is not excluded, the finding posts inline. The excluded-file tests assert the excluded-file warning and no unknown-file warning; the plain excluded-file test also asserts the filter summary log and both job-summary rows. New filter cases cover the excluded bucket beside kept and unknown findings, normalized excluded paths, an excluded path reached through " (not rewritten), and a path both known and excluded. A job-summary test pins the whole findings-pipeline section with the new row.

Testing

• npm test: 829 passed (26 files). npm run lint and npm run build are clean.
• Mutation checks, each restored afterwards:
  • An identity decoder failed 6 tests (4 filter, 2 orchestrate), each because the escaped finding was dropped as unknown.
  • Keeping a decoded match without rewriting its file failed the orchestrate dedup test, because the finding was posted again instead of matching the prior anchor.
  • Changing the encoder to " failed the seam test and the two prompt tests that pin the escaped attribute.
  • An encoder that also escapes & as & failed the seam test, whose path holds both & and ".
  • A decoder that also decodes & failed the "decodes only "" test.
  • Checking the decoded spelling before the written one failed the raw-wins test only.
  • A filter that keeps every finding, and one that drops every finding, each failed every matching-rule test individually.
  • Removing the diff-exclusion check on the conventions path failed the excluded-conventions test only, because the finding on the conventions file was kept and posted.
  • Comparing the configured conventions path without normalizing it failed the ./AGENTS.md test only.
  • Routing excluded-path findings back to the unknown bucket failed 7 tests (4 filter, 3 orchestrate excluded-file tests), each because the drop landed in droppedAsUnknownFile with the unknown-file warning.

🤖 Generated with Claude Code

[umm-actually]

umm-actually re-reviewed at d8f0e40

1 new finding(s) posted (8 tracked finding(s) across all runs).

---

umm-actually · deepseek/deepseek-v4.1-flash