Skip to content

fix(review): wire category tally into unified comments#3969

Merged
JSONbored merged 1 commit into
mainfrom
codex/fix-missing-category-tally-in-comments
Jul 7, 2026
Merged

fix(review): wire category tally into unified comments#3969
JSONbored merged 1 commit into
mainfrom
codex/fix-missing-category-tally-in-comments

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The renderer emits the compact category-tally line only when UnifiedReviewInput.inlineFindings is present, but the live bridge built the renderer input via buildUnifiedReviewInput without supplying inlineFindings, so production unified comments omitted the new one-line tally.
  • Bridge-side category data already exists as findingCategories, so the live path should reuse that data to enable the tally without changing rendering semantics.

Description

  • Add an optional inlineFindings option to buildUnifiedReviewInput and preserve it on the returned UnifiedReviewInput so renderers can read line-anchored finding categories. (file: src/review/unified-comment.ts)
  • Wire the bridge to pass args.findingCategories into buildUnifiedReviewInput as inlineFindings when present so the live unified comment path can render the compact category tally. (file: src/review/unified-comment-bridge.ts)
  • Update unit test expectations to assert that the live buildUnifiedCommentBody produces both the one-line **Findings by category:** summary and the existing "Finding categories" collapsible when category data is supplied, and that both are absent when the input is missing or empty. (file: test/unit/finding-category-collapsible.test.ts)
  • Preserve byte-identical/flag-OFF parity: absent or empty category inputs continue to omit the new tally line and the collapsible.

Testing

  • Ran the focused unit tests with npx vitest run test/unit/finding-category-collapsible.test.ts test/unit/finding-category-tally.test.ts, and both test files passed.
  • Ran npm run typecheck and it completed successfully.
  • Ran git diff --check to validate whitespace/conflict hygiene and it passed.
  • npm audit --audit-level=moderate failed due to an external registry 403 Forbidden (audit endpoint), so dependency-audit could not be completed locally.
  • Attempted npm run test:coverage; the full coverage run did not complete in time in this environment due to long-running pre-existing queue-suite output and was interrupted, so a full unsharded coverage run should be verified before pushing to ensure patch-coverage requirements are met.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 7, 2026
@loopover-orb

loopover-orb Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-07 08:17:42 UTC

3 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This is a narrow, correct wiring fix: it adds an optional `inlineFindings` field to `buildUnifiedReviewInput`'s opts (spread only when defined) and threads `args.findingCategories` into it from the bridge, letting the renderer's existing compact category-tally logic activate on the live path exactly as it already does in tests. The change is minimal and additive (no existing spread entries removed or reordered), and the updated test assertions correctly check both the new one-line tally and the pre-existing collapsible appear together when data is present, and both are absent when data is missing or empty. The type `ReadonlyArray<{category?: UnifiedFindingCategory | undefined}>` is a narrowed inline shape rather than reusing the renderer's actual `inlineFindings` type, which is a minor duplication risk if that shape drifts.

Nits — 4 non-blocking
  • src/review/unified-comment.ts: the inline type `ReadonlyArray<{ category?: UnifiedFindingCategory | undefined }>` duplicates rather than reuses the renderer's actual `UnifiedReviewInput['inlineFindings']` type — consider importing/aliasing that type so the two can't silently diverge.
  • test/unit/finding-category-collapsible.test.ts: only the bridge-level `buildUnifiedCommentBody` path is asserted; consider adding a direct `buildUnifiedReviewInput` unit test confirming `inlineFindings` round-trips onto the returned object, since that's the actual new surface being added in unified-comment.ts.
  • Add a small direct test for `buildUnifiedReviewInput({ ..., inlineFindings })` asserting the field is preserved on the output, decoupled from the bridge/body-rendering test.
  • Point the new `inlineFindings` option type at the same type alias the renderer already uses for that field to avoid shape drift.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 51 registered-repo PR(s), 43 merged, 343 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 343 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 51 PR(s), 343 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 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.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.68%. Comparing base (b8a1186) to head (fe97772).
⚠️ Report is 18 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3969   +/-   ##
=======================================
  Coverage   93.68%   93.68%           
=======================================
  Files         373      373           
  Lines       34895    34897    +2     
  Branches    12768    12770    +2     
=======================================
+ Hits        32692    32694    +2     
  Misses       1584     1584           
  Partials      619      619           
Files with missing lines Coverage Δ
src/review/unified-comment-bridge.ts 99.52% <100.00%> (+<0.01%) ⬆️
src/review/unified-comment.ts 99.54% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 6bf98e1 into main Jul 7, 2026
10 checks passed
@JSONbored
JSONbored deleted the codex/fix-missing-category-tally-in-comments branch July 7, 2026 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

1 participant