feat(review): advisory-only AI-vision analysis of before/after visual captures - #4120
Merged
Merged
Conversation
Contributor
|
Superagent did not find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4120 +/- ##
==========================================
+ Coverage 93.65% 93.66% +0.01%
==========================================
Files 384 385 +1
Lines 35865 35930 +65
Branches 13160 13185 +25
==========================================
+ Hits 33589 33654 +65
Misses 1618 1618
Partials 658 658
🚀 New features to boost your workflow:
|
Contributor
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-08 02:49:14 UTC
⏸️ Suggested Action - Manual Review
Nits — 2 non-blocking
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
JSONbored
force-pushed
the
feat/ai-vision-visual-findings-4111
branch
from
July 8, 2026 02:26
32648c9 to
edc50ee
Compare
Add a text|image content-block union (AiContentBlock) to both AI message-construction paths -- selfhost/ai.ts::toMessages (the live production path) and every services/ai-review.ts content: call site, including the dual-AI tie-break judge, so a split visual verdict can receive the same screenshots the two reviewers saw instead of falling back to text-only reasoning. The HTTP providers (OpenAI-compatible, Anthropic) translate blocks to their native image shape; the subscription CLIs (claude-code/codex) degrade to text-only since they cannot consume inline image bytes through stdin. Add review/visual/visual-findings.ts: pure gating (pixel-diff threshold via the existing diff-overlay URL, submitter reputation, BYOK) plus prompt/response/finding-construction helpers for an advisory-only visual-regression finding. The finding rides the exact same advisory-findings pipeline ai_consensus_defect/ai_review_split use and is not one of the codes isConfiguredGateBlocker recognizes, so it can never become a gate blocker. Wire a new "Visual findings" collapsible into the unified PR comment, recovered from advisoryFindings the same way the consensus defect is recovered, and excluded from the generic Nits list so it renders exactly once.
JSONbored
force-pushed
the
feat/ai-vision-visual-findings-4111
branch
from
July 8, 2026 02:39
edc50ee to
784b2cd
Compare
4 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 27, 2026
Follows #9276, which fixed 8 files / 12 cases and left these 4 files / 5 cases still red on main (and therefore red on every open PR). Each diagnosed to its introducing commit; two are real source gaps, two are stale sibling tests a deliberate behavior change never updated. SOURCE fixes: - selfhost/metrics.ts: register two counters emitted since #9243 and #9245 but never added to DEFAULT_METRIC_META, so renderMetrics() emitted them as bare undocumented samples with no HELP/TYPE — exactly what that drift guard exists to catch. Both commits even name the counter in their own body text; a pure registration miss, not a behavior decision. TEST fixes (source verified correct in both cases): - selfhost-pg-retention: the fake Postgres still matched the pre-#9083 ctid semi-join, so every DELETE fell through to rowCount 0. #9083 deliberately moved retention to an index-backed PK range delete with ORDER BY (the fix for prune-retention blowing its timeout and stalling permanently); it updated the SQLite twin but not the Postgres one. Regex now captures whichever key column is in play, keeping BOTH the mapped-PK and ctid-fallback paths exercised. - salvageability: #9085 made an absent blocker confidence degrade to CONFIDENCE_WHEN_UNSTATED (0.5) instead of 1.0, so it is sub-floor against the 0.93 default and the low-confidence hold owns the case. It renamed both sibling assertions in rules.test.ts but missed this third consumption site. Both calls kept — they are the only coverage of the default-floor arm and the nullish-confidence arm respectively. - worker-entry-boundary: false positive, not a real dependency leak. The check grepped whole-file text, so #9230's user-facing string "crossed the visual-diff threshold" failed a green tree over a sentence, in a file worker-reachable since #4120 that imports none of these deps. Narrowed to scan module specifiers (reusing the file's own parseImportSpecifiers) — the only way a Node-only dep can actually reach the bundle. Added a discriminating test so it cannot pass vacuously; verified by injecting a real `import sharp` and confirming it fails, naming the file and specifier.
JSONbored
added a commit
that referenced
this pull request
Jul 27, 2026
) * fix: close the remaining 5 pre-existing test failures on main Follows #9276, which fixed 8 files / 12 cases and left these 4 files / 5 cases still red on main (and therefore red on every open PR). Each diagnosed to its introducing commit; two are real source gaps, two are stale sibling tests a deliberate behavior change never updated. SOURCE fixes: - selfhost/metrics.ts: register two counters emitted since #9243 and #9245 but never added to DEFAULT_METRIC_META, so renderMetrics() emitted them as bare undocumented samples with no HELP/TYPE — exactly what that drift guard exists to catch. Both commits even name the counter in their own body text; a pure registration miss, not a behavior decision. TEST fixes (source verified correct in both cases): - selfhost-pg-retention: the fake Postgres still matched the pre-#9083 ctid semi-join, so every DELETE fell through to rowCount 0. #9083 deliberately moved retention to an index-backed PK range delete with ORDER BY (the fix for prune-retention blowing its timeout and stalling permanently); it updated the SQLite twin but not the Postgres one. Regex now captures whichever key column is in play, keeping BOTH the mapped-PK and ctid-fallback paths exercised. - salvageability: #9085 made an absent blocker confidence degrade to CONFIDENCE_WHEN_UNSTATED (0.5) instead of 1.0, so it is sub-floor against the 0.93 default and the low-confidence hold owns the case. It renamed both sibling assertions in rules.test.ts but missed this third consumption site. Both calls kept — they are the only coverage of the default-floor arm and the nullish-confidence arm respectively. - worker-entry-boundary: false positive, not a real dependency leak. The check grepped whole-file text, so #9230's user-facing string "crossed the visual-diff threshold" failed a green tree over a sentence, in a file worker-reachable since #4120 that imports none of these deps. Narrowed to scan module specifiers (reusing the file's own parseImportSpecifiers) — the only way a Node-only dep can actually reach the bundle. Added a discriminating test so it cannot pass vacuously; verified by injecting a real `import sharp` and confirming it fails, naming the file and specifier. * fix(github): guard backfill GraphQL helpers' repoFullName parsing fetchLiveCiAggregateViaGraphQl, fetchLivePullRequestReviewDecision, and fetchLiveReviewThreadBlockers parsed repoFullName with a bare split/truthiness check, so extra segments and whitespace-padded slugs reached GraphQL queries. Add a local parseBackfillRepoFullName helper mirroring #8311's segment-count and whitespace guard, preserving each call site's fail-soft return contract. Closes #9317 --------- Co-authored-by: JSONbored <49853598+JSONbored@users.noreply.github.com>
JSONbored
added a commit
that referenced
this pull request
Jul 27, 2026
…locked (#9375) * fix: close the remaining 5 pre-existing test failures on main Follows #9276, which fixed 8 files / 12 cases and left these 4 files / 5 cases still red on main (and therefore red on every open PR). Each diagnosed to its introducing commit; two are real source gaps, two are stale sibling tests a deliberate behavior change never updated. SOURCE fixes: - selfhost/metrics.ts: register two counters emitted since #9243 and #9245 but never added to DEFAULT_METRIC_META, so renderMetrics() emitted them as bare undocumented samples with no HELP/TYPE — exactly what that drift guard exists to catch. Both commits even name the counter in their own body text; a pure registration miss, not a behavior decision. TEST fixes (source verified correct in both cases): - selfhost-pg-retention: the fake Postgres still matched the pre-#9083 ctid semi-join, so every DELETE fell through to rowCount 0. #9083 deliberately moved retention to an index-backed PK range delete with ORDER BY (the fix for prune-retention blowing its timeout and stalling permanently); it updated the SQLite twin but not the Postgres one. Regex now captures whichever key column is in play, keeping BOTH the mapped-PK and ctid-fallback paths exercised. - salvageability: #9085 made an absent blocker confidence degrade to CONFIDENCE_WHEN_UNSTATED (0.5) instead of 1.0, so it is sub-floor against the 0.93 default and the low-confidence hold owns the case. It renamed both sibling assertions in rules.test.ts but missed this third consumption site. Both calls kept — they are the only coverage of the default-floor arm and the nullish-confidence arm respectively. - worker-entry-boundary: false positive, not a real dependency leak. The check grepped whole-file text, so #9230's user-facing string "crossed the visual-diff threshold" failed a green tree over a sentence, in a file worker-reachable since #4120 that imports none of these deps. Narrowed to scan module specifiers (reusing the file's own parseImportSpecifiers) — the only way a Node-only dep can actually reach the bundle. Added a discriminating test so it cannot pass vacuously; verified by injecting a real `import sharp` and confirming it fails, naming the file and specifier. * fix(review): honor autoCloseExemptLogins in closeDraftDodgeAttemptIfBlocked closeDraftDodgeAttemptIfBlocked was the one remaining review-evasion auto-close guard that skipped isProtectedAutomationAuthor and isAutoCloseExempt, so authors on the operator allowlist or protected automation bots were still draft-dodge closed despite every sibling guard honoring those exemptions (#6165 pattern). Closes #9294 --------- Co-authored-by: JSONbored <49853598+JSONbored@users.noreply.github.com> Co-authored-by: loopover-orb[bot] <296761690+loopover-orb[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
text|imagecontent-block union to both AI-review message-construction paths — prioritizingsrc/selfhost/ai.ts(the live production path;src/services/ai-review.ts's own comments describe its Workers-AI-oriented path as retired) — so a reviewer call can attach the before/after visual-capture screenshots (Epic: extend automated visual-capture convergence (#577) to self-host + metagraphed #3607/Generic per-repo preview-URL provider config (Cloudflare/Vercel/Netlify/manual template) #3609/Generalize visual-capture route-mapping to be per-repo configurable #3610) alongside the normal text prompt.runDualAiTieBreakJudgeCall) — a split verdict between two reviewers now still gets the visual context, instead of silently falling back to text-only reasoning.src/review/visual/visual-findings.ts: a pure, advisory-onlyvisual_regression_finding-codedAdvisoryFindingshape. Deliberately not one of the codesisConfiguredGateBlocker(src/rules/advisory.ts) recognizes — a visual finding can never become a gate blocker, only a warning.src/review/unified-comment-bridge.ts: recoversvisual_regression_findingentries from the same advisory-findings arrayconsensusDefectFromFindingsalready reads, and renders them in their own "Visual findings" collapsible (ahead of "Visual preview") — excluded from the generic Nits collapsible so a finding never renders twice.screenshotTableGate) has landed.Scope
CONTRIBUTING.md.Validation
git diff --checknpm run typecheck— clean.npm run test:changed— 260 files / 6936 tests passed (2 pre-existing skips), includingtest/unit/visual-findings.test.ts(new),test/unit/unified-comment-bridge.test.ts(extended),test/unit/ai-review.test.ts,test/unit/selfhost-ai.test.ts.visual-findings.ts,unified-comment-bridge.ts,selfhost/ai.ts,services/ai-review.ts,types.ts) — 100% branch/line on every changed hunk; the small number of uncovered lines reported (bridge:77, ai.ts:1149, ai-review.ts:1280-1322) are pre-existing and outside this diff's hunks (confirmed viagit diff origin/main...HEADhunk ranges).npm run ui:openapi:check— clean (no schema drift).npm audit --audit-level=moderate— 0 vulnerabilities.Safety
publicSafeNitscrub boundary as every other bridge-recovered Nit string.ui:openapi:checkconfirms).visual_regression_findingis excluded fromisConfiguredGateBlocker, so this can never flip a merge/close decision — only ever adds a "Visual findings" collapsible to the comment.