Skip to content

fix(rules): honor alreadyPublicSafe in check-run annotations (#8321)#8459

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:fix/advisory-annotation-already-public-safe
Jul 24, 2026
Merged

fix(rules): honor alreadyPublicSafe in check-run annotations (#8321)#8459
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:fix/advisory-annotation-already-public-safe

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Summary

Closes #8321.

formatCheckRunOutput in src/rules/advisory.ts already honors a finding's alreadyPublicSafe flag when building the check-run text:

return [`${label} ${f.alreadyPublicSafe ? f.publicText : sanitizeForCheckRun(f.publicText)}`];

Per #7981, that flag marks a fixed, engineer-authored message with no interpolated contributor/AI content, so scrubbing it would mangle a deliberately-worded string -- e.g. turning "...secret, wallet, PAT..." into "...secret, [context], PAT...".

buildCheckRunAnnotations renders those same findings as inline annotations, but its addCandidate helper sanitized the message unconditionally and never received the flag. The result: one finding, on one PR, rendered verbatim in the check-run text but scrubbed in the annotation for that same finding -- reintroducing the exact bug class #7981 fixed, in a sibling path #7981 didn't touch.

Change

addCandidate now takes an alreadyPublicSafe parameter (defaulting to false) and skips the message sanitizer when it is set, mirroring formatCheckRunOutput's conditional exactly.

  • The title deliberately keeps its sanitizer: there is no per-title safety flag on a finding, so nothing licenses skipping it (as the issue requires -- no new flag invented).
  • Findings without the flag are byte-for-byte unchanged, and the parameter's default keeps the other two addCandidate call sites (missing-test evidence, duplicate overlap) untouched.

Tests

Two tests pinning both arms on the annotation path, placed beside the existing #7981 formatCheckRunOutput test so the pair reads together:

  • an alreadyPublicSafe finding's annotation message renders verbatim (no [context] placeholder)
  • a finding without the flag is still sanitized (unchanged behavior)

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (Closes #8321).

Validation

  • git diff --check
  • npm run actionlint - no workflow files touched.
  • npm run typecheck
  • npx vitest run test/unit/rules.test.ts - 101 pass (was 99).
  • Changed-line coverage verified locally with the v8 provider: the ternary at the sanitizer takes both arms ([1, 164]), the ?? false at the call site takes both ([31, 30]), and the new default-arg is exercised ([165]) by the two untouched call sites.
  • npm run test:coverage (full) / test:workers / build:mcp / ui:* - not run: this is a scoped change to one backend module plus its unit test, touching no worker, MCP, or UI surface.
  • npm audit --audit-level=moderate - no dependency changes.
  • New or changed behavior has unit tests for new branches and fallback paths.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics -- this only stops double-scrubbing a message the codebase already classifies as public-safe; every unflagged finding still goes through sanitizeForCheckRun.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests - n/a, none touched.
  • API/OpenAPI/MCP behavior is updated and tested where needed - n/a.
  • UI changes use live API data or real empty/error/loading states - n/a, no UI change.
  • Visible UI changes include a UI Evidence section - n/a: no visible UI/frontend/docs/extension change.
  • Public docs/changelogs are updated where needed - n/a.

Notes

  • The flag is opt-in and set only by engineer-authored findings, so this cannot widen what contributor- or AI-derived text can put into a check run.

…ed#8321)

formatCheckRunOutput already renders a finding's publicText verbatim when it
declares alreadyPublicSafe (JSONbored#7981): the flag marks a fixed, engineer-authored
message with no interpolated contributor/AI content, so running it through
sanitizeForCheckRun would mangle a deliberately-worded string (turning
"...secret, wallet, PAT..." into "...secret, [context], PAT...").

buildCheckRunAnnotations renders those same findings as inline annotations,
but its addCandidate helper sanitized the message unconditionally -- so one
finding on one PR showed verbatim in the check-run text and scrubbed in the
annotation, reintroducing the bug class JSONbored#7981 fixed in a sibling path it did
not touch.

addCandidate now takes an alreadyPublicSafe flag (default false) and skips the
message sanitizer when set, mirroring formatCheckRunOutput's conditional
exactly. The title keeps its sanitizer: there is no per-title safety flag on a
finding, so nothing licenses skipping it. Findings without the flag are
unchanged -- the default keeps the other two addCandidate call sites
byte-identical.

Adds two tests pinning both arms on the annotation path: an alreadyPublicSafe
finding renders verbatim, and one without the flag is still sanitized.
@galuis116
galuis116 requested a review from JSONbored as a code owner July 24, 2026 14:04
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.70%. Comparing base (1dba0ec) to head (576fe43).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8459      +/-   ##
==========================================
- Coverage   92.43%   89.70%   -2.73%     
==========================================
  Files         791       98     -693     
  Lines       79320    23028   -56292     
  Branches    23952     4012   -19940     
==========================================
- Hits        73317    20657   -52660     
+ Misses       4866     2187    -2679     
+ Partials     1137      184     -953     
Flag Coverage Δ
shard-1 67.39% <0.00%> (+9.62%) ⬆️
shard-2 84.47% <100.00%> (+36.80%) ⬆️
shard-3 59.31% <33.33%> (+2.72%) ⬆️

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

Files with missing lines Coverage Δ
src/rules/advisory.ts 98.13% <100.00%> (+0.01%) ⬆️

... and 693 files with indirect coverage changes

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

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-24 14:31:34 UTC

2 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR adds an `alreadyPublicSafe` parameter to `addCandidate` in `buildCheckRunAnnotations`, mirroring the conditional sanitizer-skip already present in `formatCheckRunOutput`, and threads `finding.alreadyPublicSafe ?? false` through the one call site that renders findings as annotations (src/rules/advisory.ts:540). The change is narrow, correctly named/scoped (title still sanitized since no per-title flag exists), defaults preserve byte-identical behavior for existing callers, and the two added tests pin both arms of the flag on the annotation path exactly as described. This closes the sibling-path gap from #7981/#8321 without inventing new surface.

Nits — 3 non-blocking
  • src/rules/advisory.ts:486 — the `65535` GitHub annotation message-length limit is an existing magic number this diff touches; consider naming it (e.g. `CHECK_RUN_ANNOTATION_MESSAGE_MAX_CHARS`) while you're in this line, though it predates this PR so it's optional.
  • The new parameter comment at src/rules/advisory.ts:479-483 is fairly long for a single default-parameter annotation; a one-line pointer to formatCheckRunOutput's own comment would read just as clearly.
  • Consider extracting the 65535 literal into a shared constant alongside CHECK_RUN_ANNOTATION_LIMIT if there's a natural home for check-run-related limits in this file.

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 #8321
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: 1895 registered-repo PR(s), 1231 merged, 56 issue(s).
Contributor context ✅ Confirmed Gittensor contributor galuis116; Gittensor profile; 1895 PR(s), 56 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: minor
Linked issue satisfaction

Addressed
The diff threads finding.alreadyPublicSafe through addCandidate's new parameter and conditionally skips sanitizeForCheckRun on the message exactly as formatCheckRunOutput does, while leaving the title sanitizer untouched and other call sites defaulting to false. Added tests cover both branches (verbatim for alreadyPublicSafe, sanitized otherwise) and mirror the issue's parity requirement.

Review context
  • Author: galuis116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript, Python, TypeScript, Dart, HTML, MDX, Rust, C++
  • Official Gittensor activity: 1895 PR(s), 56 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 b554e15 into JSONbored:main Jul 24, 2026
14 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.

buildCheckRunAnnotations scrubs alreadyPublicSafe findings that formatCheckRunOutput already renders verbatim

1 participant