fix(signals): deduplicate collision clusters, label buckets - #446
Conversation
…ission surface projection
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. |
JSONbored
left a comment
There was a problem hiding this comment.
Thanks for picking this up. The direction matches #440, but I found two blockers before this should merge:
package-lock.jsonhas broad optional dependency metadata churn even though the PR does not change dependencies. Please drop that file from the PR unless there is a concrete package-manager reason tied to this fix.- The scoped-overlap regression test does not exercise the production path changed in
processors.ts, and the final assertion can pass even if the processor goes back toMath.max(...). Please either extract the scoped-overlap union helper and test that directly, or add a focused processor-level/public-surface test that fails ifscopedOverlapCountis computed fromMath.max(prSpecificCount, preflight.collisions.length)again.
The label dedupe and permission-constant direction look reasonable once those two issues are fixed.
Signed-off-by: dale053 <star05223@outlook.com>
…test Reverts unrelated package-lock.json optional-dependency metadata churn that crept in with no corresponding dependency change. Replaces the weak `toBeLessThanOrEqual(unionCount)` test with a concrete overlapping-cluster scenario: prSpecific=[A,B], preflight=[A] → union=2. The assertion now fails if the union logic is replaced by Math.max or a naive intersection. Also fixes the missing `});` closing brace that left the `keeps the public PR queue row coherent` test unnested in the broken diff. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds two cases to the queue coherence test to push branch coverage back above the 97% threshold: - score=8 (5–8 open PRs): the only queuePressureScore bucket not hit - sampledLikelyReviewable with cachedOpenPullRequests=0: exercises the "likely-reviewable count unavailable from cached PR metadata" evidence string that was unreachable from existing tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JSONbored
left a comment
There was a problem hiding this comment.
@dale053 the direction is still useful, and the unrelated lockfile churn is gone from the current diff.
A few notes:
- The label dedupe and permission-constant changes are reasonable.
- The production code now computes the scoped-overlap union instead of
Math.max(...). - The remaining substantive blocker is still regression coverage for that production path. The new test exercises rendered comment behavior and can pass without proving
maybePublishPrPublicSurfacefeeds the deduped scoped-overlap count through the live webhook processor path. - The PR title also still contains a literal ellipsis, so it does not meet the Conventional Commit title rule.
Required changes:
- Test the extracted scoped-overlap union helper directly, or add a focused processor/public-surface test that fails if the code returns to
Math.max(...). - Retitle the PR to
fix(signals): deduplicate collision clusters and permission buckets.
Validation expected:
- Rerun the focused backfill, settings-preview, and signals/public-surface tests plus standard validation.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
JSONbored
left a comment
There was a problem hiding this comment.
@dale053 the source diff is focused and the production-path overlap coverage is much better than the earlier version.
A few notes:
- I tried GitHub's update-branch rebase before reviewing because the PR is behind current main, and GitHub reported a rebase conflict.
- I am not going to resolve that conflict from the maintainer side.
- After a clean rebase, this is likely reviewable again without changing the product direction.
Required changes:
- Rebase onto current main and resolve the conflict locally.
- Preserve the backfill label dedupe regression, scoped-overlap union coverage, and settings-preview permission regression coverage.
- Re-run the focused signals/backfill/settings-preview tests and full validation.
Validation expected:
- git diff --check
- focused backfill/signals/settings-preview tests
- full validate
JSONbored
left a comment
There was a problem hiding this comment.
@dale053 this now looks good to land.
A few notes:
- The unrelated lockfile churn is gone.
- The scoped-overlap union is now in the production queue path rather than only comment rendering.
- Regression coverage now exercises the dedupe behavior that was previously missing.
Fix failing CI.
…ission surface projection
…test Reverts unrelated package-lock.json optional-dependency metadata churn that crept in with no corresponding dependency change. Replaces the weak `toBeLessThanOrEqual(unionCount)` test with a concrete overlapping-cluster scenario: prSpecific=[A,B], preflight=[A] → union=2. The assertion now fails if the union logic is replaced by Math.max or a naive intersection. Also fixes the missing `});` closing brace that left the `keeps the public PR queue row coherent` test unnested in the broken diff. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: dale053 <star05223@outlook.com>
|
Hi,@JSONbored! |
JSONbored
left a comment
There was a problem hiding this comment.
@dale053 this is good to land.
A few notes:
- The label dedupe now handles overlapping PR buckets before deriving dominant labels.
- The scoped-overlap count now uses the same union logic in the production queue path and rendered panel path.
- The settings preview now derives read permissions from the canonical install-permission constant.
- The regression tests cover the three drift cases called out in #440.
No code changes requested.
Summary
requiredInstallPermissionsinsettings-preview.tsto derive read-only base permissions fromREQUIRED_INSTALLATION_PERMISSIONSinstead of hardcoded strings, so the preview can't silently drift from the canonical constant (regression from issue [Bug]: Repo settings-preview reportspull_requests: readand never flags a missingpull_requestspermission, contradicting the app's ownREQUIRED_INSTALLATION_PERMISSIONS(which mandatespull_requests: write) #419/fix(settings): require and surface pull_requests:write in the install preview for PR comment/label output #420 pattern).refreshContributorActivity:allPullRequests,mergedPullRequests, andopenPullRequestsare overlapping views of the same PR set — deduplicate nodes by URL before extracting labels so a label on one PR isn't counted three times (regression from issue [Bug]: Contributor profiledominantLabelsdouble-counts labels by foldingrepoStatsdominant labels into the same-activity raw labels, inconsistent with every sibling aggregation — biasinglabelFitscoring #417/fix(signals): dedupe overlapping label sources in contributor dominantLabels #418 pattern).scopedOverlapCountin the gate check path ofprocessors.ts: replaceMath.max(prSpecificCount, preflight.collisions.length)with the same Map-deduped union thatbuildPublicPrIntelligenceCommentuses, so the readiness score matches the rendered panel details (regression from issue [Bug]: Public PR panel "Duplicate risk" count usesMath.maxof two cluster sources instead of their union, so the posted comment's count disagrees with the related-work items it lists #424/fix(signals): count scoped related-work as the union of PR-specific and preflight clusters #425 pattern).backfill.test.ts,settings-preview.test.ts, andsignals-coverage.test.ts.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 audit --audit-level=moderateIf any required check was skipped, explain why:
Safety
Notes
Math.max→ union change inprocessors.tsis a pure correctness fix; the readiness score total is unaffected when the two sets are disjoint (the common case), and is now correct when they overlap.read-valued entries fromREQUIRED_INSTALLATION_PERMISSIONSas the base, keepingissues: writeconditional on whether output is actually enabled — preserving the existing behavior while ensuring future additions to the constant are automatically reflected.