From ae9fb66409d1f255dab2fb7a9e91154f92234c04 Mon Sep 17 00:00:00 2001 From: Nick M Date: Tue, 14 Jul 2026 09:03:46 -0500 Subject: [PATCH] =?UTF-8?q?fix(review):=20never=20render=20a=20=E2=9D=8C?= =?UTF-8?q?=20for=20a=20non-Gittensor=20contributor=20(#5100)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Contributor context row showed "❌ No public Gittensor match" for any author without a public Gittensor match — a hard-failure icon directly next to evidence reading "not a blocker." The signal table had no neutral state (only ok/warn/fail), so a non-blocking row had nowhere to land. Add a neutral `info` (ℹ️) state to the unified table and route the contributor row through it. This also fixes a latent double-icon bug: the Improvement "none detected" row rendered as "⚠️ ℹ️" (a warn icon plus an un-stripped legacy ℹ️) and is now a single, correct ℹ️. --- src/review/unified-comment-bridge.ts | 5 ++++- src/review/unified-comment.ts | 6 ++++-- src/signals/engine.ts | 5 ++++- test/unit/queue-4.test.ts | 7 +++++-- test/unit/unified-comment-bridge.test.ts | 12 +++++++++++- 5 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 14eaf1e89e..0a45cf8751 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -116,12 +116,15 @@ export function verdictToRecommendation(verdict: Verdict): ReviewRecommendation function rowState(resultCell: string): UnifiedSignalRow["state"] { if (resultCell.startsWith("✅")) return "ok"; if (resultCell.startsWith("❌")) return "fail"; + // A leading ℹ️ is an explicit neutral/informational marker (e.g. "no public Gittensor match", "none detected") — + // it must map to the `info` state, not fall through to `warn`, so a non-blocking row never renders as ⚠️. + if (resultCell.startsWith("ℹ️")) return "info"; return "warn"; } /** Strip the leading status icon from a result cell so it is not duplicated next to the unified icon. */ function rowResultText(resultCell: string): string { - return resultCell.replace(/^[✅⚠️❌]+\s*/u, "").trim(); + return resultCell.replace(/^[✅⚠️❌ℹ️]+\s*/u, "").trim(); } /** Map the legacy panel signal rows → the unified table's rows (label/state/result/evidence). The diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 38d611fc46..9952f39ed1 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -198,7 +198,7 @@ export interface UnifiedReviewInput { /** One row of the readiness signal table (loopover side, host-provided; the engine adds Code review). */ export interface UnifiedSignalRow { label: string; - state: "ok" | "warn" | "fail"; + state: "ok" | "warn" | "fail" | "info"; /** Short result text, e.g. "Linked", "25/25". */ result?: string; /** Evidence cell, e.g. "#1372". */ @@ -296,7 +296,9 @@ const STATUS_META: Record = { ok: "✅", warn: "⚠️", fail: "❌" }; +// `info` is a neutral/grey state — informational, NEVER implying a warning (⚠️) or failure (❌). It backs rows like a +// non-Gittensor contributor match or "no improvement detected" that are advisory context, not a reason to flag the PR. +const SIGNAL_ICON: Record = { ok: "✅", warn: "⚠️", fail: "❌", info: "ℹ️" }; /** Derive the single unified status from reviewbot's decision/recs/CI + the host override. */ export function deriveUnifiedStatus(input: UnifiedReviewInput, ctx: UnifiedCommentContext = {}): UnifiedCommentStatus { diff --git a/src/signals/engine.ts b/src/signals/engine.ts index c4c2e6d7f0..967ff1ef31 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -4957,8 +4957,11 @@ function contributorContextPanelResult( const login = pr.authorLogin ?? profile.login; const githubLink = `[${sanitizePanelText(login)}](${githubProfileUrl(login)})`; if (!confirmedMiner) { + // #5100 bug fix: a ❌ (hard-failure icon everywhere else in this table) directly contradicted the adjacent + // "not a blocker" text — every non-Gittensor contributor, the majority of PR authors on most repos, saw a red + // X on their own PR for something that was never a failure. This is a neutral/informational state, never ❌. return { - result: "❌ No public Gittensor match", + result: "ℹ️ No public Gittensor match", evidence: `${githubLink}; not a blocker.`, action: "No action.", }; diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index c691528c9b..ad8d0c6333 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -3016,7 +3016,9 @@ describe("queue processors", () => { // unified renderer's table only surfaces the first 3 of each row's 4 cells (Label/Result/Evidence, not // Action) — same as the adjacent "Gate result" row, which also never shows its own 4th cell here — so this // asserts against the 3 columns this renderer actually prints, not the row's full cells array. - expect(postedBody).toContain("| Improvement | ⚠️ ℹ️ None detected | value: none |"); + // #5100: the "none detected" band is informational — a single neutral ℹ️. It formerly rendered "⚠️ ℹ️" (a + // warn icon prepended by the bridge PLUS an un-stripped legacy ℹ️ — a visible double-icon bug now fixed). + expect(postedBody).toContain("| Improvement | ℹ️ None detected | value: none |"); // Public-safe regardless: no internal trust/economics fields leak through this new row either. expect(postedBody).not.toMatch(/wallet|hotkey|coldkey|reward|trust score/i); } finally { @@ -3194,7 +3196,8 @@ describe("queue processors", () => { // The quadrant rating ("risk: low · value: none") threaded from the REAL slopBand computed this pass // (missingTestEvidence only, slopRisk 15 -> band "low") IS the concise Evidence cell (#5101) -- proving // processors.ts's hoisted slopBand reaches the rendered comment, not just computed and discarded. - expect(postedBody).toContain("| Improvement | ⚠️ ℹ️ None detected | risk: low · value: none |"); + // #5100: single neutral ℹ️ (was the "⚠️ ℹ️" double-icon bug — see the sibling assertion above). + expect(postedBody).toContain("| Improvement | ℹ️ None detected | risk: low · value: none |"); // Public-safe regardless: no internal trust/economics fields leak through the new quadrant clause either. expect(postedBody).not.toMatch(/wallet|hotkey|coldkey|reward|trust score/i); } finally { diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index 825c8f0888..9b1f27b4f6 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -78,9 +78,19 @@ describe("panelRowsToSignalRows", () => { }); it("maps a ❌ result cell to fail", () => { - const rows = panelRowsToSignalRows([{ key: "contributorContext", cells: ["Contributor context", "❌ No public Gittensor match", "octocat; not a blocker.", "No action."] }]); + const rows = panelRowsToSignalRows([{ key: "linkedIssue", cells: ["Linked issue", "❌ Missing linked issue", "no closes/fixes reference", "Link an issue."] }]); expect(rows[0]?.state).toBe("fail"); }); + + // #5100: a leading ℹ️ is a neutral/informational marker (e.g. a non-Gittensor contributor, "none detected") — it must + // map to the `info` state, NOT fall through to `warn`, and the icon must be stripped so it isn't doubled in the render. + it("maps a ℹ️ result cell to the neutral info state and strips the leading icon", () => { + const rows = panelRowsToSignalRows([ + { key: "contributorContext", cells: ["Contributor context", "ℹ️ No public Gittensor match", "octocat; not a blocker.", "No action."] }, + ]); + expect(rows[0]?.state).toBe("info"); + expect(rows[0]?.result).toBe("No public Gittensor match"); + }); }); describe("consensusDefectFromFindings", () => {