Skip to content

fix(review): stop bare hotkey/coldkey mentions from false-positiving as secret leaks#7994

Merged
JSONbored merged 5 commits into
mainfrom
fix/hotkey-coldkey-secret-scan-false-positive
Jul 22, 2026
Merged

fix(review): stop bare hotkey/coldkey mentions from false-positiving as secret leaks#7994
JSONbored merged 5 commits into
mainfrom
fix/hotkey-coldkey-secret-scan-false-positive

Conversation

@JSONbored

@JSONbored JSONbored commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Closes #7981.

Summary

Test plan

  • npm run typecheck
  • New/updated tests: containsSecretLikeText (genuine key-material leaks still caught; all 4 incident payloads now pass), surfaceVerdictToGate (alreadyPublicSafe: true on close/manual findings), unified-comment-bridge (blocker renders verbatim with the flag, still scrubbed without it), formatCheckRunOutput (same, check-run path)
  • Full unsharded npm run test:coverage: 1072/1072 files, 20095 tests, 0 failures
  • 100% line/branch coverage on every new/changed line

…as secret leaks

containsSecretLikeText matched the bare words "hotkey"/"coldkey" anywhere in a
registry document, unlike "wallet" (already scoped to "wallet path"). A Bittensor
hotkey is the standard PUBLIC miner identifier, not secret material, and appears
routinely in ordinary registry content -- API paths, field names, even notes
explicitly denying any such data ("No wallet/hotkey data"). Confirmed root cause
of 4 mis-closed metagraphed PRs in one day (#7469, #7589, #7591, #7594).

Scope hot/coldkey the same way wallet already is: require adjacency to something
that actually indicates key material (a keystore path, a private-key/password/
mnemonic/seed qualifier) rather than a bare word match.

Also fixes a companion bug: the privacy scrub that redacts private-rubric terms
from dynamically assembled/AI-generated public text was being applied to the
reviewer's own static, hardcoded secret-detection message too, rendering
"...secret, wallet, PAT..." as the confusing "...secret, [context], PAT...".
AdvisoryFinding gains an alreadyPublicSafe flag a producer can set when its
detail/publicText has no interpolated contributor or AI content, so a fixed
message an engineer already wrote and reviewed renders verbatim.

Closes #7981
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.91%. Comparing base (eb31847) to head (4f13691).
⚠️ Report is 6 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7994   +/-   ##
=======================================
  Coverage   91.91%   91.91%           
=======================================
  Files         739      739           
  Lines       75840    75842    +2     
  Branches    23016    23017    +1     
=======================================
+ Hits        69707    69709    +2     
  Misses       5041     5041           
  Partials     1092     1092           
Flag Coverage Δ
shard-1 58.35% <22.22%> (-0.01%) ⬇️
shard-2 52.12% <66.66%> (+<0.01%) ⬆️
shard-3 50.03% <44.44%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
src/review/content-lane-wire.ts 98.61% <100.00%> (+0.01%) ⬆️
src/review/content-lane/registry-logic.ts 100.00% <100.00%> (ø)
src/review/unified-comment-bridge.ts 99.62% <100.00%> (+<0.01%) ⬆️
src/rules/advisory.ts 97.78% <100.00%> (ø)
src/types.ts 100.00% <ø> (ø)

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

loopover-orb Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-22 16:12:57 UTC

15 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
The AI review returned non-blocking notes for this change but did not include a separate narrative summary. Review the nits below before deciding this PR.

Nits — 5 non-blocking
  • `src/review/content-lane-wire.ts`: `surfaceFinding`'s new `alreadyPublicSafe` default parameter relies on every future call site remembering not to pass `true` for interpolated content — a short assertion or lint comment at the call site could help, but the doc comment already explains this well.
  • The regex in `registry-logic.ts:110` combines several qualifiers into one alternation; a named constant per qualifier group (e.g. `KEY_MATERIAL_QUALIFIERS`) could make future additions less error-prone, though the accompanying comment already documents intent thoroughly.
  • Test coverage for `containsSecretLikeText` is thorough, but consider also testing a hyphen/underscore-separated qualifier variant beyond `path`/`private_key` to fully exercise the `[ _-]?` optional-separator branch for every alternative.
  • In `unified-comment-bridge.ts`, `gateBlockerLines`'s ternary (`line && finding.alreadyPublicSafe ? line : publicSafeNit(line)`) could be a one-liner helper (e.g. `publicSafeOrVerbatim`) shared with `advisory.ts`'s analogous check, since both files now duplicate the same `alreadyPublicSafe ? raw : scrub(raw)` logic.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

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 #7981
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: 17 registered-repo PR(s), 14 merged, 301 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 17 PR(s), 301 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, JavaScript, MDX, Shell, Solidity
  • Official Gittensor activity: 17 PR(s), 301 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
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 added the manual-review Gittensor contributor context label Jul 22, 2026
…n parity

The #7981 fix touched src/rules/advisory.ts (formatCheckRunOutput) without a
matching edit to its gate-decision twin (packages/loopover-engine/src/advisory/gate-advisory.ts),
which has no such function. check-engine-parity.ts requires a version bump in
that case; expected-engine.version must stay in sync with it too.
…parity check

runEngineParityChecks aliased an un-overridden baseEngineVersion straight to
headEngineVersion, while its changedFiles default already resolved via a real
git diff against origin/main. That asymmetry meant a genuine version bump
could never be detected unless the caller passed baseEngineVersion explicitly
(only runEngineParityMain did), so any branch with a single-sided
gate-decision edit plus a correct version bump still failed this check.
Discovered while landing the #7981 hotkey/coldkey fix, which needed exactly
that bump for src/rules/advisory.ts's formatCheckRunOutput.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 22, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
loopover-ui 307e179 Commit Preview URL

Branch Preview URL
Jul 22 2026, 02:59 PM

.release-please-manifest.json tracks packages/loopover-engine's version
independently of package.json; the 3.4.0 -> 3.4.1 twin-pair parity bump
needed a matching entry here too, caught by release-manifest:sync:check.
@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

@JSONbored
JSONbored merged commit c73f518 into main Jul 22, 2026
13 checks passed
@JSONbored
JSONbored deleted the fix/hotkey-coldkey-secret-scan-false-positive branch July 22, 2026 17:22
loopover-orb Bot pushed a commit that referenced this pull request Jul 22, 2026
…ent (#8068)

Closes #8067.

heldForReview only checked for a hard-guardrail-path hit, so a PR whose
manual-review label came from elsewhere (a since-resolved gate blocker
on a protected author, a migration collision, ...) kept headlining
"approve/merge recommended" while the executor's live-label guard
silently denied every merge/approve attempt, indefinitely, with no
visible explanation on the PR. Confirmed live on PR #7994, stuck 3+
hours despite fully green CI, before being merged manually.

Broaden the check to also fire on the live, configured manual-review
label being present — this only changes what the comment says, not the
"only a maintainer removes the label" safety invariant itself.
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 bare-word hotkey/coldkey false positives in containsSecretLikeText + static-message redaction bug

1 participant