fix(report)!: address CI files from the repository root#1808
Conversation
When --root pointed at a package inside a larger repo, fallow's CI output was unusable in two ways that shared one cause: findings were keyed and emitted relative to the analysis root, while CI platforms and git address files from the repository root. Report paths. codeclimate, review-github, and review-gitlab emitted --root-relative paths, so GitLab's Code Quality widget matched nothing and every inline review discussion was rejected for naming a file absent from the MR diff. github-annotations already rebased onto the git toplevel; the others now do too, via the same PathRebase. The review and sticky-summary formats derive their paths from CodeClimate issues, so one seam covers them all. --annotations-path-prefix becomes --report-path-prefix (old name kept as an alias) and governs every CI format. Both names are now registered in global_value_options, fixing a latent misparse of the following subcommand. The generated capability manifest and CLI reference are regenerated for the rename. The prefix is presentation only. The review and sticky-summary filters used to key issues by their rendered path, so a custom or empty prefix silently stopped matching the diff and dropped every inline comment. They now key by the analysis-root-relative path plus the diff's own base. Diff filtering. --diff-file compared diff keys against finding paths already stripped to --root, so below the toplevel nothing matched: every source-anchored finding was dropped and the run reported a clean diff, exit 0, silently. A unified diff does not declare its own base, and both conventions occur in practice -- `git diff` writes toplevel-relative paths, `git diff --relative` writes root-relative ones. The base is therefore resolved from the diff's paths themselves: whichever candidate directory they actually name files under wins. The chosen base rides on DiffIndex, the type that defines the namespace, so every filter call site picks it up and callers that build their own index keep today's behavior. Existence cannot always place a diff. A path naming a real file under both candidates is ambiguous, and a path naming one under neither is foreign. Both are reported. Silence here would rebuild the very failure this change removes: a filter that matches nothing looks exactly like a clean diff. When --root is the repository toplevel the two candidates coincide and output is byte-identical, which the untouched snapshot suite pins. SARIF is deliberately excluded: four builders and its own URI encoding make it a separate change, so it is left out of the flag's allowlist rather than half-rebased. BREAKING CHANGE: codeclimate, review-github, and review-gitlab now emit repository-root-relative paths when --root is a subdirectory. Consumers that prepend the offset themselves should drop that step or pass --report-path-prefix '' to restore the previous output.
Jerc92
force-pushed
the
fix/monorepo-report-paths
branch
from
5402659 to
78f10aa
Compare
|
| gate | wants cli-reference.md to say |
source of truth |
|---|---|---|
generate:contracts:check |
--report-path-prefix |
the live CLI |
Skills vendor drift |
--annotations-path-prefix |
fallow-rs/fallow-skills |
Verified locally: restoring the old row makes generate:contracts:check fail with generated contract surfaces are stale; keeping the regenerated row makes the drift gate fail. No state of this branch satisfies both.
This is inherent to changing a CLI flag's name or help text, since cli-reference.md is generated from the CLI but owned by a separate repo. I don't have push access to fallow-rs/fallow-skills, so I can't land the other half.
Options, whichever you prefer:
- A maintainer regenerates
cli-reference.mdinfallow-rs/fallow-skillsand merges it; this PR's drift gate then goes green with no further change here. - I open a PR against
fallow-rs/fallow-skillsfrom a fork with the regenerated file, to be merged before this one. - If the rename isn't wanted, I can drop it and keep
--annotations-path-prefixas the only name — though the flag now governscodeclimate,review-github, andreview-gitlabtoo, so the name and its "Valid only with the GitHub-native formats" help text would both be wrong.
Everything else on this PR is green: the full suite (including the untouched snapshot suite, which is the load-bearing check that --root == toplevel output is byte-identical), clippy -D warnings, fmt --check, generate:contracts:check, and a --features schema-emit build.
1 participant
What
When
--rootpoints at a package inside a larger git repo, fallow's CI output was unusable in two ways that share one cause: findings were keyed and emitted relative to the analysis root, while CI platforms andgit diffaddress files from the repository root.Report paths.
codeclimate,review-github, andreview-gitlabemitted--root-relative paths. GitLab's Code Quality widget matcheslocation.pathagainst the MR diff, and the review-discussion API rejects aposition.new_pathnaming a file absent from the diff — so the widget silently showed nothing and every inline discussion POST was rejected.github-annotationsalready rebased onto the git toplevel viaPathRebase; the other CI formats now do too.review-*and the sticky summary derive their paths from CodeClimate issues, so one seam covers them.Diff filtering.
--diff-filecompared diff keys against finding paths already stripped to--root. Below the toplevel nothing matched: every source-anchored finding was dropped and the run reported a clean diff, exit 0, nothing on stderr. That is the dangerous one — it doesn't produce wrong output, it produces empty output that looks like "clean diff, no findings."A unified diff does not declare its own base, and both conventions occur in practice:
git diffwrites toplevel-relative paths,git diff --relativewrites root-relative ones. So the base is resolved from the diff's paths themselves — whichever candidate directory they actually name existing files under wins. Both conventions now work.Namespaces. The underlying fix is that a path lives in one of three namespaces and the code mixed them:
src/a.jspackages/pkg/src/a.jsorsrc/a.jsDiffIndexlookupspackages/pkg/src/a.jsPaths now stay analysis-root-relative through diff filtering and ownership resolution. The presentation prefix is applied at exactly two boundaries:
emit_codeclimate(wire format) andReviewEnvelopeRenderInput::path_prefix(review render).DiffIndexowns the translation (key_for_root_relative,root_relative_from_key,old_path_for_root_relative).Also fixed, all found while reviewing the above:
old_path. The rename map is keyed in the diff's namespace but the lookup used the rendered path, soold_pathsilently fell back tonew_path— telling GitLab a moved file had not moved.--diff-filedid not scope inline review comments. That filter gated on$FALLOW_DIFF_FILE, so the flag rendered every comment while appearing to have applied the diff.--report-path-prefixdecided which comments survived the diff filter. Keys came from the rendered path; they now come from the root-relative path plus the diff's base. The prefix is presentation only.--annotations-path-prefixnor its replacement was registered inglobal_value_options(), sofallow --annotations-path-prefix p checkread the prefix as the subcommand.Fail-open, never silently clean. A diff whose paths name no file under any candidate base is foreign; one that names real files under two candidates is ambiguous. Neither can be placed.
check/filtering.rssets the convention — an unfilterable path is RETAINED, never silently dropped — so both cases now discard the diff, warn, and report at full scope. Filtering to zero on a guess is the exact failure this PR removes.Why
Downstream, this is what makes fallow usable on a monorepo package at all: the Code Quality widget, the sticky summary, and inline review discussions all address files by repo-root-relative path. Consumers have been working around it with post-processing scripts that rebase
location.pathby hand.--annotations-path-prefixis renamed to--report-path-prefix(old name kept as avisible_alias) and now governs every CI-facing format. Pass an empty value to opt out.Breaking change
codeclimate,review-github, andreview-gitlabnow emit repository-root-relative paths when--rootis a subdirectory. Consumers that prepend the offset themselves should drop that step, or pass--report-path-prefix ''to restore the previous output. Single-package repositories, where--rootis the toplevel, are unaffected — the two candidate bases coincide and output is byte-identical, which the untouched snapshot suite pins.Deliberately out of scope
[slug]directory becomes%5Bslug%5D), and security-tracerelatedLocations.analysis.diffFile(crates/api/src/analysis_context.rs) still builds an unbasedDiffIndexand keys against--root. Same defect class; left out to keep this PR's blast radius on the CLI/CI surface.Happy to split either into a follow-up if you'd prefer them in this PR.
Test plan
Both defects were first reproduced against a freshly built
main(3.3.0) in a throwaway monorepo, then pinned:crates/cli/tests/monorepo_report_paths_tests.rs) over a real git repo whosepackages/pkgis the analysis root. They cover: repo-root-relative CodeClimate paths;--report-path-prefixoverride, empty opt-out, and deprecated alias;codeclimateandgithub-annotationsagreeing on path shape with no double prefix; toplevel-relative and--relativediffs both keeping source-anchored findings; renames resolvingold_pathacross all four namespace combinations; review paths prefixed exactly once on the combined and subcommand routes;--diff-filescoping review comments without the env var; ambiguous base failing open; foreign base warning; and--root == topleveloutput unchanged.crates/output/src/diff.rsfor the namespace round-trip (key_for_root_relative/root_relative_from_key/old_path_for_root_relative), empty offsets, and sibling directories sharing a name prefix (packages/pkg-extramust not matchpackages/pkg).cargo test --workspace --lib --bins --tests— 59 test binaries, 0 failures.snapshot_testsis untouched, which is the load-bearing check: if a snapshot had moved, the fix would have changedroot == toplevelbehavior and be wrong.cargo clippy --workspace --all-targets -- -D warningsandcargo fmt --all -- --checkclean.The change was additionally reviewed by an independent adversarial pass, which caught three defects that are fixed here: the rename
old_pathfallback, a double prefix on the subcommand review route, and the ambiguous base failing closed.Note: the commit is unsigned — no GPG key on the machine it was authored on. Happy to re-sign if that blocks merge.