Skip to content

fix(signals): catch plural private terms in the public-safe comment backstop#1380

Open
minion1227 wants to merge 1 commit into
JSONbored:mainfrom
minion1227:minion_plural-leak
Open

fix(signals): catch plural private terms in the public-safe comment backstop#1380
minion1227 wants to merge 1 commit into
JSONbored:mainfrom
minion1227:minion_plural-leak

Conversation

@minion1227

Copy link
Copy Markdown

Summary

  • containsPrivatePublicTerm (src/signals/engine.ts) is the fail-safe public-safe backstop that drops a public-comment line if it still names private reward/wallet/trust internals. It is the sole gate at four public-surface call sites — publicSafeNextSteps, publicSafePreflightFindings, the inline next-steps filter in buildPublicPrIntelligenceComment, and buildPublicCommentSignalBundle — with no scrub step before it (unlike src/review/unified-comment-bridge.ts, whose byte-identical PRIVATE_DROP_TERMS is safe only because publicSafeNit first scrubs with the plural-aware PRIVATE_FORBIDDEN_TERMS).
  • The denylist wrapped bare-singular terms in word boundaries, so plural forms slipped through: reward matched but rewards did not — likewise payouts, wallets, hotkeys, trust scores, estimated scores. A finding/step whose text used a plural (e.g. a title "Quarterly rewards summary") therefore passed the backstop and could surface on a public GitHub PR comment.
  • Fix: add the s? plural form to the pluralizable terms, matching the intent already established by every sibling denylist — CHECK_RUN_FORBIDDEN_TERMS (src/rules/advisory.ts), PRIVATE_FORBIDDEN_TERMS (src/review/unified-comment-bridge.ts), and FORBIDDEN_PUBLIC_COMMENT_WORDS (src/queue-intelligence.ts), which all use rewards? / wallets? / trust\s+scores?. Purely additive: every input matched before still matches; plurals now match too.
  • No issue linked: this is a small, self-evident correctness fix to an existing sanitizer boundary, kept narrow (one regex line + a regression test). The repo's linkedIssuePolicy is preferred (not required) and issueDiscoveryPolicy is discouraged, so a direct PR with this rationale is the intended path rather than filing a discovery issue.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • 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 an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥97% coverage of the lines AND branches you changed (aim for 98%+ on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • None skipped. (The full suite passes with adequate per-test time; the changed line is covered by the added regression test plus existing public-comment suites.)

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 PR strengthens that guarantee — it closes a plural-form leak in the public-comment backstop.)
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/session/CORS surface; this is the public/private sanitizer boundary, covered by the added sanitizer-boundary regression test.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP shape change.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI change.)
  • Visible UI changes include a UI Evidence section below. (N/A — backend-only, no visible change.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

N/A — backend-only change to a sanitizer regex; no visible UI, frontend, docs, or extension change.

Notes

  • Regression test (test/unit/signals-coverage.test.ts): builds the public comment signal bundle with a finding whose title carries each plural private term and asserts none survive into publicFindingTitles; a singular-form control pins that the fix does not regress the cases already caught. Verified the test fails on the pre-fix regex (the plural leaks) and passes after the fix.

…ackstop

containsPrivatePublicTerm is the sole public-safe gate for next-step lines and
public finding titles/details on the converged PR comment (no scrub partner, unlike
the unified-comment bridge). Its denylist wrapped bare-singular terms in word
boundaries, so plural forms — "rewards", "payouts", "wallets", "hotkeys", "trust
scores", "estimated scores" — passed the boundary and could surface on a public
GitHub comment, while every sibling denylist (advisory.ts, queue-intelligence.ts,
unified-comment-bridge.ts) already uses the plural-aware `s?` forms.

Add `s?` to the pluralizable terms so the backstop matches singular and plural
alike. Add a regression test asserting plural private terms are dropped from public
finding titles, with a singular control proving no regression.
@minion1227 minion1227 requested a review from JSONbored as a code owner June 25, 2026 16:37
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 25, 2026
@superagent-security

Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant