Rank skills by installs and harden install telemetry#2581
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Codex review: found issues before merge. Reviewed June 15, 2026, 9:40 PM ET / 01:40 UTC. Summary Reproducibility: not applicable. this is a feature and compatibility PR rather than a bug report. The observable checks are the linked prod-backed screenshots and the live GitHub mergeability state. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Rebase onto current main, preserve the current telemetry compatibility behavior, keep this PR's dedupe/clear/prune implementation, run Convex runtime validation, then have maintainers explicitly accept the installs-based search-ranking change. Do we have a high-confidence way to reproduce the issue? Not applicable; this is a feature and compatibility PR rather than a bug report. The observable checks are the linked prod-backed screenshots and the live GitHub mergeability state. Is this the best way to solve the issue? Unclear until the branch is rebased; the implementation direction is plausible, but the current-main compatibility conflict and Convex runtime-validation gap must be settled before this is the best merge path. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e3e5705d8969. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
Pull request overview
Updates Clawhub’s popularity/ranking model to use all-time installs (with stars as the primary tie-breaker), and adds install-telemetry deduplication plus retention-based pruning to reduce duplicate install counting and abuse signal noise.
Changes:
- Switch recommended/search popularity ranking inputs from downloads → all-time installs (stars remain first tie-breaker).
- Add per-(user, skill, root, UTC day) install telemetry dedupe table + daily pruning cron.
- Update docs/specs and expand regression coverage for install-based ranking and cursor/index compatibility.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/install-telemetry-ranking-abuse.md | Updates the spec to reflect installs as a ranking input and documents dedupe behavior. |
| docs/http-api.md | Updates API docs for recommended sorting to use all-time installs. |
| docs/api.md | Updates API docs for recommended sorting to use all-time installs. |
| convex/telemetry.ts | Implements install telemetry dedupe + adds internal prune mutation and day-bucketing helper. |
| convex/telemetry.test.ts | Adds/updates tests for deduped install telemetry behavior. |
| convex/skills.ts | Switches recommended indexing and fallback logic from downloads-based to installs-based ranking. |
| convex/skills.publicListCursor.test.ts | Adds regression coverage for stale recommended cursors after index rename. |
| convex/skills.listPublicPageV4.test.ts | Updates index-field contract and highlighted sorting expectations for installs-based ranking. |
| convex/search.ts | Switches popularity boost and tie-breaking from downloads → installsAllTime (downloads retained for compatibility). |
| convex/search.test.ts | Updates and adds tests validating installs-based popularity scoring and ordering. |
| convex/schema.ts | Renames recommended rank indexes and adds installTelemetryDedupes table + indexes. |
| convex/crons.ts | Schedules daily pruning for install telemetry dedupe rows. |
| convex/crons.test.ts | Extends internal API mock for the new telemetry prune cron reference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const pruneInstallTelemetryDedupesInternal = internalMutation({ | ||
| args: {}, | ||
| handler: async (ctx) => { | ||
| const cutoffDayStart = getDayStart(Date.now() - INSTALL_TELEMETRY_DEDUPE_RETENTION_MS); | ||
| const stale = await ctx.db | ||
| .query("installTelemetryDedupes") | ||
| .withIndex("by_day", (q) => q.lt("dayStart", cutoffDayStart)) | ||
| .take(PRUNE_BATCH_SIZE); | ||
|
|
||
| for (const entry of stale) { | ||
| await ctx.db.delete(entry._id); | ||
| } | ||
|
|
||
| const hasMore = stale.length === PRUNE_BATCH_SIZE; | ||
| if (hasMore) { | ||
| await ctx.scheduler.runAfter(0, internal.telemetry.pruneInstallTelemetryDedupesInternal, {}); | ||
| } | ||
|
|
||
| return { deleted: stale.length, hasMore }; | ||
| }, | ||
| }); |
| telemetry: { | ||
| pruneInstallTelemetryDedupesInternal: Symbol("install-telemetry-dedupe-prune"), | ||
| }, |
a808e16 to
eaf0e3a
Compare
f960445 to
526f32e
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
f43b8ed to
24ec574
Compare
24ec574 to
7bc81fd
Compare
Summary
This PR keeps the skill install telemetry and search-ranking work that remains after #2586 and #2633.
What Changed
Current net changed files:
Branch-only churn with no net diff:
Those files are intentionally not described as current PR behavior.
Flow
sequenceDiagram actor CLI participant Telemetry as telemetry:reportCliInstallInternal participant Dedupe as installTelemetryDedupes participant Installs as userSkillInstalls participant Stats as skillStatEvents CLI->>Telemetry: install event with slug and version Telemetry->>Dedupe: lookup user + skill + UTC day alt already seen today Telemetry-->>CLI: return without new stat event else first report for day Telemetry->>Dedupe: insert dedupe row Telemetry->>Installs: upsert rootless install state Telemetry->>Stats: insert install_new or install_reactivate endflowchart TD A[Account deletion or ban cleanup starts] --> B[Use clearStartedAt cutoff] B --> C[Delete old aggregate install rows in bounded batches] C --> D[Delete old root rows in bounded batches] D --> E[Delete old root install rows in bounded batches] E --> F[Delete installTelemetryDedupes for that user] F --> G{More rows remain?} G -- yes --> H[Schedule next clear batch with same cutoff] H --> C G -- no --> I[Clear complete] J[Daily cron] --> K[Prune stale dedupe day buckets]Proof
python3 /Users/jmerhi/repos/AGENT.MDs/skills/pr-proof-pack/scripts/pr_net_diff.py --markdownconvex/telemetry.ts,convex/telemetry.test.tsconvex/telemetry.ts,convex/crons.ts, focused testsconvex/search.test.tsScreenshots
None. The current net diff has no UI files. For this backend/system PR, Mermaid diagrams, tests, and command proof are clearer than route screenshots.
How To Test
Focused local verification:
Useful read-only spot checks after deploy:
Expected signals:
Verification