Grafana-sourced shame history: uptime streaks + auto node-version#867
Grafana-sourced shame history: uptime streaks + auto node-version#867rasca wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughAdds configurable node-version grace timing, Grafana-driven version syncing and daily observability rollups, clean-streak computation, and read-only node-version handling across backend and frontend profile paths. ChangesNode version grace period, observability history, and streaks
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/validators/views.py (1)
906-938: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce redundant scanning in the per-operator network-streak rollup.
For each group, the code re-scans the entire
operator_network_wallet_idsdict (built across all operators) just to filter byop_key. This is O(groups × total operator-network pairs) instead of O(total pairs). It also contributes to the "too many branches" complexity flagged for_build_validator_groups. Pre-group the wallet ids by operator key once, outside thefor group in groups.values()loop.♻️ Proposed fix
+ wallet_ids_by_operator = {} + for (op_key, net), wallet_ids in operator_network_wallet_ids.items(): + wallet_ids_by_operator.setdefault(op_key, {})[net] = wallet_ids + for group in groups.values(): ... - network_streaks = {} - for (op_key, net), wallet_ids in operator_network_wallet_ids.items(): - if op_key != group['id']: - continue - network_streaks[net] = streaks_lib.clean_streak( - wallet_ids, now, snapshot_index - ) + network_streaks = { + net: streaks_lib.clean_streak(wallet_ids, now, snapshot_index) + for net, wallet_ids in wallet_ids_by_operator.get(group['id'], {}).items() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/views.py` around lines 906 - 938, The per-operator network-streak rollup in _build_validator_groups is repeatedly scanning the full operator_network_wallet_ids mapping for every group, which is unnecessary and adds complexity. Pre-group operator_network_wallet_ids by op_key once before the for group in groups.values() loop, then have the network_streaks build use the pre-indexed wallet IDs for the current group’s id instead of filtering the whole dict each time. Keep the existing clean_streak logic, group['network_streaks'] shape, and network_order-based sorting unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/validators/grafana_service.py`:
- Around line 462-472: The dedup logic in `_award_node_upgrade` is vulnerable to
a TOCTOU race because `already_awarded` and `pending` are checked with
`.exists()` before the new contribution is persisted. Move the dedup decision
and insert into a single atomic flow in `_award_node_upgrade` (for example by
using a transaction plus a uniqueness guard or other write-time constraint) so
overlapping sync runs cannot both pass the check and award the same `dedup_key`
twice.
- Around line 392-443: The outer try/except in the node version sync path is too
broad and lets one operator failure stop the rest of the network processing.
Keep the target auto-create logic in the existing `sync` flow, but move the
per-operator update and award work inside its own isolated try/except within the
`for operator, versions in by_operator.items()` loop so one bad `parse_version`
or `_award_node_upgrade` call does not block other operators. Preserve the
current warning logging in `grafana_service.py` by logging the specific operator
and continuing the loop after a failure.
- Around line 308-375: The `_record_history` read-then-write rollup in
`ValidatorWalletStatusSnapshot` is vulnerable to lost updates when concurrent
syncs overlap. Fix it by removing the Python-side merge based on the stale
`existing` snapshot and moving the sample/status accumulation to an atomic
database update path inside `_record_history` (or equivalent per-wallet
transaction/locking), so `metrics_samples`, `logs_samples`, and the latched
status fields are merged from the current row state rather than overwritten by a
concurrent run. Ensure the `ValidatorWalletObservation` insert remains
best-effort, but the snapshot upsert must be concurrency-safe.
- Around line 417-433: The `node_version_<network>` update in
`grafana_service.py` can regress because `by_operator` only considers wallets
present in `normalized`, so a missing wallet can make `highest` lower than the
value already stored. In the wallet-processing loop that updates
`Validator.objects.filter(pk=operator.pk).update(...)`, compare the newly
computed `highest` against the current operator field and only write when the
new version is newer, not merely different. Use the existing `parse_version`
logic and the `field`/`operator` update path to keep the stored version
monotonic.
In `@backend/validators/management/commands/rebuild_daily_snapshots.py`:
- Around line 31-36: The `handle` method in `rebuild_daily_snapshots` treats
`days` as a truthy check, so `--days 0` skips the cutoff filter and behaves like
no filter. Update the conditional in `handle` to explicitly check whether
`options.get('days')` is not None, so `0` still applies the `observed_at__gte`
filter while only a missing value bypasses it.
- Around line 90-93: The summary message in rebuild_daily_snapshots currently
calls observations.count() after the queryset has already been streamed via
iterator(), causing an extra full scan; update the command’s loop to track the
number of processed observations in a local counter while iterating, then use
that counter in the final self.stdout.write message. Keep the fix localized to
the rebuild_daily_snapshots command and the observations/rollups processing
flow.
- Around line 14-19: The command is importing internal helpers from
grafana_service.py, which should be moved to a shared module instead of relying
on private implementation details. Extract _latch, _latch_version,
_METRICS_SEVERITY, and _VERSION_SEVERITY into a common shared location, update
rebuild_daily_snapshots.py to import them from there, and keep
grafana_service.py as a consumer of the shared helpers.
In `@backend/validators/models.py`:
- Around line 166-198: Add a retention/archival strategy for
ValidatorWalletObservation so the append-only table does not grow without bound.
Update the observation lifecycle in/around ValidatorWalletObservation and the
snapshot rebuild flow (especially rebuild_daily_snapshots) to either prune or
archive rows older than the chosen retention window, and make sure any new
cleanup job is scheduled consistently with the Grafana sync cadence.
- Around line 141-155: Add help_text to the new status/version fields on
ValidatorWalletStatusSnapshot and ValidatorWalletObservation so they match the
model’s existing documentation style. Update the definitions for metrics_status,
logs_status, version_status, and node_version to include concise
intent-explaining help_text values, keeping the wording consistent with nearby
fields like show_in_overview and assets_under_management_usd. Use the field
names in these two model classes as the places to fix.
---
Outside diff comments:
In `@backend/validators/views.py`:
- Around line 906-938: The per-operator network-streak rollup in
_build_validator_groups is repeatedly scanning the full
operator_network_wallet_ids mapping for every group, which is unnecessary and
adds complexity. Pre-group operator_network_wallet_ids by op_key once before the
for group in groups.values() loop, then have the network_streaks build use the
pre-indexed wallet IDs for the current group’s id instead of filtering the whole
dict each time. Keep the existing clean_streak logic, group['network_streaks']
shape, and network_order-based sorting unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3aa7893-aea9-4672-afdb-2e5de44b9703
📒 Files selected for processing (18)
CHANGELOG.mdbackend/CLAUDE.mdbackend/contributions/node_upgrade/admin.pybackend/contributions/node_upgrade/migrations/0004_targetnodeversion_grace_days.pybackend/contributions/node_upgrade/models.pybackend/tally/settings.pybackend/validators/grafana_service.pybackend/validators/management/commands/rebuild_daily_snapshots.pybackend/validators/migrations/0015_validatorwalletstatussnapshot_logs_samples_and_more.pybackend/validators/models.pybackend/validators/serializers.pybackend/validators/streaks.pybackend/validators/tests/test_grafana_service.pybackend/validators/tests/test_node_version_sync.pybackend/validators/tests/test_streaks.pybackend/validators/tests/test_version_status.pybackend/validators/version_status.pybackend/validators/views.py
| @classmethod | ||
| def _record_history(cls, samples, now): | ||
| """ | ||
| Persist the raw observations for this sync run and latch them into today's | ||
| per-day rollup (worst-of-day). Never raises: history is best-effort and must | ||
| not break the live status sync. | ||
| """ | ||
| if not samples: | ||
| return | ||
| try: | ||
| today = timezone.localdate(now) | ||
|
|
||
| ValidatorWalletObservation.objects.bulk_create([ | ||
| ValidatorWalletObservation( | ||
| wallet=s['wallet'], | ||
| observed_at=now, | ||
| onchain_status=s['onchain_status'], | ||
| metrics_status=s['metrics_status'], | ||
| logs_status=s['logs_status'], | ||
| version_status=s['version_status'], | ||
| node_version=s['node_version'], | ||
| ) | ||
| for s in samples | ||
| ]) | ||
|
|
||
| wallet_ids = [s['wallet'].id for s in samples] | ||
| existing = { | ||
| snap.wallet_id: snap | ||
| for snap in ValidatorWalletStatusSnapshot.objects.filter( | ||
| wallet_id__in=wallet_ids, date=today | ||
| ) | ||
| } | ||
|
|
||
| rollups = [] | ||
| for s in samples: | ||
| wallet = s['wallet'] | ||
| prev = existing.get(wallet.id) | ||
| prev_metrics = prev.metrics_status if prev else 'unknown' | ||
| prev_logs = prev.logs_status if prev else 'unknown' | ||
| prev_version = prev.version_status if prev else 'unknown' | ||
| prev_m_samples = prev.metrics_samples if prev else 0 | ||
| prev_l_samples = prev.logs_samples if prev else 0 | ||
|
|
||
| rollups.append(ValidatorWalletStatusSnapshot( | ||
| wallet=wallet, | ||
| date=today, | ||
| status=wallet.status, | ||
| metrics_status=_latch(prev_metrics, s['metrics_status'], _METRICS_SEVERITY), | ||
| logs_status=_latch(prev_logs, s['logs_status'], _METRICS_SEVERITY), | ||
| version_status=_latch(prev_version, s['version_status'], _VERSION_SEVERITY), | ||
| node_version=s['node_version'] or (prev.node_version if prev else ''), | ||
| metrics_samples=prev_m_samples + (1 if s['metrics_ok'] else 0), | ||
| logs_samples=prev_l_samples + (1 if s['logs_ok'] else 0), | ||
| )) | ||
|
|
||
| # On insert, `status` is set from the wallet; on conflict only the | ||
| # observability columns update, so the on-chain sync's `status` is preserved. | ||
| ValidatorWalletStatusSnapshot.objects.bulk_create( | ||
| rollups, | ||
| update_conflicts=True, | ||
| unique_fields=['wallet', 'date'], | ||
| update_fields=[ | ||
| 'metrics_status', 'logs_status', 'version_status', | ||
| 'node_version', 'metrics_samples', 'logs_samples', | ||
| ], | ||
| ) | ||
| except Exception as exc: # pragma: no cover - defensive | ||
| logger.warning("Failed to record validator observation history: %s", exc) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Read-then-write rollup update is racy under concurrent sync runs.
_record_history reads existing snapshots (existing = {...}) and computes metrics_samples/logs_samples/latched status in Python from that stale snapshot, then upserts via bulk_create(update_conflicts=True). If two sync runs for the same network/day overlap (retry, manual trigger overlapping cron, etc.), both read the same prev state and the second upsert clobbers the first's contribution — losing a sample count / latch update rather than merging it. Given this directly feeds the new clean-streak/days-in-shame reporting, a lost update here silently corrupts the history the whole feature is built on.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 309-309: Missing return type annotation for classmethod _record_history
Add return type annotation: None
(ANN206)
[warning] 374-374: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/grafana_service.py` around lines 308 - 375, The
`_record_history` read-then-write rollup in `ValidatorWalletStatusSnapshot` is
vulnerable to lost updates when concurrent syncs overlap. Fix it by removing the
Python-side merge based on the stale `existing` snapshot and moving the
sample/status accumulation to an atomic database update path inside
`_record_history` (or equivalent per-wallet transaction/locking), so
`metrics_samples`, `logs_samples`, and the latched status fields are merged from
the current row state rather than overwritten by a concurrent run. Ensure the
`ValidatorWalletObservation` insert remains best-effort, but the snapshot upsert
must be concurrency-safe.
| wallets = list( | ||
| ValidatorWallet.objects | ||
| .filter(id__in=list(normalized.keys()), operator__isnull=False) | ||
| .select_related('operator', 'operator__user') | ||
| ) | ||
| by_operator = {} | ||
| for wallet in wallets: | ||
| by_operator.setdefault(wallet.operator, []).append(normalized[wallet.id]) | ||
|
|
||
| field = f'node_version_{network}' | ||
| for operator, versions in by_operator.items(): | ||
| highest = max(versions, key=parse_version) | ||
| # (2) Direct update bypasses NodeVersionMixin.save() so we control the | ||
| # award path (direct/approved) instead of its pending-submission path. | ||
| if getattr(operator, field, None) != highest: | ||
| Validator.objects.filter(pk=operator.pk).update(**{field: highest}) | ||
| setattr(operator, field, highest) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
node_version_<network> can regress when one of an operator's wallets doesn't report this cycle.
by_operator is built only from wallets present in normalized (i.e. wallets that reported a valid version in this sync run). If an operator has multiple wallets and one temporarily stops reporting metrics (network blip, restart, scrape gap), highest is computed from a smaller set and can be lower than the version already stored. Since the write is gated on != rather than "newer than," this silently downgrades the field.
This field is the fallback source compute_version_status reads from (via wallet.operator.node_version_<network>) when no explicit node_version is passed — which is exactly what the Wall of Shame view does. A transient reporting gap on one wallet can therefore falsely flip an already-upgraded operator back to "warning"/"shame" on the public page.
🐛 Proposed fix: never let the stored version regress
by_operator = {}
for wallet in wallets:
by_operator.setdefault(wallet.operator, []).append(normalized[wallet.id])
field = f'node_version_{network}'
for operator, versions in by_operator.items():
- highest = max(versions, key=parse_version)
+ current = getattr(operator, field, None) or ''
+ candidates = versions + ([current] if _SEMVER_RE.match(current) else [])
+ highest = max(candidates, key=parse_version)
# (2) Direct update bypasses NodeVersionMixin.save() so we control the
# award path (direct/approved) instead of its pending-submission path.
if getattr(operator, field, None) != highest:
Validator.objects.filter(pk=operator.pk).update(**{field: highest})
setattr(operator, field, highest)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wallets = list( | |
| ValidatorWallet.objects | |
| .filter(id__in=list(normalized.keys()), operator__isnull=False) | |
| .select_related('operator', 'operator__user') | |
| ) | |
| by_operator = {} | |
| for wallet in wallets: | |
| by_operator.setdefault(wallet.operator, []).append(normalized[wallet.id]) | |
| field = f'node_version_{network}' | |
| for operator, versions in by_operator.items(): | |
| highest = max(versions, key=parse_version) | |
| # (2) Direct update bypasses NodeVersionMixin.save() so we control the | |
| # award path (direct/approved) instead of its pending-submission path. | |
| if getattr(operator, field, None) != highest: | |
| Validator.objects.filter(pk=operator.pk).update(**{field: highest}) | |
| setattr(operator, field, highest) | |
| wallets = list( | |
| ValidatorWallet.objects | |
| .filter(id__in=list(normalized.keys()), operator__isnull=False) | |
| .select_related('operator', 'operator__user') | |
| ) | |
| by_operator = {} | |
| for wallet in wallets: | |
| by_operator.setdefault(wallet.operator, []).append(normalized[wallet.id]) | |
| field = f'node_version_{network}' | |
| for operator, versions in by_operator.items(): | |
| current = getattr(operator, field, None) or '' | |
| candidates = versions + ([current] if _SEMVER_RE.match(current) else []) | |
| highest = max(candidates, key=parse_version) | |
| # (2) Direct update bypasses NodeVersionMixin.save() so we control the | |
| # award path (direct/approved) instead of its pending-submission path. | |
| if getattr(operator, field, None) != highest: | |
| Validator.objects.filter(pk=operator.pk).update(**{field: highest}) | |
| setattr(operator, field, highest) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/grafana_service.py` around lines 417 - 433, The
`node_version_<network>` update in `grafana_service.py` can regress because
`by_operator` only considers wallets present in `normalized`, so a missing
wallet can make `highest` lower than the value already stored. In the
wallet-processing loop that updates
`Validator.objects.filter(pk=operator.pk).update(...)`, compare the newly
computed `highest` against the current operator field and only write when the
new version is newer, not merely different. Use the existing `parse_version`
logic and the `field`/`operator` update path to keep the stored version
monotonic.
| user = operator.user | ||
| dedup_key = f"version {target.version} [{network}]" | ||
| already_awarded = Contribution.objects.filter( | ||
| user=user, contribution_type=contribution_type, notes__contains=dedup_key, | ||
| ).exists() | ||
| pending = SubmittedContribution.objects.filter( | ||
| user=user, contribution_type=contribution_type, | ||
| state__in=['pending', 'accepted'], notes__contains=dedup_key, | ||
| ).exists() | ||
| if already_awarded or pending: | ||
| return |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Dedup check-then-create in _award_node_upgrade is a TOCTOU race.
already_awarded/pending are checked via .exists() and only later does contribution.save() persist the new row (line 498). Two overlapping sync runs (e.g. a slow request retried, or sync_all_networks triggered twice) that both reach an operator's target version in the same window could both pass the dedup check before either writes, resulting in a double-awarded contribution (double points on the leaderboard).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/grafana_service.py` around lines 462 - 472, The dedup
logic in `_award_node_upgrade` is vulnerable to a TOCTOU race because
`already_awarded` and `pending` are checked with `.exists()` before the new
contribution is persisted. Move the dedup decision and insert into a single
atomic flow in `_award_node_upgrade` (for example by using a transaction plus a
uniqueness guard or other write-time constraint) so overlapping sync runs cannot
both pass the check and award the same `dedup_key` twice.
| from validators.grafana_service import ( | ||
| _METRICS_SEVERITY, | ||
| _VERSION_SEVERITY, | ||
| _latch, | ||
| ) | ||
| from validators.models import ValidatorWalletObservation, ValidatorWalletStatusSnapshot |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "_LOGS_SEVERITY|_METRICS_SEVERITY|logs_status.*_latch|_latch\(" backend/validators/grafana_service.pyRepository: genlayer-foundation/points
Length of output: 452
🏁 Script executed:
#!/bin/bash
sed -n '1,220p' backend/validators/management/commands/rebuild_daily_snapshots.py
printf '\n--- grafana_service ---\n'
sed -n '1,420p' backend/validators/grafana_service.pyRepository: genlayer-foundation/points
Length of output: 20408
Extract shared latch helpers Move _latch, _latch_version, and the severity maps into a shared module so this command doesn’t depend on grafana_service.py internals.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/management/commands/rebuild_daily_snapshots.py` around
lines 14 - 19, The command is importing internal helpers from
grafana_service.py, which should be moved to a shared module instead of relying
on private implementation details. Extract _latch, _latch_version,
_METRICS_SEVERITY, and _VERSION_SEVERITY into a common shared location, update
rebuild_daily_snapshots.py to import them from there, and keep
grafana_service.py as a consumer of the shared helpers.
| def handle(self, *args, **options): | ||
| days = options.get('days') | ||
| observations = ValidatorWalletObservation.objects.all() | ||
| if days: | ||
| cutoff = timezone.now() - timedelta(days=days) | ||
| observations = observations.filter(observed_at__gte=cutoff) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--days 0 silently behaves like "no filter".
if days: treats 0 as falsy, so --days 0 skips the cutoff filter entirely instead of restricting to (effectively) no observations. Use an explicit is not None check.
🐛 Proposed fix
- if days:
+ if days is not None:
cutoff = timezone.now() - timedelta(days=days)
observations = observations.filter(observed_at__gte=cutoff)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def handle(self, *args, **options): | |
| days = options.get('days') | |
| observations = ValidatorWalletObservation.objects.all() | |
| if days: | |
| cutoff = timezone.now() - timedelta(days=days) | |
| observations = observations.filter(observed_at__gte=cutoff) | |
| def handle(self, *args, **options): | |
| days = options.get('days') | |
| observations = ValidatorWalletObservation.objects.all() | |
| if days is not None: | |
| cutoff = timezone.now() - timedelta(days=days) | |
| observations = observations.filter(observed_at__gte=cutoff) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 31-31: Missing type annotation for *args
(ANN002)
[warning] 31-31: Unused method argument: args
(ARG002)
[warning] 31-31: Missing type annotation for **options
(ANN003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/management/commands/rebuild_daily_snapshots.py` around
lines 31 - 36, The `handle` method in `rebuild_daily_snapshots` treats `days` as
a truthy check, so `--days 0` skips the cutoff filter and behaves like no
filter. Update the conditional in `handle` to explicitly check whether
`options.get('days')` is not None, so `0` still applies the `observed_at__gte`
filter while only a missing value bypasses it.
|
|
||
| self.stdout.write(self.style.SUCCESS( | ||
| f'Rebuilt {len(rollups)} daily rollup(s) from {observations.count()} observation(s).' | ||
| )) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid a second full scan for the summary count.
observations.count() re-runs the (potentially unfiltered, whole-table) query after .iterator() already streamed it once. Track a counter inside the loop instead of calling .count() at the end.
♻️ Proposed fix
acc = {}
+ obs_count = 0
for obs in observations.iterator():
+ obs_count += 1
key = (obs.wallet_id, timezone.localdate(obs.observed_at))
...
self.stdout.write(self.style.SUCCESS(
- f'Rebuilt {len(rollups)} daily rollup(s) from {observations.count()} observation(s).'
+ f'Rebuilt {len(rollups)} daily rollup(s) from {obs_count} observation(s).'
))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.stdout.write(self.style.SUCCESS( | |
| f'Rebuilt {len(rollups)} daily rollup(s) from {observations.count()} observation(s).' | |
| )) | |
| self.stdout.write(self.style.SUCCESS( | |
| f'Rebuilt {len(rollups)} daily rollup(s) from {obs_count} observation(s).' | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/management/commands/rebuild_daily_snapshots.py` around
lines 90 - 93, The summary message in rebuild_daily_snapshots currently calls
observations.count() after the queryset has already been streamed via
iterator(), causing an extra full scan; update the command’s loop to track the
number of processed observations in a local counter while iterating, then use
that counter in the final self.stdout.write message. Keep the fix localized to
the rebuild_daily_snapshots command and the observations/rollups processing
flow.
| # Latched worst-of-day observability verdict (from ValidatorWalletObservation). | ||
| metrics_status = models.CharField( | ||
| max_length=10, choices=ValidatorWallet.GRAFANA_STATUS_CHOICES, default='unknown' | ||
| ) | ||
| logs_status = models.CharField( | ||
| max_length=10, choices=ValidatorWallet.GRAFANA_STATUS_CHOICES, default='unknown' | ||
| ) | ||
| version_status = models.CharField( | ||
| max_length=10, choices=VERSION_STATUS_CHOICES, default='unknown' | ||
| ) | ||
| node_version = models.CharField(max_length=50, blank=True) | ||
| # Number of observations that day where the node was reporting metrics / logs. | ||
| metrics_samples = models.PositiveIntegerField(default=0) | ||
| logs_samples = models.PositiveIntegerField(default=0) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add help_text to the new status/version fields.
Other fields in this model (e.g. show_in_overview, assets_under_management_usd) consistently document intent via help_text. The new metrics_status/logs_status/version_status/node_version fields on both ValidatorWalletStatusSnapshot and ValidatorWalletObservation don't, which the repo's own ast-grep rule flags as a convention violation.
Also applies to: 166-198
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 141-143: use help_text to document model columns
Context: models.CharField(
max_length=10, choices=ValidatorWallet.GRAFANA_STATUS_CHOICES, default='unknown'
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 144-146: use help_text to document model columns
Context: models.CharField(
max_length=10, choices=ValidatorWallet.GRAFANA_STATUS_CHOICES, default='unknown'
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 147-149: use help_text to document model columns
Context: models.CharField(
max_length=10, choices=VERSION_STATUS_CHOICES, default='unknown'
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 150-150: use help_text to document model columns
Context: models.CharField(max_length=50, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/models.py` around lines 141 - 155, Add help_text to the
new status/version fields on ValidatorWalletStatusSnapshot and
ValidatorWalletObservation so they match the model’s existing documentation
style. Update the definitions for metrics_status, logs_status, version_status,
and node_version to include concise intent-explaining help_text values, keeping
the wording consistent with nearby fields like show_in_overview and
assets_under_management_usd. Use the field names in these two model classes as
the places to fix.
Source: Linters/SAST tools
| class ValidatorWalletObservation(BaseModel): | ||
| """ | ||
| Append-only log of a single Grafana-sync observation for a validator wallet. | ||
|
|
||
| One row is written per active wallet per Grafana sync run, capturing the | ||
| point-in-time observability verdict plus the on-chain status and the node | ||
| version reported to Prometheus. This is the raw source of truth from which the | ||
| daily ValidatorWalletStatusSnapshot rollup is materialised (and rebuildable). | ||
| """ | ||
| wallet = models.ForeignKey( | ||
| ValidatorWallet, | ||
| on_delete=models.CASCADE, | ||
| related_name='observations' | ||
| ) | ||
| observed_at = models.DateTimeField(db_index=True) | ||
| onchain_status = models.CharField(max_length=20, choices=ValidatorWallet.STATUS_CHOICES) | ||
| metrics_status = models.CharField(max_length=10, choices=ValidatorWallet.GRAFANA_STATUS_CHOICES) | ||
| logs_status = models.CharField(max_length=10, choices=ValidatorWallet.GRAFANA_STATUS_CHOICES) | ||
| version_status = models.CharField( | ||
| max_length=10, choices=ValidatorWalletStatusSnapshot.VERSION_STATUS_CHOICES, default='unknown' | ||
| ) | ||
| node_version = models.CharField(max_length=50, blank=True) | ||
|
|
||
| class Meta: | ||
| ordering = ['-observed_at'] | ||
| indexes = [ | ||
| models.Index(fields=['wallet', 'observed_at']), | ||
| ] | ||
|
|
||
| def __str__(self): | ||
| return f"{self.wallet.address[:10]}... @ {self.observed_at:%Y-%m-%d %H:%M} ({self.metrics_status}/{self.logs_status})" | ||
|
|
||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Consider a retention/archival plan for ValidatorWalletObservation.
This table gets one row per active wallet per Grafana sync run (every ~5 minutes per the cron description elsewhere). With no pruning job visible in this cohort, it will grow unbounded (e.g. ~100 wallets × 12/hr × 24 × 365 ≈ 10M+ rows/year), which will eventually slow down rebuild_daily_snapshots (full table scan) and inflate storage.
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 180-180: use help_text to document model columns
Context: models.CharField(max_length=20, choices=ValidatorWallet.STATUS_CHOICES)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 181-181: use help_text to document model columns
Context: models.CharField(max_length=10, choices=ValidatorWallet.GRAFANA_STATUS_CHOICES)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 182-182: use help_text to document model columns
Context: models.CharField(max_length=10, choices=ValidatorWallet.GRAFANA_STATUS_CHOICES)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 183-185: use help_text to document model columns
Context: models.CharField(
max_length=10, choices=ValidatorWalletStatusSnapshot.VERSION_STATUS_CHOICES, default='unknown'
)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
[info] 186-186: use help_text to document model columns
Context: models.CharField(max_length=50, blank=True)
Note: [CWE-710] Improper Adherence to Coding Standards.
(model-help-text)
🪛 Ruff (0.15.20)
[warning] 190-190: Mutable default value for class attribute
(RUF012)
[warning] 191-193: Mutable default value for class attribute
(RUF012)
[warning] 195-195: Missing return type annotation for special method __str__
Add return type annotation: str
(ANN204)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/models.py` around lines 166 - 198, Add a
retention/archival strategy for ValidatorWalletObservation so the append-only
table does not grow without bound. Update the observation lifecycle in/around
ValidatorWalletObservation and the snapshot rebuild flow (especially
rebuild_daily_snapshots) to either prune or archive rows older than the chosen
retention window, and make sure any new cleanup job is scheduled consistently
with the Grafana sync cadence.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
13-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the “upcoming uptime-streak” wording.
Line 11 already says clean-uptime streak reporting ships in this release, so calling it “upcoming” here makes the Unreleased notes contradictory. Reword this bullet to scope it to future days-in-shame reporting or to the daily-history foundation only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 13, Update the changelog entry in CHANGELOG.md to remove the contradictory “upcoming uptime-streak” wording; keep the bullet scoped to the daily-history foundation and/or future days-in-shame reporting only. Edit the existing unreleased note text so it no longer implies uptime-streak is forthcoming, while preserving the validator observability history summary.
♻️ Duplicate comments (5)
backend/validators/grafana_service.py (4)
432-443: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not let
node_version_<network>regress on partial observations.
by_operatoronly includes wallets that reported a valid version in this run. If an operator’s newest wallet is temporarily missing,highestis computed from an incomplete set and the!=guard writes an older version back to the operator row. Compare against the currently stored field and only update when the newly observed version parses newer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/grafana_service.py` around lines 432 - 443, The node_version_<network> update in grafana_service.py can regress because by_operator only contains wallets with a valid version in the current run, so the max() result may be older than the value already stored on the Validator. Update the loop around by_operator/highest and Validator.objects.filter(...).update(...) to compare the newly observed version against the operator’s existing field value, and only write when the new parsed version is newer rather than merely different.
472-481: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDeduplicate the automatic award at write time.
The
.exists()checks andcontribution.save()are separate operations, so overlapping syncs can both pass the dedup checks and create the same approved contribution twice. This needs a single atomic decision+insert path backed by a uniqueness guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/grafana_service.py` around lines 472 - 481, The dedup logic in the award path is not atomic, so concurrent runs can both pass the `.exists()` checks and create duplicate approved contributions. Update the write path around the `Contribution`/`SubmittedContribution` checks and the `contribution.save()` in `grafana_service.py` to use a single atomic decision-and-insert flow, backed by a uniqueness guard on the identifying fields such as `user`, `contribution_type`, and the `dedup_key`/notes marker.
402-453: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIsolate failures per operator during version sync.
Any exception inside the
for operator, versions in by_operator.items()loop is still swallowed by the outerexcept, which stops version updates and awards for every remaining operator on that network for this cycle. Catch/log inside the loop and continue so one bad operator does not block the whole network.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/grafana_service.py` around lines 402 - 453, The version sync logic in grafana_service.py currently wraps the entire normalization and operator-processing flow in one broad try/except, so a single failure in the by_operator loop can stop updates and awards for every remaining operator. Move the exception handling into the per-operator processing path inside the loop that iterates over by_operator.items(), using the same logger warning style there, and continue to the next operator so one bad record does not block the whole network. Keep the surrounding validation/target creation flow intact and ensure _award_node_upgrade and the direct Validator.objects.update path are still used per operator.
343-382: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the daily rollup update atomic.
existingis read once, then sample counts and latched statuses are recomputed in Python beforebulk_create(update_conflicts=True)writes them back. Two overlapping syncs for the same wallet/day can start from the same snapshot and the later write will clobber the earlier run’s increments/status transition, silently corrupting the history that streaks and shame reporting depend on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/grafana_service.py` around lines 343 - 382, The daily rollup in grafana_service.py is doing a read-modify-write cycle outside a transaction, so overlapping syncs can clobber `metrics_samples`, `logs_samples`, and latched statuses. Make the rollup path atomic around the `existing` snapshot lookup and `ValidatorWalletStatusSnapshot.objects.bulk_create(...)`, using the `samples` loop and `_latch`/`_latch_version` calculations under a transaction or equivalent locking so concurrent runs serialize correctly. Ensure the upsert logic in this daily snapshot aggregation preserves incremented counts and status transitions when multiple workers process the same wallet/date.backend/validators/management/commands/rebuild_daily_snapshots.py (1)
32-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
--days 0explicitly.
if days:still treats0as falsy, so--days 0rebuilds all history instead of applying the cutoff.Suggested fix
- if days: + if days is not None: cutoff = timezone.now() - timedelta(days=days) observations = observations.filter(observed_at__gte=cutoff)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/management/commands/rebuild_daily_snapshots.py` around lines 32 - 36, The `rebuild_daily_snapshots` command currently uses a truthy check on `days`, so `--days 0` is treated the same as omitting the option and rebuilds all history. Update the `days` handling in `rebuild_daily_snapshots` to explicitly check for `None` rather than relying on truthiness, and keep applying the `cutoff` filter on `observations` whenever `days` is provided, including zero.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/CLAUDE.md`:
- Around line 249-251: The backend reference for `validators/views.py` is
outdated: `/api/v1/validators/` should no longer be described as CRUD for all
authenticated users. Update the `validators` API entry to reflect that
authenticated users have read-only access while `create`, `update`,
`partial_update`, and `destroy` are staff-only and return 403 for non-staff.
Keep the `/api/v1/validators/me/` note unchanged unless its behavior also
differs, and make sure the wording matches the current `Validator` viewset
permissions and mutation contract.
In `@frontend/CLAUDE.md`:
- Line 441: The documentation reference uses the wrong route component name,
which will send maintainers to the wrong file. Update the mention currently
pointing to EditProfile.svelte so it matches the actual component filename,
ProfileEdit.svelte, in the affected CLAUDE.md sections. Use the route component
name consistently anywhere this edit-profile docs reference appears.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Line 13: Update the changelog entry in CHANGELOG.md to remove the
contradictory “upcoming uptime-streak” wording; keep the bullet scoped to the
daily-history foundation and/or future days-in-shame reporting only. Edit the
existing unreleased note text so it no longer implies uptime-streak is
forthcoming, while preserving the validator observability history summary.
---
Duplicate comments:
In `@backend/validators/grafana_service.py`:
- Around line 432-443: The node_version_<network> update in grafana_service.py
can regress because by_operator only contains wallets with a valid version in
the current run, so the max() result may be older than the value already stored
on the Validator. Update the loop around by_operator/highest and
Validator.objects.filter(...).update(...) to compare the newly observed version
against the operator’s existing field value, and only write when the new parsed
version is newer rather than merely different.
- Around line 472-481: The dedup logic in the award path is not atomic, so
concurrent runs can both pass the `.exists()` checks and create duplicate
approved contributions. Update the write path around the
`Contribution`/`SubmittedContribution` checks and the `contribution.save()` in
`grafana_service.py` to use a single atomic decision-and-insert flow, backed by
a uniqueness guard on the identifying fields such as `user`,
`contribution_type`, and the `dedup_key`/notes marker.
- Around line 402-453: The version sync logic in grafana_service.py currently
wraps the entire normalization and operator-processing flow in one broad
try/except, so a single failure in the by_operator loop can stop updates and
awards for every remaining operator. Move the exception handling into the
per-operator processing path inside the loop that iterates over
by_operator.items(), using the same logger warning style there, and continue to
the next operator so one bad record does not block the whole network. Keep the
surrounding validation/target creation flow intact and ensure
_award_node_upgrade and the direct Validator.objects.update path are still used
per operator.
- Around line 343-382: The daily rollup in grafana_service.py is doing a
read-modify-write cycle outside a transaction, so overlapping syncs can clobber
`metrics_samples`, `logs_samples`, and latched statuses. Make the rollup path
atomic around the `existing` snapshot lookup and
`ValidatorWalletStatusSnapshot.objects.bulk_create(...)`, using the `samples`
loop and `_latch`/`_latch_version` calculations under a transaction or
equivalent locking so concurrent runs serialize correctly. Ensure the upsert
logic in this daily snapshot aggregation preserves incremented counts and status
transitions when multiple workers process the same wallet/date.
In `@backend/validators/management/commands/rebuild_daily_snapshots.py`:
- Around line 32-36: The `rebuild_daily_snapshots` command currently uses a
truthy check on `days`, so `--days 0` is treated the same as omitting the option
and rebuilds all history. Update the `days` handling in
`rebuild_daily_snapshots` to explicitly check for `None` rather than relying on
truthiness, and keep applying the `cutoff` filter on `observations` whenever
`days` is provided, including zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 538e1e88-ef95-4019-8d34-3d60046f5337
📒 Files selected for processing (10)
CHANGELOG.mdbackend/CLAUDE.mdbackend/users/serializers.pybackend/validators/grafana_service.pybackend/validators/management/commands/rebuild_daily_snapshots.pybackend/validators/tests/test_api.pybackend/validators/tests/test_grafana_service.pybackend/validators/views.pyfrontend/CLAUDE.mdfrontend/src/routes/ProfileEdit.svelte
| - **`/profile`** - Edit profile page (authenticated users only) | ||
| - Component: `EditProfile.svelte` | ||
| - Only allows editing display name and node version | ||
| - Allows editing display name and profile fields; node version is shown read-only (Grafana-sourced, auto-detected from node metrics — not editable) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the actual route component filename.
These docs point readers to EditProfile.svelte, but the file in this PR is frontend/src/routes/ProfileEdit.svelte. Please rename the reference so future maintenance and code search land on the right component.
Also applies to: 607-607
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/CLAUDE.md` at line 441, The documentation reference uses the wrong
route component name, which will send maintainers to the wrong file. Update the
mention currently pointing to EditProfile.svelte so it matches the actual
component filename, ProfileEdit.svelte, in the affected CLAUDE.md sections. Use
the route component name consistently anywhere this edit-profile docs reference
appears.
rasca
force-pushed
the
feat/grafana-shame-history-streaks
branch
from
ec3d7a0 to
c11151b
Compare
The version-shame window on the Wall of Shame is no longer hardcoded to three days. It now reads a NODE_VERSION_SHAME_GRACE_DAYS setting (default three days, env-overridable), so the grace period can be tuned per environment without a code change. The version verdict logic also moves out of the wallet viewset into a shared version_status helper — with an explicit node_version override for callers that already know the running version — so the same rule can be reused by the Grafana-driven sync added later in this branch. ## Claude Implementation Notes - backend/validators/version_status.py: New compute_version_status(wallet, target, now, node_version=...) — extracted from the viewset; grace from settings.NODE_VERSION_SHAME_GRACE_DAYS via default_grace_days(); node_version param lets the Grafana sync pass the observed version (compares via NodeVersionMixin._compare_versions, no operator required) - backend/validators/views.py: _version_context now delegates to the helper; removed hardcoded VERSION_SHAME_GRACE_DAYS constant and unused timedelta import - backend/tally/settings.py: Add NODE_VERSION_SHAME_GRACE_DAYS (env-overridable, default 3, applied globally at evaluation time) - backend/validators/tests/test_version_status.py: Unit tests — parity + grace configurable via the setting - backend/CLAUDE.md, CHANGELOG.md: Document the setting and the shared helper
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/validators/views.py (1)
3-4: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid rescanning
operator_network_wallet_idsinside_build_validator_groups
network_streaksis rebuilt by iterating over every(op_key, net)entry for each group, which makes this quadratic in the number of groups/wallets. Precompute the per-operator/per-network streaks once and reuse them here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/views.py` around lines 3 - 4, The `_build_validator_groups` path is rescanning `operator_network_wallet_ids` repeatedly when rebuilding `network_streaks`, making the grouping logic unnecessarily quadratic. Refactor the code in the validator grouping flow to precompute the per-operator/per-network streak data once, then reuse that cached structure inside `_build_validator_groups` instead of iterating over every `(op_key, net)` pair for each group.
♻️ Duplicate comments (5)
backend/validators/models.py (2)
141-155: 📐 Maintainability & Code Quality | 🔵 TrivialMissing
help_texton new observability fields (still unresolved).
metrics_status,logs_status,version_status,node_version(and the same fields onValidatorWalletObservation, lines 180-187) still lackhelp_text, unlike other fields in this file (show_in_overview,assets_under_management_usd). This was already flagged in a prior review pass and remains unaddressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/models.py` around lines 141 - 155, Add help_text to the new observability fields in the model definitions: metrics_status, logs_status, version_status, and node_version in the daily summary model, and mirror the same update on the corresponding fields in ValidatorWalletObservation. Follow the existing style used by fields like show_in_overview and assets_under_management_usd, and make sure the new help_text clearly describes each field’s purpose without changing behavior.Source: Linters/SAST tools
166-198: 🚀 Performance & Scalability | 🔵 TrivialNo retention/archival plan for the append-only
ValidatorWalletObservationtable.One row is written per active wallet per ~5-minute Grafana sync, so this table grows unbounded and will eventually slow
rebuild_daily_snapshots(full scan) and inflate storage. This was already raised in a prior review pass and remains unaddressed here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/models.py` around lines 166 - 198, The append-only ValidatorWalletObservation table currently has no retention or archival strategy, so it will grow without bound and impact rebuild_daily_snapshots and storage. Add a cleanup/retention mechanism for ValidatorWalletObservation, such as a scheduled purge or archival job, and define a clear retention window for old rows. Use the ValidatorWalletObservation model and its observed_at field to identify records eligible for retention, and ensure the snapshot rebuild path still works with the retained data.backend/validators/grafana_service.py (3)
521-531: 🗄️ Data Integrity & Integration | 🟠 MajorDedup check-then-create in
_award_node_upgradeis still a TOCTOU race.
already_awarded/pendingare checked via.exists()and only later doescontribution.save()persist the row. Two overlapping sync runs reaching an operator's target version in the same window could both pass the dedup check before either writes, double-awarding the contribution (double leaderboard points). This was flagged previously and remains unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/grafana_service.py` around lines 521 - 531, The `_award_node_upgrade` dedup logic still has a check-then-create race between the `already_awarded`/`pending` `.exists()` queries and the later `contribution.save()`. Make the award creation atomic by moving the dedup guard into the write path in `_award_node_upgrade` using a transactional approach and a uniqueness-enforced lookup on the dedup key (for example via an idempotent create/get-or-create pattern tied to `dedup_key`), so overlapping sync runs cannot award the same upgrade twice.
472-486: 🎯 Functional Correctness | 🟠 Major
node_version_<network>can still regress when one of an operator's wallets doesn't report this cycle.
by_operatoris built only from wallets present innormalized(line 473-476), sohighest = max(versions, key=parse_version)(line 481) is computed only from wallets that reported this run. If an operator has multiple wallets and one has a transient reporting gap,highestcan be lower than the already-stored version, and since the write is gated on!=(not "newer than"), the field silently regresses — flipping an already-upgraded operator back to warning/shame on the public Wall of Shame viacompute_version_status's fallback read. This was flagged previously and is still present.🐛 Proposed fix: never let the stored version regress
field = f'node_version_{network}' for operator, versions in by_operator.items(): try: - highest = max(versions, key=parse_version) + current = getattr(operator, field, None) or '' + candidates = versions + ([current] if _SEMVER_RE.match(current) else []) + highest = max(candidates, key=parse_version) if getattr(operator, field, None) != highest: Validator.objects.filter(pk=operator.pk).update(**{field: highest}) setattr(operator, field, highest)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/grafana_service.py` around lines 472 - 486, The operator version update in the wallet aggregation path can still regress when some wallets are missing from the current run. In the logic around by_operator, highest, and the Validator.objects.filter(...).update step, compare the computed version against the already stored node_version_<network> and only write it when the new value is newer, not merely different. Keep the update in the same loop, but guard against downgrades so compute_version_status cannot fall back to an older stored version.
348-415: 🩺 Stability & Availability | 🟠 MajorRead-then-write rollup update is still racy under concurrent sync runs.
_record_historyreads existing snapshots intoexisting, computes latched status/sample counts in Python, then upserts viabulk_create(update_conflicts=True). Two overlapping sync runs for the same wallet/day (retry, overlapping cron/manual trigger) both read the sameprevstate and the second upsert clobbers the first's contribution, silently losing a sample count or un-latching a shame verdict. This directly corrupts the data the new clean-streak/days-in-shame reporting is built on. This issue was raised previously and remains unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/grafana_service.py` around lines 348 - 415, _make _record_history concurrency-safe by removing the read-then-write rollup pattern that can lose updates when overlapping sync runs touch the same wallet/day. In ValidatorWalletService._record_history, do not compute latched statuses and sample totals from an in-memory `existing` snapshot; instead update ValidatorWalletStatusSnapshot atomically in the database within a transaction, using DB-side increments/merge semantics for the daily snapshot keyed by `(wallet, date)`. Keep the same latching behavior for `_latch` and `_latch_version`, but ensure repeated upserts cannot clobber another run’s metrics_samples, logs_samples, or status transitions._
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/validators/streaks.py`:
- Around line 48-58: The _has_observation helper in streaks.py is missing
version rollup state, so version-only snapshots are treated as unobserved.
Update _has_observation(snap) to also consider snap.version_status when deciding
whether a snap counts as observed, alongside the existing metrics/logs sample
and status checks. Keep the change localized to _has_observation so version-only
snapshots like a shame state are not skipped and broken_by tracking can include
version.
In `@backend/validators/tests/test_streaks.py`:
- Around line 76-80: The current streak test only covers a shame version
alongside metrics/logs still marked on, so it misses the version-only case.
Extend test_version_shame_breaks_streak in test_streaks.py to add a regression
scenario where _snap() records version='shame' while metrics/logs are unknown
and sample counts are zero, then assert _streak() still reports a broken streak
with 'version' in broken_by.
In `@backend/validators/views.py`:
- Around line 895-916: The per-group network streak build in
_build_validator_groups is rescanning operator_network_wallet_ids for every
group, causing quadratic work on the hot path. Pre-group the wallet IDs by
operator key once before the group loop, then have the network_streaks
construction read only that operator’s precomputed entries instead of filtering
the full dict each time. Keep the existing streak calculation and sorting
behavior intact, but use the pre-grouped structure to simplify the branching in
_build_validator_groups.
---
Outside diff comments:
In `@backend/validators/views.py`:
- Around line 3-4: The `_build_validator_groups` path is rescanning
`operator_network_wallet_ids` repeatedly when rebuilding `network_streaks`,
making the grouping logic unnecessarily quadratic. Refactor the code in the
validator grouping flow to precompute the per-operator/per-network streak data
once, then reuse that cached structure inside `_build_validator_groups` instead
of iterating over every `(op_key, net)` pair for each group.
---
Duplicate comments:
In `@backend/validators/grafana_service.py`:
- Around line 521-531: The `_award_node_upgrade` dedup logic still has a
check-then-create race between the `already_awarded`/`pending` `.exists()`
queries and the later `contribution.save()`. Make the award creation atomic by
moving the dedup guard into the write path in `_award_node_upgrade` using a
transactional approach and a uniqueness-enforced lookup on the dedup key (for
example via an idempotent create/get-or-create pattern tied to `dedup_key`), so
overlapping sync runs cannot award the same upgrade twice.
- Around line 472-486: The operator version update in the wallet aggregation
path can still regress when some wallets are missing from the current run. In
the logic around by_operator, highest, and the
Validator.objects.filter(...).update step, compare the computed version against
the already stored node_version_<network> and only write it when the new value
is newer, not merely different. Keep the update in the same loop, but guard
against downgrades so compute_version_status cannot fall back to an older stored
version.
- Around line 348-415: _make _record_history concurrency-safe by removing the
read-then-write rollup pattern that can lose updates when overlapping sync runs
touch the same wallet/day. In ValidatorWalletService._record_history, do not
compute latched statuses and sample totals from an in-memory `existing`
snapshot; instead update ValidatorWalletStatusSnapshot atomically in the
database within a transaction, using DB-side increments/merge semantics for the
daily snapshot keyed by `(wallet, date)`. Keep the same latching behavior for
`_latch` and `_latch_version`, but ensure repeated upserts cannot clobber
another run’s metrics_samples, logs_samples, or status transitions._
In `@backend/validators/models.py`:
- Around line 141-155: Add help_text to the new observability fields in the
model definitions: metrics_status, logs_status, version_status, and node_version
in the daily summary model, and mirror the same update on the corresponding
fields in ValidatorWalletObservation. Follow the existing style used by fields
like show_in_overview and assets_under_management_usd, and make sure the new
help_text clearly describes each field’s purpose without changing behavior.
- Around line 166-198: The append-only ValidatorWalletObservation table
currently has no retention or archival strategy, so it will grow without bound
and impact rebuild_daily_snapshots and storage. Add a cleanup/retention
mechanism for ValidatorWalletObservation, such as a scheduled purge or archival
job, and define a clear retention window for old rows. Use the
ValidatorWalletObservation model and its observed_at field to identify records
eligible for retention, and ensure the snapshot rebuild path still works with
the retained data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e9d808e8-1302-4666-ab12-38d9b988923a
📒 Files selected for processing (21)
CHANGELOG.mdbackend/CLAUDE.mdbackend/tally/settings.pybackend/users/serializers.pybackend/validators/grafana_service.pybackend/validators/management/commands/rebuild_daily_snapshots.pybackend/validators/migrations/0015_validatorwalletstatussnapshot_logs_samples_and_more.pybackend/validators/models.pybackend/validators/node_version.pybackend/validators/serializers.pybackend/validators/streaks.pybackend/validators/tests/test_api.pybackend/validators/tests/test_grafana_service.pybackend/validators/tests/test_node_version_sync.pybackend/validators/tests/test_node_version_tracking.pybackend/validators/tests/test_streaks.pybackend/validators/tests/test_version_status.pybackend/validators/version_status.pybackend/validators/views.pyfrontend/CLAUDE.mdfrontend/src/routes/ProfileEdit.svelte
💤 Files with no reviewable changes (1)
- backend/validators/node_version.py
| def _has_observation(snap): | ||
| """Whether the Grafana sync recorded anything for this day (vs not-yet-synced).""" | ||
| return bool( | ||
| snap is not None | ||
| and ( | ||
| snap.metrics_samples > 0 | ||
| or snap.logs_samples > 0 | ||
| or snap.metrics_status != 'unknown' | ||
| or snap.logs_status != 'unknown' | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Count version rollups as observations.
_has_observation() ignores version_status, so a version-only snapshot such as version_status='shame' with unknown metrics/logs is treated as unobserved. That lets today’s version shame be skipped and suppresses broken_by=['version'].
🐛 Proposed fix
def _has_observation(snap):
"""Whether the Grafana sync recorded anything for this day (vs not-yet-synced)."""
return bool(
snap is not None
and (
snap.metrics_samples > 0
or snap.logs_samples > 0
or snap.metrics_status != 'unknown'
or snap.logs_status != 'unknown'
+ or snap.version_status != 'unknown'
)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _has_observation(snap): | |
| """Whether the Grafana sync recorded anything for this day (vs not-yet-synced).""" | |
| return bool( | |
| snap is not None | |
| and ( | |
| snap.metrics_samples > 0 | |
| or snap.logs_samples > 0 | |
| or snap.metrics_status != 'unknown' | |
| or snap.logs_status != 'unknown' | |
| ) | |
| ) | |
| def _has_observation(snap): | |
| """Whether the Grafana sync recorded anything for this day (vs not-yet-synced).""" | |
| return bool( | |
| snap is not None | |
| and ( | |
| snap.metrics_samples > 0 | |
| or snap.logs_samples > 0 | |
| or snap.metrics_status != 'unknown' | |
| or snap.logs_status != 'unknown' | |
| or snap.version_status != 'unknown' | |
| ) | |
| ) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 48-48: Missing return type annotation for private function _has_observation
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/streaks.py` around lines 48 - 58, The _has_observation
helper in streaks.py is missing version rollup state, so version-only snapshots
are treated as unobserved. Update _has_observation(snap) to also consider
snap.version_status when deciding whether a snap counts as observed, alongside
the existing metrics/logs sample and status checks. Keep the change localized to
_has_observation so version-only snapshots like a shame state are not skipped
and broken_by tracking can include version.
| def test_version_shame_breaks_streak(self): | ||
| _snap(self.wallet, self.today, version='shame') | ||
| result = self._streak() | ||
| self.assertEqual(result['days'], 0) | ||
| self.assertIn('version', result['broken_by']) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for version-only shame observations.
This test still has metrics/logs marked on, so it misses the case where Grafana records only version_status='shame'. Add a regression case with metrics/logs unknown and zero samples to lock the streak behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/tests/test_streaks.py` around lines 76 - 80, The current
streak test only covers a shame version alongside metrics/logs still marked on,
so it misses the version-only case. Extend test_version_shame_breaks_streak in
test_streaks.py to add a regression scenario where _snap() records
version='shame' while metrics/logs are unknown and sample counts are zero, then
assert _streak() still reports a broken streak with 'version' in broken_by.
Every Grafana status sync now captures a per-run observation for each active
validator wallet (on-chain status, metrics, logs, and the node version read
from the Prometheus `version` label) and latches it into a per-day rollup on
the existing daily snapshot. Metrics and logs latch pessimistically — shamed
at any point means the day is shamed — while version latches optimistically:
once a node has upgraded during a day, an earlier stale reading cannot shame
it. Per-day sample counters record whether the node was seen reporting at all,
the building blocks for uptime-streak and days-in-shame reporting.
Version labels are normalised at ingest ('v' prefix stripped, capped to the
column length, and when a node briefly reports two series right after an
upgrade the higher parseable one wins), so bad node-reported data can never
corrupt or abort a whole network's history. The rollup is fully rebuildable
from the raw observation log, whose rows are retained forever by explicit
decision. History writing is best-effort and isolated from the live status
update, so a failure there never corrupts the Wall of Shame status. No points
or public API behaviour changes in this step.
## Claude Implementation Notes
- backend/validators/models.py: New ValidatorWalletObservation (append-only raw log); extend ValidatorWalletStatusSnapshot with metrics_status/logs_status/version_status, node_version, metrics_samples/logs_samples
- backend/validators/grafana_service.py: PromQL adds the `version` label; parse_response returns a 4-tuple with version_by_address (normalised via _normalize_version, capped to _VERSION_MAX_LENGTH, higher parseable version wins on duplicate series via _safe_parse); sync_network computes per-wallet version_status via compute_version_status; _record_history writes observations + latched rollup (worst-of-day _latch for metrics/logs, best-of-day _latch_version for version)
- backend/validators/management/commands/rebuild_daily_snapshots.py: New command to re-materialise rollups from observations; --days N cutoff snapped to the local-day boundary so the oldest day is never rebuilt from partial observations
- backend/validators/migrations/0015_*: New model + snapshot columns
- backend/validators/tests/test_grafana_service.py: version-label parse (normalised), duplicate-series keeps-higher, overlong-label truncation, observation/rollup writes, both latch directions, no-observations-on-failure, rebuild day-boundary regression
- backend/CLAUDE.md, CHANGELOG.md: Document the observation log, rollup columns, latch directions, rebuild command, and retain-forever decision
The Wall of Shame now surfaces how long each validator has gone without being shamed. Every wallet reports a consecutive clean-day streak and the reasons the streak was last broken, and each operator gets a per-network streak using any-node-clean roll-up: a network-day counts as clean if at least one of the operator's nodes was healthy that day. Streaks are computed on read from the daily observability rollup, so they cost one extra snapshot query per request (the endpoint stays cached 60s) and start accumulating from deploy. ## Claude Implementation Notes - backend/validators/streaks.py: New module. clean_streak(wallet_ids, now, index) walks the daily rollup backward counting consecutive clean days (any-node-clean over the given wallet ids); clean day = active + >=1 metrics & logs sample + no shame dim. A partial today never breaks the streak; broken_by only attributes a reason for observed days (edge-of-history returns []). load_snapshot_index prefetches the window in one query. - backend/validators/views.py: wall_of_shame builds the snapshot index once and per-wallet streaks, passes them to the serializer via context and into _build_validator_groups; groups gain network_streaks (per-network any-node-clean) and each node entry gains clean_streak_days / clean_streak_broken_by - backend/validators/serializers.py: WallOfShameSerializer adds clean_streak_days + clean_streak_broken_by (from context, no N+1) - backend/validators/tests/test_streaks.py: streak counting, shame/gap/version breaks, unsynced-today, any-node-clean operator roll-up - backend/validators/tests/test_grafana_service.py: endpoint exposes streak fields + network_streaks - backend/CLAUDE.md, CHANGELOG.md: Document the streak fields
Grafana becomes the single source of truth for validator node versions. The
status sync reads each node's reported version and: promotes the fleet's
highest stable release to the active upgrade target the first time it is seen
(ignoring pre-release and build-tagged versions), keeps each operator's
recorded version in step with what their nodes actually run, and awards the
node-upgrade contribution directly — with the existing sooner-is-better
bonus — the moment a visible operator reaches the target, with no manual
submission or steward review.
Detection covers every reporting node regardless of on-chain status, so a
quarantined validator that upgrades still records it and earns the award.
Versions that packaging cannot parse are excluded from comparisons instead of
aborting the run, and one operator's failure never blocks version updates or
awards for the rest. Because versions are now observed rather than
self-reported, the portal stops accepting manual edits: the profile shows the
detected version read-only, the two backend write paths are closed, and the
old save()-driven pending-submission flow is removed. Dedup on the shared
notes key guarantees nothing is ever awarded twice.
## Claude Implementation Notes
- backend/validators/grafana_service.py: new _sync_node_versions({address_lower: version}) — matches wallets of ANY on-chain status, filters to semver-valid AND PEP 440-parseable versions, auto-creates TargetNodeVersion from the highest stable observed (never blindly supersedes an unparseable active target), writes node_version_<network> via Validator.objects.update() (max across the operator's nodes), per-operator try/except fault isolation; _award_node_upgrade creates a direct approved Contribution (early-bonus 4/3/2/1, dedup on `version {v} [{network}]`, multiplier fallback via _allow_missing_multiplier)
- backend/validators/node_version.py: remove NodeVersionMixin.save() and _create_upgrade_submission (dead once the portal can't write versions); keep fields, validation, comparison helpers, calculate_early_upgrade_bonus
- backend/users/serializers.py: UserProfileUpdateSerializer drops the writable node_version fields and the custom update()
- backend/validators/views.py: ValidatorViewSet.my_profile is GET-only (PATCH → 405)
- frontend/src/routes/ProfileEdit.svelte: node version inputs replaced with read-only display ("Not detected yet" fallback, auto-detected hint); removed the related state/change-tracking/save logic
- backend/validators/tests/test_node_version_sync.py: auto-target stable-only guard, supersede/no-op cases, max-across-nodes, single award + dedup, invisible-operator no-award, quarantined-wallet award, PEP 440-invalid isolation, one-failing-operator isolation
- backend/validators/tests/test_node_version_tracking.py, test_api.py: drop save()-driven submission tests; /validators/me PATCH asserts read-only
- backend/CLAUDE.md, frontend/CLAUDE.md, CHANGELOG.md: document Grafana as source of truth and the read-only portal surface
rasca
force-pushed
the
feat/grafana-shame-history-streaks
branch
from
3ae689a to
aa67bde
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/validators/streaks.py (1)
48-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill missing
version_statusin_has_observation— regressesbroken_byreporting for version-only shame days.A day where Grafana only observed a version violation (metrics/logs
unknown, zero samples,version_status='shame') is still classified as "not observed" here._is_cleancorrectly breaks the streak (version_status == 'shame'), but_shame_dims/dims_onwill return[]for that day since_has_observationreturnsFalse, silently dropping'version'frombroken_by. This is the same gap flagged on a previous commit of this file and remains unaddressed.🐛 Proposed fix
def _has_observation(snap): """Whether the Grafana sync recorded anything for this day (vs not-yet-synced).""" return bool( snap is not None and ( snap.metrics_samples > 0 or snap.logs_samples > 0 or snap.metrics_status != 'unknown' or snap.logs_status != 'unknown' + or snap.version_status != 'unknown' ) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/streaks.py` around lines 48 - 58, The `_has_observation` helper in `streaks.py` is still treating version-only shame days as “not observed,” which causes `broken_by` to miss the version dimension. Update `_has_observation(snap)` to also return true when `snap.version_status` is not `'unknown'`, alongside the existing metrics/logs sample and status checks, so `_shame_dims` and `dims_on` can correctly include `'version'` for those days.backend/validators/tests/test_streaks.py (1)
76-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a version-only regression case.
This test still keeps
metrics='on', logs='on', so it doesn't cover the version-only shame scenario (metrics/logsunknown, zero samples) that exposes the_has_observationbug flagged instreaks.py. This mirrors a prior trivial comment on the same test that remains unaddressed.🧪 Proposed additional test
def test_version_shame_breaks_streak(self): _snap(self.wallet, self.today, version='shame') result = self._streak() self.assertEqual(result['days'], 0) self.assertIn('version', result['broken_by']) + + def test_version_only_shame_breaks_streak_and_reports_dim(self): + # No metrics/logs reported at all this day, only a version verdict. + _snap(self.wallet, self.today, version='shame', + metrics='unknown', logs='unknown', m_samples=0, l_samples=0) + result = self._streak() + self.assertEqual(result['days'], 0) + self.assertIn('version', result['broken_by'])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/validators/tests/test_streaks.py` around lines 76 - 80, The current `test_version_shame_breaks_streak` still uses `metrics='on'` and `logs='on'`, so it does not exercise the version-only shame path that triggers the `_has_observation` issue in `streaks.py`. Add a separate regression test in `test_streaks.py` using `_snap` with `version='shame'` and no metrics/logs observations (so they remain unknown/zero samples), then assert `_streak()` returns `days == 0` and is broken by `version`. Keep the existing mixed-observation test if needed, but ensure this new case specifically targets the version-only scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/validators/grafana_service.py`:
- Around line 149-162: The version selection logic in the `version_by_address`
update block should prefer a later parseable version when the existing `prev`
value cannot be parsed. In the address/version merge path, adjust the comparison
in this loop so `_safe_parse(prev)` being `None` does not block replacement by a
valid `version`; use the existing helpers `_normalize_version` and `_safe_parse`
around `version_by_address` to keep the highest parseable version, and fall back
to parseable values over unparseable ones.
In `@backend/validators/version_status.py`:
- Around line 54-57: The version comparison in VersionStatus validation can
incorrectly classify malformed stored versions because
NodeVersionMixin._compare_versions() falls back to lexicographic ordering on
parse errors. Update the logic around matches_target in the validator to
normalize or reject invalid node_version values before calling the comparison,
so bad legacy strings are treated as unknown/shame instead of matching
target.version. Refer to the VersionStatus path and
NodeVersionMixin._compare_versions when making the fix.
---
Duplicate comments:
In `@backend/validators/streaks.py`:
- Around line 48-58: The `_has_observation` helper in `streaks.py` is still
treating version-only shame days as “not observed,” which causes `broken_by` to
miss the version dimension. Update `_has_observation(snap)` to also return true
when `snap.version_status` is not `'unknown'`, alongside the existing
metrics/logs sample and status checks, so `_shame_dims` and `dims_on` can
correctly include `'version'` for those days.
In `@backend/validators/tests/test_streaks.py`:
- Around line 76-80: The current `test_version_shame_breaks_streak` still uses
`metrics='on'` and `logs='on'`, so it does not exercise the version-only shame
path that triggers the `_has_observation` issue in `streaks.py`. Add a separate
regression test in `test_streaks.py` using `_snap` with `version='shame'` and no
metrics/logs observations (so they remain unknown/zero samples), then assert
`_streak()` returns `days == 0` and is broken by `version`. Keep the existing
mixed-observation test if needed, but ensure this new case specifically targets
the version-only scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bc4eff83-6cdc-4f28-aaa1-c8b5ca311ab0
📒 Files selected for processing (21)
CHANGELOG.mdbackend/CLAUDE.mdbackend/tally/settings.pybackend/users/serializers.pybackend/validators/grafana_service.pybackend/validators/management/commands/rebuild_daily_snapshots.pybackend/validators/migrations/0015_validatorwalletstatussnapshot_logs_samples_and_more.pybackend/validators/models.pybackend/validators/node_version.pybackend/validators/serializers.pybackend/validators/streaks.pybackend/validators/tests/test_api.pybackend/validators/tests/test_grafana_service.pybackend/validators/tests/test_node_version_sync.pybackend/validators/tests/test_node_version_tracking.pybackend/validators/tests/test_streaks.pybackend/validators/tests/test_version_status.pybackend/validators/version_status.pybackend/validators/views.pyfrontend/CLAUDE.mdfrontend/src/routes/ProfileEdit.svelte
💤 Files with no reviewable changes (1)
- backend/validators/node_version.py
| version = _normalize_version(labels.get('version'))[:_VERSION_MAX_LENGTH] | ||
| if version: | ||
| prev = version_by_address.get(addr) | ||
| if prev is None: | ||
| version_by_address[addr] = version | ||
| else: | ||
| prev_parsed = _safe_parse(prev) | ||
| cur_parsed = _safe_parse(version) | ||
| if ( | ||
| prev_parsed is not None | ||
| and cur_parsed is not None | ||
| and cur_parsed > prev_parsed | ||
| ): | ||
| version_by_address[addr] = version |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Version-overlap resolution never replaces an unparseable "first" frame with a later valid one.
The replace condition requires prev_parsed is not None, so if the first frame processed for an address has a PEP-440-unparseable label (e.g. 0.6.0-genlayer.1, a format this codebase explicitly expects — see test_pep440_invalid_semver_does_not_abort_other_operators), every subsequent frame — including a valid, higher, parseable version reported in the same batch — is silently ignored. version_by_address[addr] stays pinned to the garbage string, which then poisons ValidatorWalletObservation.node_version, the daily rollup, and compute_version_status for that node until the bad-format series disappears from Prometheus entirely.
🐛 Proposed fix: prefer a parseable version over an unparseable one
version = _normalize_version(labels.get('version'))[:_VERSION_MAX_LENGTH]
if version:
prev = version_by_address.get(addr)
if prev is None:
version_by_address[addr] = version
else:
prev_parsed = _safe_parse(prev)
cur_parsed = _safe_parse(version)
- if (
- prev_parsed is not None
- and cur_parsed is not None
- and cur_parsed > prev_parsed
- ):
- version_by_address[addr] = version
+ if prev_parsed is None:
+ if cur_parsed is not None:
+ version_by_address[addr] = version
+ elif cur_parsed is not None and cur_parsed > prev_parsed:
+ version_by_address[addr] = version📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| version = _normalize_version(labels.get('version'))[:_VERSION_MAX_LENGTH] | |
| if version: | |
| prev = version_by_address.get(addr) | |
| if prev is None: | |
| version_by_address[addr] = version | |
| else: | |
| prev_parsed = _safe_parse(prev) | |
| cur_parsed = _safe_parse(version) | |
| if ( | |
| prev_parsed is not None | |
| and cur_parsed is not None | |
| and cur_parsed > prev_parsed | |
| ): | |
| version_by_address[addr] = version | |
| version = _normalize_version(labels.get('version'))[:_VERSION_MAX_LENGTH] | |
| if version: | |
| prev = version_by_address.get(addr) | |
| if prev is None: | |
| version_by_address[addr] = version | |
| else: | |
| prev_parsed = _safe_parse(prev) | |
| cur_parsed = _safe_parse(version) | |
| if prev_parsed is None: | |
| if cur_parsed is not None: | |
| version_by_address[addr] = version | |
| elif cur_parsed is not None and cur_parsed > prev_parsed: | |
| version_by_address[addr] = version |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/grafana_service.py` around lines 149 - 162, The version
selection logic in the `version_by_address` update block should prefer a later
parseable version when the existing `prev` value cannot be parsed. In the
address/version merge path, adjust the comparison in this loop so
`_safe_parse(prev)` being `None` does not block replacement by a valid
`version`; use the existing helpers `_normalize_version` and `_safe_parse`
around `version_by_address` to keep the highest parseable version, and fall back
to parseable values over unparseable ones.
| matches_target = bool( | ||
| node_version | ||
| and NodeVersionMixin._compare_versions(node_version, target.version) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd node_version.py backend/validators
cat -n backend/validators/node_version.py | sed -n '1,140p'
ast-grep run --pattern '_compare_versions($_, $_)' --lang python backend/validatorsRepository: genlayer-foundation/points
Length of output: 5208
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the status calculation path and any tests covering malformed versions.
fd version_status.py backend/validators
cat -n backend/validators/version_status.py | sed -n '1,180p'
printf '\n--- tests mentioning compare_versions / version_status ---\n'
rg -n "_compare_versions|version_matches_or_higher|compute_version_status|node_version" backend -g '*test*' -g '*tests*'
printf '\n--- default path into compute_version_status ---\n'
rg -n "node_version_asimov|node_version_bradbury|version_status" backendRepository: genlayer-foundation/points
Length of output: 23184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n backend/validators/tests/test_node_version_tracking.py | sed -n '126,170p'
cat -n backend/validators/tests/test_version_status.py | sed -n '1,140p'Repository: genlayer-foundation/points
Length of output: 6222
Avoid lexicographic fallback for malformed versions. Legacy operator values still reach this path, and NodeVersionMixin._compare_versions() falls back to node_version >= target_version on parse errors, so invalid strings can be classified incorrectly instead of being treated as unknown/shame. Normalize or reject invalid stored versions before this check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/validators/version_status.py` around lines 54 - 57, The version
comparison in VersionStatus validation can incorrectly classify malformed stored
versions because NodeVersionMixin._compare_versions() falls back to
lexicographic ordering on parse errors. Update the logic around matches_target
in the validator to normalize or reject invalid node_version values before
calling the comparison, so bad legacy strings are treated as unknown/shame
instead of matching target.version. Refer to the VersionStatus path and
NodeVersionMixin._compare_versions when making the fix.
1 participant
What & why
Adds per-validator days-in-shame history and a consecutive "not shamed" uptime streak (per node and per network, Asimov + Bradbury), plus folds node-version tracking into the Grafana sync so upgrades are detected and rewarded automatically. Everything the portal needs that Grafana can provide (metrics/logs reporting, node version) now comes from Grafana; on-chain status/operator/stake/identity still come from the existing RPC sync (unchanged).
Built as 4 self-contained, independently-reviewable commits (safe → risky):
TargetNodeVersion.grace_days(editable, default from newNODE_VERSION_SHAME_GRACE_DAYS); version verdict extracted tovalidators/version_status.py. Pure refactor + one field.ValidatorWalletObservation; the daily snapshot becomes a latched worst-of-day rollup (metrics/logs/version + sample counters + node_version); Prometheus query now reads theversionlabel;rebuild_daily_snapshotscommand. No points/behaviour change.validators/streaks.py; wall-of-shame returnsclean_streak_days/clean_streak_broken_byper node, plus per-operator-per-networknetwork_streaksusing any-node-clean roll-up (a network-day is clean if ≥1 node was clean). Read-only over commit 2's data.TargetNodeVersionfrom the first stable higher release, write each operator's version, and directly award thenode-upgradecontribution (4/3/2/1 early bonus). Dedups against the manual flow so no double-award. Only commit that changes points.Decisions baked in
activeis the gate.since.Notes / caveats
*/5but GitHub throttles it to ~1–3h in practice; the "≥1 sample/day" rule is chosen to suit that cadence.genlayer_node_infometric reportsv-prefixed versions the profile field forbids — normalized (stripv) before storing.node_upgrade 0004(grace_days),validators 0015(observation model + rollup columns).Testing
python manage.py test validators— 68 pass (56 directly exercise this feature: version-helper parity + configurable grace, observation/rollup latch, streak + any-node roll-up, version-label parse, auto-target stable-only guard, auto-award dedup/points). Pre-existingcontributionstest failures (discord XP, daily-uptime, featured-content) are an unrelated Python 3.14 test-seeding collision — they fail identically on a cleandevcheckout.🤖 Generated with Claude Code
Summary by CodeRabbit