Skip to content

fix(scoring): skip unmodeled-drift sync on garbage upstream constants bodies - #8972

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
philluiz2323:fix/8902-garbage-upstream-unmodeled-drift-guard
Jul 26, 2026
Merged

fix(scoring): skip unmodeled-drift sync on garbage upstream constants bodies#8972
JSONbored merged 1 commit into
JSONbored:mainfrom
philluiz2323:fix/8902-garbage-upstream-unmodeled-drift-guard

Conversation

@philluiz2323

Copy link
Copy Markdown
Contributor

Problem

Closes #8902.

refreshScoringModelSnapshot correctly fails closed when a 200 constants.py body is semantically garbage and a verified last-good exists — but with no last-good (first run / prior fallback), execution continues and the drift-sync guard was only if (constantsResult.ok). That still ran findUnmodeledUpstreamConstants against HTML/LFS junk and could open a spurious upstream_drift_reports row.

Fix

Gate syncUnmodeledScoringConstantDrift on constantsUsable (the same recognized-count floor the fail-closed path already uses), not merely HTTP-ok.

Tests

  • Existing no-last-good garbage bootstrap test now asserts listUpstreamDriftReports stays empty even when the junk body contains a parseable fake unmodeled constant name.
  • Existing real-unmodeled path still opens a drift report (regression).

Reverting the guard fails the empty-reports assertion. git diff --check clean.

Scope

Validation

  • git diff --check
  • Targeted vitest for garbage bootstrap + unmodeled scoring dimension paths (2 pass)
  • Scoring-only; unrelated UI/MCP checks not exercised.

Safety

  • No secrets / private scores exposed
  • No UI changes (N/A evidence)

… bodies

The fail-closed path freezes last-good when constants.py is garbage-but-200, but
with no verified last-good the refresh still fell through and ran
syncUnmodeledScoringConstantDrift on HTTP-ok alone — opening a spurious drift
report from HTML/LFS junk. Gate the sync on constantsUsable instead.

Closes JSONbored#8902
@philluiz2323
philluiz2323 requested a review from JSONbored as a code owner July 26, 2026 14:38
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.57%. Comparing base (b723ba8) to head (16ccc5c).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8972      +/-   ##
==========================================
+ Coverage   90.56%   90.57%   +0.01%     
==========================================
  Files          96       97       +1     
  Lines       22490    22558      +68     
  Branches     3884     3908      +24     
==========================================
+ Hits        20367    20433      +66     
  Misses       1945     1945              
- Partials      178      180       +2     
Flag Coverage Δ
backend 97.05% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/scoring/model.ts 97.05% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 26, 2026
@loopover-orb

loopover-orb Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-26 15:20:47 UTC

2 files · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

Decision drivers

  • ✅ Code review — No blockers (No AI review summary)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8902
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 989 registered-repo PR(s), 565 merged, 125 issue(s).
Contributor context ✅ Confirmed Gittensor contributor philluiz2323; Gittensor profile; 989 PR(s), 125 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff changes line 137's guard from `if (constantsResult.ok)` to `if (constantsUsable)`, exactly matching the requested fix, and adds an assertion that `listUpstreamDriftReports` stays empty for a 200-with-garbage-body fetch with no last-good snapshot, even when the garbage contains a parseable fake unmodeled constant name.

Review context
  • Author: philluiz2323
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, MDX, TypeScript, CSS, Cuda, HTML, Kotlin
  • Official Gittensor activity: 989 PR(s), 125 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: merge · clause: success
  • config: 03a7f8b529a9 · pack: oss-anti-slop
  • record: 763bc93a3043 (schema v2, head 16ccc5c)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@JSONbored
JSONbored merged commit cce7f00 into JSONbored:main Jul 26, 2026
8 checks passed
JSONbored added a commit that referenced this pull request Jul 27, 2026
…nd name every silent AI-review skip (#9180)

* fix(review): finalize CI-stuck silence, post a waiting placeholder, and name every silent AI-review skip

#9011: prReadyForReview used to defer a missing-required-context PR forever past
the 2-minute cap instead of falling through to the existing finalize-past-cap
block; a required context that never appears keeps ciState pending (never
"passed"), so finalizing here can never produce a would-merge disposition. A new
stuckReason ("missing_required_context" vs "ci_running") is threaded through the
audit/log calls so the two causes are distinguishable.

#9042: the silent wait before a review runs (measured live: 8.3min median, 21.9min
p90, up to 50.9min) now gets an immediate "waiting" panel comment
(renderWaitingForCiPlaceholder) upserted through the same PR_PANEL_COMMENT_MARKER
the final verdict replaces, instead of leaving the PR blank until CI settles.

#9000: root-causes the #8972 incident where a forced ("Re-run LoopOver review")
retrigger completed with none of the events a forced pass should emit anywhere in
the audit trail. shouldStartAiReviewForAdvisory folds in
shouldRequirePublicAiReviewForAdvisory's hard gate (aiReviewMode off, an
ineligible author, no head SHA, an AI kill-switch off, no AI binding) - a forced
retrigger does not bypass this gate, and every one of its branches previously
returned false with zero audit trail. resolvePublicAiReviewGateSkipReason names
the exact reason, and a new catch-all audit event fires whenever aiReviewWillRun
ends up false for a cause none of the other (already-audited) paths cover. The
two adjacent silent branches in the frozen/paused/one-shot reuse chain - a hold
condition true but nothing published to actually reuse - get their own named
"unavailable" events for the same reason.

This only closes the exhaustive-auditing and root-cause portion of #9000; the
receipt-feedback / lost-webhook-click recovery sweep it also asks for is not yet
implemented.

Closes #9011
Closes #9042

* fix(review): correct stale 'gittensory' brand reference in a comment

#9173 introduced a comment referencing the deprecated pre-rename product name,
which the branding-drift check on main was not catching -- breaking CI for
every PR based on current main, including this one.
loopover-orb Bot pushed a commit that referenced this pull request Jul 28, 2026
…erasing them (#9000) (#9584)

The lost-click half of #9000, completing the issue (the skip-auditing half landed
earlier; its enforcement went in as #9583).

THE DEFECT. Three checkbox states looked identical to a maintainer: processed (panel
republished, box reset), deferred for CI (box stays ticked, honored later via the
#7626 pending marker), and DELIVERY LOST (box stays ticked, nothing recorded, nothing
will ever happen -- three such losses observed live on #8972, 2026-07-26). Only the
third is broken, and it is unrecoverable from our own state alone: the tick exists
only in the live comment body, because the webhook that would have told us about it
never became a job. Worse, the next panel republish -- whose renderer emits its
checkboxes unticked unconditionally -- would OVERWRITE the ticked box, erasing the
only evidence of the click on the exact pass best placed to honor it.

THE FIX, at zero additional API cost. createOrUpdateIssueCommentWithMarker already
fetches the existing comment for its marker search; it now returns that pre-overwrite
body. Every panel republish (webhook pass, re-gate sweep, backlog convergence) doubles
as the detector: a ticked rerun box in the body being replaced, with no recent
processing and no pending marker, IS a lost click. Recovery then:

  - records github_app.pr_panel_retrigger_recovered (the named reason #9003 demands,
    and the loop guard against re-detection),
  - persists the #7626 pending marker, so the next review pass consumes it as
    forceAiReview exactly like a deferred click, and
  - enqueues an agent-regate-pr job immediately (with the #9499 prCreatedAt sort key)
    -- a PR with a lost click is precisely the PR that cannot count on a natural pass.

No new sweep, no new persistence: the issue's "within one sweep interval" acceptance
is bounded by the republish cadence that already exists.

FALSE-POSITIVE PROOFING, the part that took the design care:

  - A pass that IS the retrigger (forceAiReview) skips detection -- overwriting its
    own tick is the receipt acknowledgement, not a loss.
  - A recently-processed retrigger (actor-agnostic audit lookup, new
    countAuditEventsForTargetSince -- hasAuditEventForDelivery is deliberately
    actor+delivery-keyed, the wrong shape when the delivery never existed) means the
    delivery raced the pass rather than being lost.
  - A DEFERRED click is guarded by ordering, not luck: the retrigger handler now marks
    the pending marker BEFORE its readiness check (consuming it right back on the
    immediate path), so the CI-wait placeholder inside that very pass sees the marker
    and stands down. A crash between mark and consume degrades to one extra forced
    review -- the safe direction for an explicit user click.

The #7626 sibling test's "never creates a pending marker" assertion is updated to the
invariant it documents ("no pending INTENT survives the immediate path") rather than
its old mechanism (key absence), since consumption can leave the one-shot consumed
sentinel behind on adapters without a real delete.

TESTS. Eight in the new suite: the #8972 regression (natural pass recovers the tick:
event + marker + job with the sort key), full end-to-end (the recovery job's pass
consumes the marker, spends exactly one fresh AI review, and does not re-diagnose its
own republish), the raced-delivery guard, the deferred-click guard (CI genuinely
pending, marker untouched afterward), the no-head ghost PR (marker guard skipped, job
omits the optional sort key), unticked and first-publish no-ops, and the empty
event-type-list guard. Plus the three github-comments assertions extended to pin
previousBody, including that it is the POSTED body, not the re-render, on the
byte-identical skip path. Changed lines: 0 uncovered statements, 0 uncovered branches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(scoring): stop garbage upstream bodies from creating false "unmodeled constants" drift reports

2 participants