fix(stats): fix skill stat field sync direction and reconcile logic#1704
Conversation
|
@momothemage is attempting to deploy a commit to the 0xBuns Team on Vercel. A member of the Team first needs to authorize it. |
|
@codex review |
Greptile SummaryThis PR fixes two maintenance-utility bugs in Confidence Score: 5/5Safe to merge — the logic is correct and well-tested against all relevant field-state scenarios. Both fixes are correct: No files require special attention. Reviews (1): Last reviewed commit: "Merge branch 'openclaw:main' into featur..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b91ca6342
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const currentStars = | ||
| typeof skill.statsStars === "number" ? skill.statsStars : skill.stats.stars; | ||
|
|
||
| if (currentStars !== actualStars || skill.stats.comments !== actualComments) { |
There was a problem hiding this comment.
This check only compares actualStars to the canonical read path (statsStars when present), so a skill where statsStars is correct but legacy stats.stars is stale will be treated as in-sync and skipped. Because the patch below is also what keeps the legacy and top-level fields aligned, that divergence can persist indefinitely and break backward-compatibility consumers that still read stats.stars. Include a direct legacy-field mismatch check (or equivalent) before deciding no patch is needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
reconcileSkillStarCounts is intentionally scoped to fixing
star-count drift against the source of truth (the stars table). Keeping
the two field sets in sync is the responsibility of backfillSkillStatFields,
which explicitly checks both topLevelInSync and nestedInSync and writes both
sets of fields in a single patch.
Running both jobs covers the full surface: reconcile corrects the canonical
value when it drifts from the actual record count; backfill then propagates
any remaining top-level → legacy divergence. Merging the two concerns into
reconcile would duplicate the sync logic and make the job harder to reason
about.
hxy91819
left a comment
hxy91819
left a comment
There was a problem hiding this comment.
Review: fix(stats): fix skill stat field sync direction and reconcile logic
The fix itself is correct and well-reasoned. I traced the blame and the root cause is clear — buildSkillStatPatch was reading from the wrong direction since its creation in 31e9a57 (Jan 19), despite applySkillStatDeltas in the same commit correctly preferring top-level fields. reconcileSkillStarCounts (191b5763, Feb 13) inherited the same wrong assumption. This PR correctly aligns both to the canonical read path established by toPublicSkill / applySkillStatDeltas.
However, I'm requesting changes on one point: these functions need unit tests.
The bugs we're fixing here are exactly the kind that tests would have caught immediately. Consider the scenarios in buildSkillStatPatch:
- Top-level fields present and in sync with nested →
null(no patch) - Top-level present, nested stale → patch both to top-level values
- Top-level absent (pre-migration doc) → read from nested, write both
- Nested out of sync with top-level → patch nested to match top-level
Today none of these are tested. The same lack of coverage that allowed the original bug to ship uncaught still exists after this fix. The next person who touches this logic will face the same risk.
I'd like to see at minimum:
- Unit tests for
buildSkillStatPatchcovering the 4 scenarios above - A unit test for
reconcileSkillStarCountsverifying it reads from the canonical path (top-level preferred) when deciding whether to patch
applySkillStatDeltas already has the right pattern — it reads top-level first, falls back to nested, and writes both sets. buildSkillStatPatch should mirror that pattern (and now does), but without tests the contract is implicit and fragile.
Happy to merge once tests are added.
hxy91819
left a comment
hxy91819
left a comment
There was a problem hiding this comment.
Follow-up: preventing this class of bug structurally
The fix is correct, but we should also close the door on the entire category of bugs — not just this instance. The root cause isn't "someone read from the wrong field", it's that the "which field is canonical" rule only lives in individual function implementations. The next person who touches stat logic faces the same risk.
Three layers, cheapest first:
1. @deprecated on nested stat fields (IDE-level guard)
Mark the four migrated fields in the stats type as @deprecated. Anyone writing skill.stats.downloads gets a strikethrough + warning in their editor at the moment they type it. Zero cognitive cost, no doc lookup needed.
// In the generated or hand-maintained SkillStats type:
export type SkillStats = {
/** @deprecated Use top-level `statsDownloads` instead. */
downloads: number;
/** @deprecated Use top-level `statsStars` instead. */
stars: number;
/** @deprecated Use top-level `statsInstallsCurrent` instead. */
installsCurrent: number;
/** @deprecated Use top-level `statsInstallsAllTime` instead. */
installsAllTime: number;
comments: number;
versions: number;
};2. Single read entry point (review-level guard)
The canonical-read ternary (typeof skill.statsX === "number" ? skill.statsX : skill.stats.x) is currently copy-pasted across toPublicSkill, applySkillStatDeltas, buildSkillStatPatch, and reconcileSkillStarCounts. Every copy is a chance to get the direction wrong. Extract into one function:
// convex/lib/skillStats.ts — add alongside existing applySkillStatDeltas
export function readCanonicalStat(
skill: Doc<"skills">,
field: "downloads" | "stars" | "installsCurrent" | "installsAllTime",
): number {
const topLevelKey = `stats${field[0].toUpperCase()}${field.slice(1)}` as
| "statsDownloads" | "statsStars" | "statsInstallsCurrent" | "statsInstallsAllTime";
return typeof skill[topLevelKey] === "number"
? skill[topLevelKey]!
: (skill.stats[field] ?? 0);
}Then replace all four call sites. Anyone bypassing this function and accessing skill.stats.stars directly stands out in code review.
3. AGENTS.md rule (documentation-level guard)
Add a section under the existing "Convex Query & Bandwidth Rules":
## Stat Field Migration Rules
The `skills` table has dual-location stat fields:
- **Top-level** (`statsDownloads`, `statsStars`, `statsInstallsCurrent`, `statsInstallsAllTime`) — source of truth, indexable
- **Nested** (`stats.downloads`, `stats.stars`, `stats.installsCurrent`, `stats.installsAllTime`) — **@deprecated**, kept for backward compat
Always use `readCanonicalStat()` to read and `applySkillStatDeltas()` to write.
Both sets of fields must be written in the same patch (see `applySkillStatDeltas` return shape).
Nested-only reads are only acceptable for `stats.comments` and `stats.versions` (no top-level field exists yet).Summary
| Layer | When it catches the bug | Cost |
|---|---|---|
@deprecated types |
At write-time, in the IDE | Trivial |
| Single read/write entry point | At review-time, naked field access is obvious | Small refactor |
| AGENTS.md rule | Post-hoc, provides grounds for enforcement | Documentation only |
These are follow-ups, not blockers for this PR. But I'd strongly recommend filing an issue to track them — the same lack of structural guard that produced this bug still exists after the fix.
|
Clarification on the review:
To merge this PR, I need the tests. Everything else can come after. |
momothemage
force-pushed
the
feature/stat_reconcile_fix
branch
from
4d09451 to
ab84209
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
hxy91819
left a comment
hxy91819
left a comment
There was a problem hiding this comment.
All blocking items addressed:
buildSkillStatPatch: 5 unit tests covering all 4 canonical scenarios + comments preservationreconcileSkillStarCountsHandler: 5 unit tests covering canonical read path, fallback, dual-write, comment drift, and soft-delete skipbuildSkillStatPatchnow dual-writes (top-level + nestedstatsspread)reconcileSkillStarCountsreads from canonical path (statsStarspreferred,stats.starsfallback)- CHANGELOG entry added
Tests pass locally (10/10). CI build green. Approved.
Follow-up items (non-blocking, tracked separately):
@deprecatedon nested stat fields- Single
readCanonicalStat()entry point to replace copy-pasted ternaries - AGENTS.md stat migration rules section
|
Merged via squash.
Thanks @momothemage! |
2 participants
Background
The
skillstable maintains two parallel sets of stat fields as part of anin-progress field migration:
stats.downloadsstatsDownloadsstats.starsstatsStarsstats.installsCurrentstatsInstallsCurrentstats.installsAllTimestatsInstallsAllTimeThe top-level fields are the source of truth — they are updated in real
time by
applySkillStatDeltason every event flush, and are preferred bytoPublicSkillandskillStats.tswhen reading. The legacy nested fieldsare kept for backward compatibility until a full migration is completed.
Two bugs in the maintenance utilities caused the two sets of fields to
silently diverge.
Problems Fixed
1.
buildSkillStatPatch— Wrong sync direction (data regression risk)Before: The patch always read from
stats.*(legacy nested fields) andused those values to overwrite the top-level fields. This meant running the
backfill job would silently roll back any incremental stat updates that
had already been written to the top-level fields by the event pipeline.
After: The patch now prefers the top-level fields (as the source of
truth), falling back to the legacy nested fields only for pre-migration
documents where top-level fields are
undefined. Both sets of fields arewritten together in the same patch to keep them in sync.
2.
reconcileSkillStarCounts— Inconsistent read path in reconcile checkBefore: The out-of-sync check compared
skill.stats.stars(legacyfield) against
actualStars. IfstatsStarsandstats.starshad alreadydiverged, the check could produce a false "in-sync" result and skip the fix
entirely — meaning the reconcile job would silently do nothing for the
records that needed it most.
After: The check now uses the same canonical read path as
toPublicSkill— prefer
statsStars, fall back tostats.stars— so the reconcilejudgment is consistent with what users actually see.
Notes
statsfields are preserved forbackward compatibility; no database migration is required.
toPublicSkillandapplySkillStatDeltas(ternary fallbacks) are intentionally left in place until a future
migration removes the legacy fields.
Files Changed
convex/statsMaintenance.ts