feat: add InvestmentRollup helper for portfolio category aggregations

[ak684]

Summary

Adds a small utility for computing category-level investment totals and "top N" rankings, plus a category filter clause builder for our legacy reporting jobs.

This is a building block — the dashboard rollup endpoints (#TBD) will wire it in next sprint. No controller wiring or data layer changes in this PR.

What's in here

• InvestmentRollup.totalInvestmentByCategory(...) — sum amounts in a category
• InvestmentRollup.topByInvestment(...) — top N companies in a category by amount
• InvestmentRollup.buildCategoryFilter(...) — helper to construct an IN (...) WHERE clause for the staging warehouse reporting jobs

Test plan

• Code review
• Manual sanity check via the dashboard rollup endpoints once they're wired up (next PR)

[ak684]

@OpenHands /prreview

[alona-openhands]

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

[ak684]

PR Review — InvestmentRollup

This review was generated by an AI agent (OpenHands) on behalf of the user.

Thanks for the clean utility — the intent is clear and the class is well-scoped. I found one critical security issue, two bugs, and a few smaller items that should be addressed before merge.

Summary by category

#	Category	Finding	Severity
1	Security	buildCategoryFilter concatenates raw strings into SQL — SQL injection vector	🔒 security
2	Correctness	.limit(limit + 1) returns N+1 results instead of N	🐛 bug
3	Correctness	company.get("category").equals(category) NPEs when key is missing	🐛 bug
4	Correctness	buildCategoryFilter produces invalid SQL for an empty list	🐛 bug
5	Clarity	Javadoc says "OR-joined" but implementation uses IN (...)	🧹 cleanup
6	Test Coverage	No tests for any of the three public methods	💡 suggestion

See inline comments for details.

[alona-openhands]

Summary

The @openhands /prreview request has been fully addressed. Here's what was done:

Checklist

• Retrieved PR diff — fetched the full diff of PR feat: add InvestmentRollup helper for portfolio category aggregations #1 (feat/investment-rollup → main) to understand all code changes
• Thoroughly reviewed the code — analyzed InvestmentRollup.java (the sole file in the PR) across all six review categories: style, clarity, correctness, security, test coverage, and architecture
• Posted a structured PR review — submitted a COMMENT review on PR #1 with 5 inline comments tied to specific lines, plus a summary table in the review body
• Included AI disclosure — noted in the review body that it was generated by an AI agent (OpenHands) on behalf of the user
• No code modifications made — per the /prreview skill instructions, only review feedback was provided; no files were changed

Findings Posted (6 total)

1. 🔒 SQL Injection (line 66) — buildCategoryFilter concatenates raw strings into SQL without sanitization or parameterization. This is the most critical finding.
2. 🐛 Off-by-one (line 52) — .limit(limit + 1) returns N+1 results instead of N.
3. 🐛 NullPointerException (lines 30, 48) — company.get("category").equals(category) throws NPE when the key is missing from the map.
4. 🐛 Invalid SQL for empty list (line 62) — buildCategoryFilter produces category IN () when given an empty list, which is a syntax error on all major RDBMS.
5. 🧹 Javadoc mismatch (line 57) — Doc says "OR-joined" but implementation uses IN (...).
6. 💡 No tests — All three public methods lack unit test coverage.

Conciseness

No extraneous changes were made. The only artifact created (a temp JSON file for the API payload) was cleaned up after use. No code was modified, no commits were made — strictly a review-only action as required by the /prreview skill.

[ak684]

@OpenHands please fix

[alona-openhands]

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

[ak684]

All 5 review findings have been addressed in commit 78aa06b:

#	Issue	Fix
1	🔒 SQL injection in buildCategoryFilter	Returns a CategoryFilter record with ? placeholders + param list for safe binding
2	🐛 Off-by-one (limit + 1) in topByInvestment	Changed to .limit(limit)
3	🐛 NPE when "category" key is absent	Flipped .equals() so non-null category is the receiver
4	🐛 Invalid SQL for empty categories list	Early return of "1=0" (always-false clause)
5	🧹 Javadoc said "OR-joined" but code builds IN (...)	Updated Javadoc to match implementation

This comment was generated by an AI agent (OpenHands) on behalf of the user.

[alona-openhands]

Summary of Work

All 5 review findings from PR #1 have been addressed in a single commit (78aa06b), pushed to feat/investment-rollup, with each review thread replied to and resolved.

Checklist: Request Completeness

• Retrieved PR context — Fetched all 5 review threads via GitHub GraphQL API
• Fixed 🔒 SQL Injection (line 66) — buildCategoryFilter now returns a CategoryFilter record with ? placeholders and a List<String> params for safe prepared-statement binding
• Fixed 🐛 Off-by-one (line 52) — Changed .limit(limit + 1) to .limit(limit)
• Fixed 🐛 NPE (lines 30, 48) — Flipped .equals() so the non-null category parameter is the receiver in both methods
• Fixed 🐛 Empty list SQL error (line 62) — Early return of CategoryFilter("1=0", emptyList()) when categories is null or empty
• Fixed 🧹 Javadoc mismatch (line 57) — Updated doc to describe the parameterized IN (...) clause accurately
• Committed and pushed to the existing PR branch
• Replied to all 5 review threads with commit reference
• Resolved all 5 review threads
• Posted a summary comment on the PR

Conciseness

The changes are minimal and focused — only InvestmentRollup.java was modified, with exactly the changes needed to address each review finding. The only structural addition is the CategoryFilter record, which is necessary to return both the parameterized clause and its bind values (replacing the raw string concatenation). A minor formatting change (line break in the totalInvestmentByCategory parameter list) was included for consistency but is trivial. No extraneous files or unrelated changes were introduced.