fix(review): re-sync the two public-surface redaction term lists#8445
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
unified-comment-bridge.ts documents PRIVATE_FORBIDDEN_TERMS as mirroring advisory.ts's CHECK_RUN_FORBIDDEN_TERMS, but the two had drifted in both directions: the Nit path was missing bare 'raw trust', mnemonic, seed phrase, cohort, miner-originated, human-originated and rankings, so those reached public PR comments unredacted even though the check-run path scrubs them; the check-run path was missing likely_duplicate and reviewability<digit>. Both regexes are now the identical union, and the Nit drop-guard gains cohort and mnemonic. Adds a behavioral parity test that drives one string per term through BOTH sanitizers via their real public entry points, so a future one-sided edit fails -- verified it fails when either list is reverted. Closes JSONbored#8322
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8445 +/- ##
==========================================
+ Coverage 83.56% 89.80% +6.24%
==========================================
Files 791 99 -692
Lines 79317 23296 -56021
Branches 23954 4114 -19840
==========================================
- Hits 66284 20922 -45362
+ Misses 10472 2187 -8285
+ Partials 2561 187 -2374
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-24 14:07:47 UTC
Review summary Nits — 3 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk 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.
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.
|
Summary
src/review/unified-comment-bridge.tsdocumentsPRIVATE_FORBIDDEN_TERMSas mirroringsrc/rules/advisory.ts'sCHECK_RUN_FORBIDDEN_TERMS. It did not — the two had drifted in both directions, so a term redacted on one public surface leaked on the other:advisory.ts): bareraw\s+trust,mnemonics?,seed\s?phrases?,cohorts?,miner[-_\s]?originated,human[-_\s]?originated,rankings?. A Nit naming e.g. "cohort" or "mnemonic" reached a public GitHub comment unredacted, while the identical term was correctly scrubbed on the check-run path.likely_duplicate,reviewability\s*\d(the digit variant exists becausereviewability3has no word boundary before the digit).Both regexes are now the identical union of the two vocabularies, and
PRIVATE_DROP_TERMS(the Nit fail-safe) gainscohortandmnemonic— the two most likely to appear in free-text Nits.Closes #8322
The parity test
The issue asked for a real behavioral test that fails if either list regresses, explicitly not a string comparison of the two regex sources (their syntax legitimately differs). The new test drives one representative string per term — 25 of them, the union — through both sanitizers via their real public entry points (
buildDualReviewNotes→publicSafeNit, andbuildCheckRunAnnotations→sanitizeForCheckRun), asserting the term never survives.I verified it actually bites, rather than assuming it does:
advisory.tsback to its drifted formThat second case matters: my first draft of the check-run leg passed against a drifted
advisory.ts— a false negative.buildCheckRunAnnotationsskips findings withoutpublicTextand only annotates files shaped asPullRequestFileRecord(path, notfilename), so it was asserting against an empty annotation list. Fixed by putting the term inpublicTextand adding an explicit non-vacuity assertion (annotations.length > 0) so the check-run leg can never silently assert nothing again.Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% 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:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
src/**plus one test file, soactionlint(no workflow change),build:mcp/test:mcp-pack(no MCP change),ui:*(no UI/OpenAPI change),test:workers(no worker change), andnpm audit(no dependency change) are not exercised by it.vitest run --coverage --coverage.all=false): all three changed source lines (advisory.ts:382,unified-comment-bridge.ts:74-75) are executed, with every newly-added term individually exercised by the parity test as the coverage requirement demands.unified-comment-bridge.test.ts+rules.test.tspass in full (195 tests) and roottsc --noEmitis clean.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.UI Evidence
Not applicable — two backend redaction regexes and a test; no visible UI, frontend, docs, or extension change.
Notes
raw\s+trust\s+scores?stays ahead of bareraw\s+trust).