feat(signals): slop signal — trivial/whitespace churn - #617
Conversation
Scaffold buildSlopAssessment and raise a deterministic churn finding when high-line-count diffs touch minimal substantive source code using the same line-split approach as local score input. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. |
|
gittensory · advisory review Reviewed 2 changed file(s) — two independent AI reviewers. Suggested action: ✅ Safe to merge — both reviewers found no blocking issues. Reviewer A · Suggestions
Worth double-checking
Reviewer B · No blocking issues spotted. |
Combine the merged missing-test-evidence slop signal with trivial whitespace churn in buildSlopAssessment and reconcile unit tests. Co-authored-by: Cursor <cursoragent@cursor.com>
837662c to
ecf21b4
Compare
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
ghost
left a comment
There was a problem hiding this comment.
Gittensory review · advisory — a maintainer merges
0 actionable · 4 nitpick(s) · 2 file(s) · two independent AI reviewers
Suggested action: ✅ Safe to merge — both reviewers found no blocking issues.
📋 Walkthrough
This PR introduces a new slop signal that flags high‑churn diffs with minimal substantive source changes, adds its weight and description to the rubric, implements the detection logic, and expands the test suite to cover the new behavior.
Changes
| File | Summary |
|---|---|
src/signals/slop.ts |
Added trivialWhitespaceChurn weight, updated rubric, introduced churn detection constants and functions, and integrated new signal into assessment calculation. |
test/unit/slop.test.ts |
Updated imports and rubric test, added multiple tests for the new churn signal, and added sanitization test for the new finding. |
🔍 Reviewer notes
Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR adds a new "trivial / whitespace‑only churn" signal to the slop assessment, updates weights and rubric, implements detection logic, and adds comprehensive tests. The implementation is clean, respects public‑safe text constraints, and all new behavior is covered by tests.
Suggestions
- Consider adding a unit test for the band determination logic at the boundary (e.g., when slopRisk equals 25) to guarantee the expected "elevated" band.
- Document the rationale for the MIN_CHURN_LINES (40) and MAX_SOURCE_LINE_SHARE (0.15) constants in a comment for future maintainers.
Worth double-checking
- If the band mapping thresholds change, the test expecting "elevated" may break; ensure alignment.
- Large changedFiles arrays could have minor performance impact, though unlikely in typical PRs.
Reviewer B · mistral-small-3.1-24b-instruct — recommends ✅ merge
This PR adds a new 'slop' signal to detect trivial or whitespace-only churn in pull requests. The implementation is clear, well-tested, and adheres to the project's conventions. The changes are safe and ready to merge.
Suggestions
- Consider adding a comment explaining the rationale behind the chosen thresholds (MIN_CHURN_LINES and MAX_SOURCE_LINE_SHARE).
Worth double-checking
- Verify that the new signal does not introduce any false positives or negatives in real-world scenarios.
🤖 Prompt for AI agents
Copy this to your coding agent to fix everything flagged:
Address the following in this PR:
1. src/signals/slop.ts (line 101): The function name 'nonNegative' does not follow the camelCase convention used elsewhere in the file. Suggested fix: function nonNegativeValue(value: number | undefined): number {
Nitpicks:
- Consider adding a unit test for the band determination logic at the boundary (e.g., when slopRisk equals 25) to guarantee the expected "elevated" band.
- Document the rationale for the MIN_CHURN_LINES (40) and MAX_SOURCE_LINE_SHARE (0.15) constants in a comment for future maintainers.
- Consider adding a comment explaining the rationale behind the chosen thresholds (MIN_CHURN_LINES and MAX_SOURCE_LINE_SHARE).
💰 Earn for open-source contributions like this — Gittensor lets contributors earn for the work they already do. · Advisory review by Gittensory.
|
@JSONbored |
Still dialing in the enhanced review bot, sorry if it's a bit noisy. |
ghost
left a comment
There was a problem hiding this comment.
Gittensory review · advisory — a maintainer merges
0 actionable · 3 nitpick(s) · 2 file(s) · two independent AI reviewers
Suggested action: ✅ Safe to merge — both reviewers found no blocking issues.
📋 Walkthrough
The PR extends the slop signal module by introducing a weight and rubric entry for trivial/whitespace-only churn, implements detection based on line counts and source code share, and adds comprehensive unit tests for the new behavior.
Changes
| File | Summary |
|---|---|
src/signals/slop.ts |
Added weight, rubric entry, constants, detection logic, and helper functions for trivial whitespace churn signal. |
test/unit/slop.test.ts |
Added tests for the new trivial whitespace churn signal and updated existing tests to reflect rubric changes. |
🔍 Reviewer notes
Reviewer A · gpt-oss-120b — recommends ✅ merge
This PR adds a new "trivial whitespace churn" signal to the slop assessment, including weight, rubric entry, detection logic, and corresponding tests. The implementation follows existing patterns and respects public safety constraints.
Suggestions
- Consider adding a test case for the edge condition where sourceShare equals MAX_SOURCE_LINE_SHARE to verify the boundary behavior.
- Run the full TypeScript linting suite to ensure no new unused imports or naming conflicts were introduced.
Worth double-checking
- The new helper functions increase the file size; ensure they do not impact performance for large diffs.
- Verify that the exported function is correctly re-exported if used elsewhere beyond the test suite.
Reviewer B · mistral-small-3.1-24b-instruct — recommends ✅ merge
This PR adds a new 'slop' signal to detect trivial or whitespace-only churn in pull requests. The implementation is clear, well-tested, and maintains the project's standards. The changes are safe and ready to merge.
Suggestions
- Consider adding a comment explaining the choice of
MIN_CHURN_LINESandMAX_SOURCE_LINE_SHAREvalues for better maintainability.
Worth double-checking
- Verify that the new signal does not introduce false positives for legitimate changes with high churn but minimal source changes.
🤖 Automated advisory review · a maintainer makes the final call.
JSONbored
left a comment
There was a problem hiding this comment.
@kiannidev current head looks merge-ready.
A few notes:
- The trivial/whitespace churn signal is deterministic and stays inside the existing slop rubric.
- The tests cover high-churn non-code diffs, substantive-source dominance, small diffs, and public-safe output.
- The stale changes-requested state does not map to an unresolved blocker on this head.
No code changes requested.
Summary
buildSlopAssessmentinsrc/signals/slop.tswithSLOP_WEIGHTS,SLOP_RUBRIC_MARKDOWN, and deterministic band mapping.buildLocalScoreInput, flagging high-churn diffs with minimal substantive source edits.isCodeFile/isTestFilefromsrc/signals/local-branch.tsfor shared file classification and add unit coverage intest/unit/slop.test.ts.Fixes #560
Scope
CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally; global coverage stays at or above 97% for lines, statements, functions, and branches (aim for 98%+ branch coverage locally so CI variance does not fail near the threshold)npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm run audit --audit-level=moderateIf any required check was skipped, explain why:
npm run validatewas run locally (coverstypecheck+test:coverage); branch coverage met the 97% threshold.Safety
Notes
src/signals/slop.ts; merge feat(signals): slop signal — missing test evidence #559 first or rebase this branch afterward to combine the shared shell with both signals.Made with Cursor