Skip to content

fix(signals): unify the test-evidence code-path predicate as isTestableCodePath - #9906

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-signals-testable-code-path-9696
Jul 29, 2026
Merged

fix(signals): unify the test-evidence code-path predicate as isTestableCodePath#9906
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-signals-testable-code-path-9696

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

The canonical "does this diff have code that needs tests" rule lives in buildMissingTestEvidenceFinding (packages/loopover-engine/src/signals/slop.ts): it exempts PADDING_CATEGORIES (generated/vendored/minified) because that output carries source-file extensions (e.g. protoc's .pb.go stubs) but nobody hand-writes tests for mechanically regenerated code. Two consumers claimed to mirror it and used bare isCodeFile instead:

  • False positiveimprovement.ts's hasCodeFileToEvaluate returned true for a codegen-only diff (isCodeFile("api/service.pb.go") is true), so buildMissingTestEvidenceFinding's null (no testable code) was interpreted as "test evidence present". A zero-test PR of only api/service.pb.go emitted added_test_evidence and banded minor instead of insufficient-signal.
  • False negativecontributor-open-pr-monitor.ts's missingTestsFromFiles flagged a vendor/**/*.go-only diff as missing_tests, telling the contributor to add tests for vendored code the gate-side deliberately exempts (its comment even claimed parity with slop.ts).

Fix

Extract the rule once as export function isTestableCodePath(path) in packages/loopover-engine/src/signals/slop.ts (isCodeFile(path) && !PADDING_CATEGORIES.has(classifyChangedFile(path))), have buildMissingTestEvidenceFinding call it, re-export it through the engine index and the src/signals/slop.ts shim, and adopt it in both consumers — removing the two hand-copied isCodeFile filters. No change to PADDING_CATEGORIES membership or any replacement/threshold. The src/signals/slop.ts shim stays a pure re-export, so scripts/check-engine-parity.ts still recognises it.

Tests

  • Engine node:test (packages/loopover-engine/test/slop.test.ts, new): isTestableCodePath is true for hand-authored source and false for generated/vendored/minified/non-code; buildMissingTestEvidenceFinding exempts a codegen-only diff and still fires on real uncovered code.
  • improvement.test.ts: a codegen-only diff no longer emits a false added_test_evidence (band insufficient-signal).
  • contributor-open-pr-monitor.test.ts: missingTestsFromFiles exempts a generated/vendored-only diff, still flags real uncovered code, and clears when a substantive test file is present.

Both root regression tests fail against the current bare-isCodeFile code. Engine-flag coverage of the changed engine lines verified 100% (source-mapped lcov).

Closes #9696

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 29, 2026 21:17
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-29 21:40:56 UTC

8 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR extracts a single `isTestableCodePath` predicate (isCodeFile && not padding-category) into slop.ts and rewires the two consumers (improvement.ts's hasCodeFileToEvaluate, contributor-open-pr-monitor.ts's missingTestsFromFiles) that previously used bare isCodeFile, fixing both a false-positive (codegen-only diffs read as test evidence present) and a false-negative (vendored-only diffs flagged missing_tests) described in the PR body. The change is well-scoped, re-exports are threaded correctly through both the engine barrel and the src shim, and each behavioral change is covered by a matching regression test in slop.test.ts, improvement.test.ts, and contributor-open-pr-monitor.test.ts. The core logic itself is an exact extraction of the pre-existing inline filter, so it introduces no new risk beyond what was already shipped.

Nits — 3 non-blocking
  • packages/loopover-engine/src/index.ts is now ~471 lines per the external size-smell note, though this PR only adds a single export line to it — not something to block on but worth a follow-up trim.
  • The external brief flags '9696' as a 'magic number' in slop.ts:118 — this is just an issue-reference comment, not an actual numeric literal in logic, so it's a false positive from that tool but worth double-checking nothing else nearby needs a named constant.
  • Consider whether isTestableCodePath should also be applied anywhere else in the codebase that still hand-copies a bare isCodeFile filter for test-evidence purposes, to fully close out signals: exempt generated/vendored/minified files from both test-evidence code-side checks #9696 rather than just the two consumers named here.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9696
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 248 registered-repo PR(s), 97 merged, 38 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 248 PR(s), 38 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Partially addressed
The PR correctly extracts isTestableCodePath as the single shared predicate, wires it into buildMissingTestEvidenceFinding, both consumers, and both shim re-exports, matching the required pattern and fixing the parity comment. However, the explicitly required regression test asserting both arms — a mixed generated-file-plus-hand-written-src/service.go diff returning the positive finding when test

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, Ruby, TypeScript, Svelte, Cuda, JavaScript, Markdown, MDX
  • Official Gittensor activity: 248 PR(s), 38 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

…leCodePath

buildMissingTestEvidenceFinding exempts generated/vendored/minified output (source
extensions, but nobody hand-writes tests for regenerated code) via
!PADDING_CATEGORIES.has(classifyChangedFile(path)). Two consumers claimed to mirror it
and used bare isCodeFile instead:

- improvement.ts's hasCodeFileToEvaluate returned true for a codegen-only diff, so
  buildMissingTestEvidenceFinding's null (no testable code) was read as "test evidence
  present" and a zero-test PR of only api/service.pb.go scored added_test_evidence and
  banded `minor` instead of `insufficient-signal`.
- contributor-open-pr-monitor.ts's missingTestsFromFiles flagged a vendored-only diff
  missing_tests, telling the contributor to test vendored code the gate-side exempts.

Extract the rule once as isTestableCodePath(path) in the engine slop module, re-export
it through the src shim, and adopt it in both consumers (removing the two hand-copied
isCodeFile filters). No change to PADDING_CATEGORIES or any replacement/threshold.

Closes JSONbored#9696
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 29, 2026
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.85%. Comparing base (b5385ca) to head (d02ec43).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9906      +/-   ##
==========================================
+ Coverage   91.76%   91.85%   +0.09%     
==========================================
  Files         921      921              
  Lines      113198   113209      +11     
  Branches    27253    27284      +31     
==========================================
+ Hits       103879   103991     +112     
+ Misses       8034     7929     -105     
- Partials     1285     1289       +4     
Flag Coverage Δ
backend 95.68% <100.00%> (-0.01%) ⬇️
engine 72.24% <100.00%> (+0.37%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-engine/src/index.ts 100.00% <100.00%> (ø)
packages/loopover-engine/src/signals/slop.ts 75.71% <100.00%> (+8.15%) ⬆️
src/signals/contributor-open-pr-monitor.ts 96.22% <100.00%> (ø)
src/signals/improvement.ts 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

signals: exempt generated/vendored/minified files from both test-evidence code-side checks

1 participant