Background
Follow-up from the co-review of #193 (raised by both the gemini and codex CLI reviewers).
PR #193 makes tsu hook collate skip the repo-wide coach codeowners check when a change can't affect path-based ownership. The relevance rule (isCodeownersRelevant) treats a change as ownership-relevant when a file is added/copied/renamed or an OWNERSHIP/CODEOWNERS file is modified.
Gap
Deleted files (D) are excluded everywhere by --diff-filter=ACMR, so deleting an OWNERSHIP/CODEOWNERS file never surfaces in detection and therefore does not trigger the codeowners check. A deletion changes the ownership mapping (orphaning the paths that file used to own), so coach codeowners generate would produce a different result — exactly the "out of sync" case the check exists to catch.
Note: this is not a regression — the pre-#193 detection also used ACMR and never surfaced deletions. It's a pre-existing limitation, deferred from #193 to keep that PR's scope tight.
Why it wasn't fixed in #193
A correct fix is cross-cutting:
- Add
D to the diff filter in the status-aware helpers (get-files-in-range-with-status.ts, get-changed-files-with-status.ts), and
- Exclude deleted paths in
collate.ts before the file list is handed to the dart format/analysis/dcm/graphql hooks — otherwise those tools receive paths that no longer exist on disk.
Suggested approach
Surface D only for the ownership-relevance decision (deleted OWNERSHIP/CODEOWNERS files → relevant), while keeping deletions out of the file list passed to the per-file dart/graphql hooks. Add a test for "deleted OWNERSHIP file → codeowners runs".
Background
Follow-up from the co-review of #193 (raised by both the gemini and codex CLI reviewers).
PR #193 makes
tsu hook collateskip the repo-widecoachcodeowners check when a change can't affect path-based ownership. The relevance rule (isCodeownersRelevant) treats a change as ownership-relevant when a file is added/copied/renamed or anOWNERSHIP/CODEOWNERSfile is modified.Gap
Deleted files (
D) are excluded everywhere by--diff-filter=ACMR, so deleting anOWNERSHIP/CODEOWNERSfile never surfaces in detection and therefore does not trigger the codeowners check. A deletion changes the ownership mapping (orphaning the paths that file used to own), socoach codeowners generatewould produce a different result — exactly the "out of sync" case the check exists to catch.Note: this is not a regression — the pre-#193 detection also used
ACMRand never surfaced deletions. It's a pre-existing limitation, deferred from #193 to keep that PR's scope tight.Why it wasn't fixed in #193
A correct fix is cross-cutting:
Dto the diff filter in the status-aware helpers (get-files-in-range-with-status.ts,get-changed-files-with-status.ts), andcollate.tsbefore the file list is handed to the dart format/analysis/dcm/graphql hooks — otherwise those tools receive paths that no longer exist on disk.Suggested approach
Surface
Donly for the ownership-relevance decision (deletedOWNERSHIP/CODEOWNERSfiles → relevant), while keeping deletions out of the file list passed to the per-file dart/graphql hooks. Add a test for "deleted OWNERSHIP file → codeowners runs".