Skip to content

fix(miner): fail closed when referencing PR has missing authorLogin (#7794) - #7824

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/submission-freshness-missing-author-7794
Jul 21, 2026
Merged

fix(miner): fail closed when referencing PR has missing authorLogin (#7794)#7824
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/submission-freshness-missing-author-7794

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • Align checkSubmissionFreshness's author-comparison predicate with claim-conflict-resolver.ts's fail-closed bias: a merged/open referencing PR with a missing/non-string authorLogin now counts as already_addressed
  • Update regression tests for open and merged PRs whose author GitHub cannot identify (e.g. deleted account)

Closes #7794

Test plan

  • npm run typecheck passes locally
  • npm run build:tsc in miner workspace emits matching .js
  • CI validate-code (incl. Miner build drift check) + validate-tests green
  • codecov/patch >= 99% on changed lines

Made with Cursor

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 21, 2026 13:48
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.87%. Comparing base (a314577) to head (bcb6f76).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7824      +/-   ##
==========================================
+ Coverage   88.85%   88.87%   +0.02%     
==========================================
  Files          88       89       +1     
  Lines       21214    21257      +43     
  Branches     3675     3697      +22     
==========================================
+ Hits        18849    18892      +43     
  Misses       2187     2187              
  Partials      178      178              
Flag Coverage Δ
shard-1 44.18% <0.00%> (?)
shard-2 100.00% <100.00%> (?)
shard-3 6.97% <0.00%> (?)

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

Files with missing lines Coverage Δ
...s/loopover-miner/lib/submission-freshness-check.ts 100.00% <100.00%> (ø)

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

loopover-orb Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-21 13:54:43 UTC

3 files · 1 AI reviewer · no blockers · readiness 95/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This diff fixes a real logic bug: previously a referencing PR with a missing/non-string authorLogin was silently excluded from the already_addressed check (since `typeof pr.authorLogin === "string"` was ANDed into the whole predicate), meaning a deleted-account author's merged/open PR would be ignored rather than treated as fail-closed evidence of prior work. The fix correctly restructures the boolean so a non-string authorLogin now falls into the already_addressed branch, matching claim-conflict-resolver.js's existing fail-closed convention for the same field. Both the .ts source and its compiled .js counterpart were updated consistently, and the test suite was updated in kind (including flipping an existing test's expectation from fresh:true to already_addressed, which is the correct behavior change, not a stale assertion).

Nits — 4 non-blocking
  • The compiled .js file is committed alongside the .ts source and manually kept in sync (submission-freshness-check.js:105-106); verify this is the repo's established build-artifact convention and not something a build step should instead regenerate.
  • test/unit/miner-submission-freshness-check.test.ts:165 renamed/repurposed the existing 'ignored rather than crashing' test description but the file still doesn't add a case for an authorLogin that's an empty string after trim — worth confirming that's still intentionally 'stale-but-not-empty' out of scope.
  • Consider a short changelog/comment note near the `addressedByAnotherAuthor` predicate cross-referencing claim-conflict-resolver.js's identical fail-closed pattern, since a future edit to one is likely to need the same edit to the other.
  • If the .js file is hand-maintained, a lint/CI check that diffs `tsc` output against the committed .js would catch future drift automatically.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7794
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: 395 registered-repo PR(s), 185 merged, 36 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 395 PR(s), 36 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff changes the predicate so a merged/open referencing PR with a non-string/missing authorLogin now short-circuits to true (matching claim-conflict-resolver.ts's bias), and updates/adds regression tests (including a null authorLogin case) asserting the result is now {fresh: false, reason: "already_addressed"} instead of {fresh: true}.

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, Ruby, Svelte, TypeScript, Markdown, MDX
  • Official Gittensor activity: 395 PR(s), 36 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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.

🟩 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

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@loopover-orb
loopover-orb Bot merged commit 1b30e5b into JSONbored:main Jul 21, 2026
12 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.

checkSubmissionFreshness silently trusts a PR with a missing authorLogin, contradicting its own documented fail-closed contract

1 participant