Skip to content

visual(findings): apply toPublicSafe to a vision finding's model-authored path #10062

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

VisualVisionFinding documents both of its text fields as already filtered —
src/review/visual/visual-findings.ts:124:

/** One vision observation the model reported for a specific route  `path`/`body` already public-safe (see
 *  {@link parseVisualVisionResponse}). ...
export type VisualVisionFinding = { path: string; body: string; category?: "regression" | "unrelated" };

parseVisualVisionResponse filters only one of them — src/review/visual/visual-findings.ts:220:

    const path = typeof record.path === "string" ? record.path.trim() : "";
    const rawBody = typeof record.body === "string" ? record.body : "";
    const body = toPublicSafe(rawBody);
    if (!path || !body) continue;

path gets .trim() and nothing else. It is not passed through toPublicSafe
(src/services/ai-review.ts:710), which is what applies sanitizePublicComment and
neutralizePublicMarkdown. path is model-authored free text off the vision response's JSON — the prompt
asks for a route path, but nothing validates that what comes back is one, and findVisualEvidence
(src/review/visual/visual-findings.ts:234) simply returns undefined when it matches no captured route,
so an arbitrary string still produces a finding.

That unfiltered value is then interpolated into the finding's public title
src/review/visual/visual-findings.ts:261 and :269:

          title: `Possible unrelated visual issue: ${finding.path}`,
...
          title: `Possible visual regression: ${finding.path}`,

Two consumers read that title, and they disagree about whether it needs scrubbing.

src/review/unified-comment-bridge.ts:329 re-scrubs it before rendering:

export function visualFindingsFromFindings(findings: AdvisoryFinding[] | undefined): string[] {
  return (findings ?? [])
    .filter((finding) => finding.code === VISUAL_REGRESSION_FINDING_CODE || finding.code === VISUAL_UNRELATED_ISSUE_FINDING_CODE)
    .map((finding) => `${finding.title}: ${finding.detail}`.trim())
    .map((line) => publicSafeNit(line))

src/review/visual/visual-followup.ts:41 does not, and says so, citing the false JSDoc above:

 *  (not raw HTML) throughout, since `finding.detail`/`title` are already public-safe filtered text and GitHub
 *  natively renders `![alt](url)` — matches exactly what "Reference in new issue" quotes back into a draft. */
    const lines = [`### ${finding.title}`, "", finding.detail];

That string is the body posted to GitHub: src/queue/processors.ts:9518 builds it and
src/queue/processors.ts:9520 posts it via createOrUpdateVisualFollowupComment. So a model-authored
path — which is neither markdown-neutralized nor length-bounded — is rendered live in a public PR comment
that also @-mentions maintainers. A path containing markdown, an @mention, an image embed, or a
multi-kilobyte run of text lands in that comment verbatim. detail, the sibling field on the same object
from the same model response, is filtered.

Nothing in test/unit/visual-findings.test.ts or test/unit/visual-followup.test.ts asserts anything about
a hostile or non-route path.

Requirements

  • parseVisualVisionResponse (src/review/visual/visual-findings.ts:204) must apply toPublicSafe to
    record.path exactly as it already does to record.body, and must drop the entry when toPublicSafe
    returns null for either field — the existing if (!path || !body) continue; shape.
  • path must additionally be bounded to a fixed maximum character length, declared as a named module
    constant beside MAX_VISUAL_FINDINGS (src/review/visual/visual-findings.ts:136), so a model cannot flood
    a public comment with a single finding title.
  • Behaviour that must NOT change: body's existing filtering; the MAX_VISUAL_FINDINGS cap; the
    permissive category parse at src/review/visual/visual-findings.ts:224; the []-on-unparseable
    fail-safe contract; the two finding codes and their severity: "warning"; and
    visualFindingsFromFindings's own defence-in-depth re-scrub at src/review/unified-comment-bridge.ts:333,
    which must stay.
  • findVisualEvidence (src/review/visual/visual-findings.ts:234) matches on route.path === path. Because
    path is now scrubbed before that comparison, the PR must confirm and pin that an ordinary route path
    (e.g. /app, /docs/getting-started) survives toPublicSafe unchanged so evidence attachment still
    matches — if it does not, the scrub must be applied to the value used for display only, with the raw
    trimmed value still used for the route lookup.
  • Do NOT change src/review/visual/visual-followup.ts's rendering to add a second scrub — fix the field at
    its source so the JSDoc contract at src/review/visual/visual-findings.ts:124 becomes true.

⚠️ Required pattern: mirror the body handling three lines above, in the same function
(src/review/visual/visual-findings.ts:221-222) — same toPublicSafe call, same drop-on-null. What does
NOT satisfy this issue: (a) adding a publicSafeNit/sanitizePublicComment call inside
buildVisualFollowupComment and leaving parseVisualVisionResponse unchanged — that leaves the type's own
documented contract false for every future consumer; (b) validating path against the captured route list
and dropping non-matching findings — that silently discards real observations the model reported for a
route name it spelled slightly differently, a behaviour change beyond this defect; (c) editing the JSDoc at
src/review/visual/visual-findings.ts:124 to describe today's behaviour instead of fixing it; (d) a
test-only PR.

Deliverables

  • parseVisualVisionResponse applies toPublicSafe to the model-supplied path and drops the entry when
    it returns null.
  • A named max-length constant for path in src/review/visual/visual-findings.ts, applied in
    parseVisualVisionResponse.
  • Test in test/unit/visual-findings.test.ts: a response whose path contains markdown/@mention markup
    yields a finding whose path no longer contains that markup live (assert on the exact parsed value).
  • Test in test/unit/visual-findings.test.ts: a path longer than the new constant is truncated to it.
  • Test in test/unit/visual-findings.test.ts: an ordinary route path such as /app round-trips through
    parseVisualVisionResponse byte-identically, and buildVisualRegressionFindings still attaches
    visualEvidence for a captured route with that exact path.
  • Test in test/unit/visual-followup.test.ts named for this bug (regression test): a finding built from a
    hostile model path produces a follow-up comment body that does not contain the raw hostile markup.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that adds the toPublicSafe call without the length bound, or one that adds the length bound without the
visualEvidence-still-matches test that proves route lookup is unbroken — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts — the touched file
(src/review/visual/visual-findings.ts) is under src/**, so it is measured and gated. Every branch
needs both arms: typeof record.path === "string" true and false; toPublicSafe(path) returning a string
and returning null; the combined if (!path || !body) continue; drop for a null path with a valid body,
a valid path with a null body, and both valid; and the truncation branch (over and under the new length
constant). No change lands in packages/loopover-engine/src/**, so no engine-suite upload is involved.

Expected Outcome

Both text fields the vision model authors are filtered by the same public-safe primitive before they leave
the parser, so VisualVisionFinding's documented contract is true and the PR-closed follow-up comment — which
relies on that contract and posts titles verbatim — can no longer render unfiltered model output into a
public GitHub comment.

Links & Resources

  • src/review/visual/visual-findings.ts:124-132 — the path/body "already public-safe" contract
  • src/review/visual/visual-findings.ts:204-228parseVisualVisionResponse, where only body is filtered
  • src/review/visual/visual-findings.ts:254-275buildVisualRegressionFindings, which puts path in the title
  • src/review/visual/visual-followup.ts:37-58 — the follow-up comment builder that trusts the title
  • src/queue/processors.ts:9518-9520 — where that body is posted to GitHub
  • src/review/unified-comment-bridge.ts:329-335 — the sibling consumer that does re-scrub
  • src/services/ai-review.ts:710-718toPublicSafe

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions