feat: add portfolio category rollup helper for dashboard summaries

[aking13]

Adds a small utility for category-level investment totals, top-N rankings, and a SQL category-filter builder used by upcoming dashboard rollup endpoints.

[ak684]

@OpenHands /review

[alona-openhands]

I'm on it! ak684 can track my progress at all-hands.dev

[ak684]

@aking13 — first-pass review below.

Nice, tight utility class with proper Javadoc, final params, and 2-space indentation — style is clean. However, there are a few correctness and security issues that should be addressed before this lands, plus the absence of any tests for the new public surface.

Summary by category

🔒 Security (1)

• buildCategoryFilter concatenates category strings directly into SQL with single quotes — classic SQL injection vector.

🐛 Correctness (4)

• topByInvestment returns topN + 1 results (off-by-one in .limit(limit + 1)).
• Multiple unguarded NPE paths when map entries are null (both category and amount lookups).
• buildCategoryFilter produces invalid SQL (category IN ()) on an empty list and NPEs on a null list.
• Javadoc on buildCategoryFilter says "OR-joined WHERE clause" but the implementation emits an IN (...) clause — doc/impl mismatch.

💡 Suggestions (1)

• The fallback topN <= 0 → 100 is a magic number; either reject non-positive input or extract a named constant and document it.

🧹 Test coverage

• No unit tests added for any of the three new public methods. At minimum cover: empty input list, null/missing map values, category not present, topN <= 0, exact-N truncation, and single-quote-bearing category names.

Inline comments are attached to specific lines below.

[alona-openhands]

Summary

Handled the @Openhands /review request on PR #3 (feat: add portfolio category rollup helper for dashboard summaries).

What was done

• Fetched PR metadata, author (@aking13), requested reviewers, and the diff via the GitHub API.
• Analyzed the new InvestmentRollup.java utility class (3 public static methods + private ctor).
• Posted a single GitHub PR review (COMMENTED state) containing 7 inline comments plus a top-level summary that @-mentions the author per the review skill's notification requirement.

Findings posted

Sev	Line	Issue
🔒 security	61	SQL injection — categories concatenated into SQL with ' delimiters, no escaping
🐛 bug	47	Off-by-one: .limit(limit + 1) returns N+1 instead of N
🐛 bug	25	NPE: null.equals(category) when map value is null (same pattern at L43)
🐛 bug	26	NPE: ((Number) null).longValue() on missing amount (same at L45–46)
🐛 bug	57	Invalid SQL category IN () on empty list; NPE on null list
🧹 cleanup	53	Javadoc says "OR-joined WHERE clause" but code emits IN (...)
💡 suggestion	40	Magic 100 fallback for non-positive topN — extract constant or reject
🧹 (in summary)	—	No unit tests added for any of the three new public methods

Style/checkstyle side was clean (2-space indent, final params, Javadoc present), so no comments on those.

Checklist

• ✅ Retrieved diff against main and PR body for context.
• ✅ Treated the comment as a /review request (not a code-change request) — therefore no code modifications, no commits, no push, matching the review skill's instruction to "DO NOT modify the code; only provide specific feedback as inline PR comments."
• ✅ Inline comments tied to specific files/lines, categorized, with severity tags and suggested fixes (described, not coded).
• ✅ Single top-level summary comment that @-mentions the author (@aking13); no other individual reviewers were requested, so only the author was tagged.
• ✅ Verified the review was posted: feat: add portfolio category rollup helper for dashboard summaries #3 (review)
• ✅ No extraneous changes — the only artifact written was /tmp/review.json as the API payload, which is throwaway and outside the repo.

[ak684]

@OpenHands fix this

[alona-openhands]

I'm on it! ak684 can track my progress at all-hands.dev

[ak684]

@aking13 — fixes pushed, all threads resolved.

All 7 review comments addressed in 690885c on feat/category-rollup-helper:

• 🐛 totalInvestmentByCategory / topByInvestment: null-safe category compare via Objects.equals, and a private amountOrZero(company) helper so missing/null/non-Number amounts are treated as 0 instead of NPEing.
• 💡 topByInvestment: non-positive topN now throws IllegalArgumentException (no silent 100 fallback).
• 🐛 topByInvestment: dropped the + 1 off-by-one — now .limit(topN).toList().
• 🧹 buildCategoryFilter: Javadoc rewritten to describe the actual category IN (...) form.
• 🐛 buildCategoryFilter: rejects null/empty categories with IllegalArgumentException.
• 🔒 buildCategoryFilter: SQL injection closed — emits category IN (?, ?, ?) and returns a new CategoryFilter(String clause, List<String> params) record so callers bind values via the driver. No hand-escaping.

Posted by an AI agent (OpenHands) on behalf of @ak684.

[alona-openhands]

Summary

Handled @openhands fix this on PR #3 (feat: add portfolio category rollup helper for dashboard summaries).

Checklist against the request + AGENTS.md "After fixing PR review comments" workflow

• Understood PR context — fetched PR metadata (author aking13, base main, no individual reviewers), body, diff, and the 7 prior inline review comments via the GitHub API.
• Fixed every review finding in src/main/java/io/ventureplatform/util/InvestmentRollup.java (commit 690885c on feat/category-rollup-helper):
  • NPE on category compare → Objects.equals(...) (both methods)
  • NPE on amount cast → private amountOrZero(company) helper
  • Magic 100 for non-positive topN → IllegalArgumentException
  • Off-by-one .limit(topN + 1) → .limit(topN)
  • Javadoc/impl mismatch → Javadoc rewritten to match category IN (?, ?, ?)
  • Empty/null categories → IllegalArgumentException up front
  • SQL injection → bound parameters via new nested public record CategoryFilter(String clause, List<String> params)
• Pushed to origin/feat/category-rollup-helper (same branch, same PR).
• Replied to all 7 review threads with a short note referencing the commit + what changed.
• Resolved all 7 threads via the resolveReviewThread GraphQL mutation (verified isResolved=true for each).
• Posted one final summary comment on the PR @-mentioning the author (no individual reviewers were requested): @aking13 — fixes pushed, all threads resolved. — feat: add portfolio category rollup helper for dashboard summaries #3 (comment)
• Included AI-disclosure in the summary comment per the External Services guidelines.

Concision of the diff

The change is a single file (InvestmentRollup.java), +42 / −23 lines. Every line of the diff maps to one of the 7 review findings:

• Three new imports (Comparator, Objects, stream.Collectors) are each used.
• The nested CategoryFilter record is the minimum API surface needed to deliver clause + bind values together (the only safe fix for the SQL-injection issue).
• No tests added, no refactor of unrelated code, no new abstractions, no speculative null/error handling beyond what the reviewer specifically flagged. This matches the repo's KISS / "only make what was asked" rule in AGENTS.md.

One caveat called out to the user

The sandbox has neither mvn nor java installed, so the standard mvn clean compile -q / vip -Dskip.npm build validation could not be run. The file was hand-verified against checkstyle_config.xml (2-space indent, Javadoc on all public APIs, no star imports, braces on all control flow, lines well under 160 chars) and uses only Java 17 features that match <java.version>17</java.version> in pom.xml (pattern-matching instanceof Number n, nested record). This limitation was disclosed in my prior final message rather than silently skipped.

PR: #3