Skip to content

fix(scoring): exclude operational constants from unmodeled scoring drift - #1044

Merged
JSONbored merged 2 commits into
JSONbored:mainfrom
bohdansolovie:fix/exclude-operational-constants-from-drift
Jun 23, 2026
Merged

fix(scoring): exclude operational constants from unmodeled scoring drift#1044
JSONbored merged 2 commits into
JSONbored:mainfrom
bohdansolovie:fix/exclude-operational-constants-from-drift

Conversation

@bohdansolovie

Copy link
Copy Markdown
Contributor

Refs #809

Summary

findUnmodeledConstantKeys treated every upstream numeric constant outside SCORING_CONSTANT_NAMES as "unmodeled scoring drift." That included operational/infra constants the scorer never consumes — HTTP timeouts, byte limits, UID sentinels, subprocess budgets, etc.

Those false positives inflated snapshot warnings and could trigger high-severity upstream drift reports without any actual scoring gap.

Problem

Upstream constants.py includes both scoring dimensions and operational knobs. Examples that were incorrectly flagged:

  • SECONDS_PER_DAY, SECONDS_PER_HOUR
  • GITHUB_HTTP_TIMEOUT_SECONDS, MIRROR_HTTP_TIMEOUT_SECONDS, MIRROR_MAX_ATTEMPTS
  • TREE_SITTER_PARSE_TIMEOUT_MICROS, SCORING_SUBPROCESS_BUDGET_S
  • MAX_FILE_SIZE_BYTES
  • RECYCLE_UID, ISSUES_TREASURY_UID, MAX_ISSUE_ID

A genuinely unmodeled scoring constant like NOVELTY_BONUS_SCALAR should still surface.

Changes

  • src/scoring/model.ts — add NON_SCORING_UPSTREAM_CONSTANT_NAMES ignore-set; filter operational constants out of findUnmodeledConstantKeys.
  • test/unit/scoring.test.ts — regression test: operational-only slice → []; operational + NOVELTY_BONUS_SCALAR → only the scoring gap.

Detection-only change — no score preview math changes.

Scope boundary

Does not model MAX_LINES_SCORED_FOR_NON_CODE_EXT or other dropped scoring constants (#809 / #991 follow-up). Those correctly remain flagged as genuinely unmodeled after this noise reduction.

API / OpenAPI / MCP contract

No schema changes. Fewer false positives in snapshot warnings and upstream drift reports.

Migration / deploy / secrets

None.

Security / privacy

None.

Validation

npm run test:ci

All gates passed locally on branch fix/exclude-operational-constants-from-drift.

Distinct from prior PRs

Unrelated to label multiplier selection (#994) or fractional underscore parsing (#992).

@bohdansolovie
bohdansolovie requested a review from JSONbored as a code owner June 22, 2026 15:45
@ghost ghost added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jun 22, 2026
@dosubot dosubot Bot added the size:S label Jun 22, 2026
@ghost

ghost commented Jun 22, 2026

Copy link
Copy Markdown

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review — safe to merge

3 files · 2 AI reviewers · no blockers · readiness 93/100 · CI green

✅ Approved — safe to merge

Review summary
The changes correctly introduce a filter to exclude known operational constants from unmodeled scoring drift detection and update tests accordingly. No build-breaking issues or runtime errors are introduced.

Suggestions

  • Consider adding a comment explaining why `NON_SCORING_UPSTREAM_CONSTANT_NAMES` includes specific constants. This will help future maintainers understand the rationale behind the exclusion.
  • Ensure that the new test cases in `scoring.test.ts` cover all edge cases, especially those involving mixed operational and scoring constants.

Risks

  • The change might inadvertently exclude other constants that should be considered for scoring. Regular reviews and updates to `NON_SCORING_UPSTREAM_CONSTANT_NAMES` will be necessary to mitigate this risk.
Signal Result Evidence
Code review ✅ No blockers 2 reviewers, synthesized
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Review load ✅ 20/20 Readiness component derived from cached public PR metadata and labels; size label size:S.
Validation evidence ✅ 25/25 PR body includes validation/test evidence.
Open PR queue ✅ 10/10 0 open PR(s), 0 likely reviewable.
Contributor context ✅ Confirmed Gittensor contributor bohdansolovie; Gittensor profile; 15 PR(s), 3 issue(s).
Gate result ✅ Passing No configured blocker found.
Nits — 1 non-blocking
  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
Review context
  • Author: bohdansolovie
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python
  • Official Gittensor activity: 15 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

The changes correctly introduce a filter to exclude known operational constants from unmodeled scoring drift detection and update tests accordingly. No build-breaking issues or runtime errors are introduced.

Suggestions

  • Consider adding a comment explaining why `NON_SCORING_UPSTREAM_CONSTANT_NAMES` includes specific constants. This will help future maintainers understand the rationale behind the exclusion.
  • Ensure that the new test cases in `scoring.test.ts` cover all edge cases, especially those involving mixed operational and scoring constants.

Risks

  • The change might inadvertently exclude other constants that should be considered for scoring. Regular reviews and updates to `NON_SCORING_UPSTREAM_CONSTANT_NAMES` will be necessary to mitigate this risk.

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@ghost ghost added the gittensory:reviewed label Jun 22, 2026
@ghost

ghost commented Jun 22, 2026

Copy link
Copy Markdown

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

🛠️ Maintainer review — Held for maintainer review

Reviewed 3 changed file(s) — two independent AI reviewers, synthesized. 🛠️ Request changes. (reviewers split: request changes / merge) Address the suggestions below before merging.

ℹ️ The reviewers were not unanimous (a change was requested) — held for your review rather than auto-closed.

Merge readiness: ✅ Mergeable · all CI checks green/neutral/skipped.

Summary

The change introduces a duplicate declaration of NON_SCORING_UPSTREAM_CONSTANT_NAMES in src/scoring/model.ts, which will cause a TypeScript compilation error and break the build. This is a concrete blocker. The added tests are appropriate, but the code must be fixed before merging.

🛑 Concerns raised — review before merging (reviewers were not unanimous)

  • Duplicate declaration of NON_SCORING_UPSTREAM_CONSTANT_NAMES in src/scoring/model.ts causes a compilation error.

💡 Nits — non-blocking, optional

  • Test timeout increased to 30_000ms – ensure this is intentional.
  • Formatting of the new test block could follow existing style (e.g., single quotes vs double).
  • Remove the newly added duplicate NON_SCORING_UPSTREAM_CONSTANT_NAMES block; keep only the original definition.
  • Consider moving the constant definition next to other related constants for clarity.
  • Ensure the import order remains consistent after the removal.

@ghost ghost added the reviewbot-review label Jun 22, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.88%. Comparing base (7ca16da) to head (6ab9989).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1044   +/-   ##
=======================================
  Coverage   94.88%   94.88%           
=======================================
  Files         151      151           
  Lines       18271    18272    +1     
  Branches     6615     6615           
=======================================
+ Hits        17337    17338    +1     
  Misses        419      419           
  Partials      515      515           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bohdansolovie
bohdansolovie force-pushed the fix/exclude-operational-constants-from-drift branch from b5c211d to f7b4f8c Compare June 22, 2026 15:57
bohdansolovie and others added 2 commits June 23, 2026 08:05
Exercise the unmodeled.length > 12 ellipsis path in refreshScoringModelSnapshot
so project coverage does not regress after operational constants are filtered.

Co-authored-by: Cursor <cursoragent@cursor.com>
@bohdansolovie
bohdansolovie force-pushed the fix/exclude-operational-constants-from-drift branch from be11fb7 to 6ab9989 Compare June 23, 2026 06:06

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@dosubot dosubot Bot added the lgtm label Jun 23, 2026
@JSONbored
JSONbored merged commit 893acfc into JSONbored:main Jun 23, 2026
16 checks passed
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants