Skip to content

fix(review): classifyChangedFile's isTestFile || isTestPath check is a permanently unreachable branch #8353

Description

@JSONbored

Context

src/review/changed-files-classify.ts line 28 reads:

if (isTestFile(path) || isTestPath(path)) return "test";

isTestFile is imported from ../signals/local-branch, which itself re-exports it (local-branch.ts:1265, export { isCodeFile, isTestFile };) from ../signals/path-matchers — a thin re-export shim over packages/loopover-engine/src/signals/path-matchers.ts. In that engine file, isTestFile is defined as:

export function isTestFile(file: string): boolean {
  return isTestPath(file);
}

— i.e. isTestFile literally calls isTestPath from packages/loopover-engine/src/signals/test-evidence.ts, the same isTestPath imported in changed-files-classify.ts from ../signals/test-evidence (itself a pure re-export of the same engine module, per that file's own header comment).

So for every possible input, isTestFile(path) === isTestPath(path) — the ||'s right-hand operand can never independently flip the result when the left is false. One truth-table combination of this branch (isTestFile(path) false AND isTestPath(path) true) is structurally unreachable by any input. Under this repo's 99%-branch-counted Codecov gate (codecov.yml, target: 99%), this is a real hazard: a future contributor touching this exact line for an unrelated reason would find one branch permanently uncoverable through no fault of their own.

Requirements

  • Remove the redundant isTestPath(path) disjunct from line 28, leaving if (isTestFile(path)) return "test"; — OR remove the isTestFile import/usage in favor of calling isTestPath directly. Either direction is acceptable; pick one and remove the now-unused import from the top of the file accordingly (do not leave both imported if only one is used).
  • Do not change the file's documented five-way precedence (generated > test > docs > config > source, see the function's own JSDoc) or any other classification behavior — this is a pure redundant-code removal, not a classification change.
  • Run npm run typecheck to confirm removing the now-unused import leaves no dangling reference.

Deliverables

  • src/review/changed-files-classify.ts line 28 simplified to check only one of isTestFile/isTestPath, with the now-unused import removed from the top of the file.
  • Confirm test/unit/changed-files-classify.test.ts's existing "test" classification cases still pass unchanged.

Test Coverage Requirements

99%+ Codecov patch coverage, branch-counted, on the changed line, per codecov.yml. This fix itself removes a structurally uncoverable branch, so a correct fix trivially satisfies the gate here.

Expected Outcome

classifyChangedFile's "test" branch check is single-sourced — no redundant, provably-always-equal disjunct that can never independently fire — removing a structurally-uncoverable branch from a file otherwise held to a 99% branch-coverage bar.

Links & Resources

  • src/review/changed-files-classify.ts
  • src/signals/local-branch.ts (line 1265, isTestFile re-export)
  • src/signals/test-evidence.ts (isTestPath re-export shim)
  • packages/loopover-engine/src/signals/path-matchers.ts (isTestFile's real implementation — delegates to isTestPath)
  • test/unit/changed-files-classify.test.ts

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions