Repository navigation
feat(account): portfolio composition report - #28
Conversation
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: fc8d1acddaa2
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| rust:implementation | 0 |
| rust:implementation-tests | 0 |
| policies:conventions | 0 |
| architecture:solid | 2 |
architecture:solid (2 findings)
Minor - src/commands/account.rs:152
U-S2: the
Portfolioarm repeats the core's rule for what counts as a holding (currentTokens > 0, a presentpropertyId) to decide which listings to fetch, andportfolio::portfolioapplies that rule again on its own (portfolio.rs:78-86). The two copies already differ. The shell skips a position with nopropertyId, but the core keeps it as a row with id""underunknown. A later change to either rule (for example, treating dust balances as sold) will fetch one set of listings and weight a different set of rows. Suggested fix: exposepub fn held_property_ids(positions: &[Value]) -> Vec<&str>(or aheld_positionsiterator) from portfolio.rs. Use it in both the fetch loop andportfolio(), so the shell only translates input and calls the core, and the holding rule lives in one tested place.
Minor - src/commands/portfolio.rs:20
U-O1 / U-L2: rent status is a closed domain set (
renting,vacant,delinquent,no-cash-flow,unknown) carried as&'static str, and behaviour branches on string equality in three separate places: the daily-rent guard (line 94), the renting filter (line 113), and the per-propertyrenting/rentSharePctdecision (line 154). A typo or rename in any one comparison compiles cleanly and silently zeroes rent or miscounts renting properties. Idiomatic Rust gives this an exhaustive type. Suggested fix:enum RentStatus { Renting, Vacant, Delinquent, NoCashFlow, Unknown }withfn is_renting(self) -> boolandfn as_str(self) -> &'static str(or#[derive(Serialize)] #[serde(rename_all = "kebab-case")]). Store it inRowand serialize only at the JSON boundary, so theaccount-portfolio/v1output stays the same. Low blast radius because the type is module-private, so not blocking.
Reviewer Coverage
rust:implementation— complete (broad); inspected 3 assigned files (10 inspected across reviewers):src/commands/account.rs,src/commands/mod.rs,src/commands/portfolio.rs; skipped: none; constraints: nonerust:implementation-tests— complete (broad); inspected 5 assigned files (10 inspected across reviewers):src/commands/portfolio.rs,tests/cli_surface.rs,tests/fixture_shapes.rs,tests/fixtures/account/positions-portfolio.json,tests/fixtures/portfolio/property-listings.json; skipped: none; constraints: nonepolicies:conventions— complete (broad); inspected 5 assigned files (10 inspected across reviewers):AGENTS.md,README.md,src/commands/account.rs,src/commands/mod.rs,src/commands/portfolio.rs; skipped: none; constraints: No sibling cli-common/.github checkouts were present in this workbench, so shared Open CLI Collective docs could not be checked; relied on repo-local AGENTS.md/README.md as the only available convention source.architecture:solid— complete (broad); inspected 4 assigned files (10 inspected across reviewers):src/commands/account.rs,src/commands/mod.rs,src/commands/portfolio.rs,tests/fixtures/README.md; skipped: none; constraints: Could not runmake verify, clippy, or cargo test: shell commands in the checkout required interactive approval that this non-interactive session cannot grant. Build and test status is taken from the PR description, not reproduced. git diff was unavailable for the same reason; changed hunks were identified by reading head files against the change map and PR description.
Inspected files (10)
AGENTS.mdREADME.mdsrc/commands/account.rssrc/commands/mod.rssrc/commands/portfolio.rstests/cli_surface.rstests/fixture_shapes.rstests/fixtures/README.mdtests/fixtures/account/positions-portfolio.jsontests/fixtures/portfolio/property-listings.json
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 19s | $2.35 | claude-sonnet-5, claude-opus-5-5 | cr 0.10.314
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5-5 |
| Reviewers | rust:implementation, rust:implementation-tests, policies:conventions, architecture:solid |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 2m 19s wall · 5m 05s compute |
| Cost | $2.35 |
| Tokens | 92 in / 22.1k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 6
- Out: 2.2k
- Cache read: 139.9k
- Cache create: 17.0k
- Cost: $0.12
- Duration: 28s
rust:implementation— claude-sonnet-5- In: 12
- Out: 3.3k
- Cache read: 410.9k
- Cache create: 46.4k
- Cost: $0.30
- Duration: 44s
rust:implementation-tests— claude-sonnet-5- In: 24
- Out: 4.6k
- Cache read: 697.8k
- Cache create: 33.7k
- Cost: $0.32
- Duration: 59s
policies:conventions— claude-sonnet-5- In: 30
- Out: 6.6k
- Cache read: 1.0M
- Cache create: 60.4k
- Cost: $0.51
- Duration: 1m 36s
architecture:solid— claude-opus-5-5- In: 14
- Out: 4.9k
- Cache read: 363.4k
- Cache create: 85.8k
- Cost: $0.86
- Duration: 1m 04s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 457
- Cache read: 158.0k
- Cache create: 21.6k
- Cost: $0.24
- Duration: 11s
`lofty account portfolio` weights holdings by estimated value across states, cities, and properties, and by rent: each property's share of the projected daily rent, plus the share of properties renting and the share of holdings value that is rent-earning. - Value is each position's `currentValue` (tokens x current price). - Daily rent is the listing's `projected_annual_cash_flow` per issued token, times tokens held, over 365. `monthly_rent` is not used: it reads 0 on occupied, distributing properties. - A property is renting unless its listing is vacant, delinquent, or projects no cash flow. Non-paying properties are excluded from the rent weighting (null share), so renting shares sum to 100%. - A held property whose listing 404s is still weighted by value under an `unknown` location instead of being dropped. Read-only. `--json` emits `account-portfolio/v1`. Pure report logic lives in `commands/portfolio.rs`, tested against synthetic fixtures.
Review follow-up. `portfolio::held` is now the single definition of a holding (tokens > 0 and a propertyId); the command uses it to pick which listings to fetch and the report uses it to pick which rows to weight. Rent status is an enum instead of compared strings; the wire names in account-portfolio/v1 are unchanged.
21c18e8 to
3b2e33a
Compare
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 3b2e33a1e2e1
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| rust:implementation | 0 |
| rust:implementation-tests | 0 |
| policies:conventions | 0 |
| architecture:solid | 0 |
Reviewer Coverage
rust:implementation— complete (constrained); inspected 3 assigned files (10 inspected across reviewers):src/commands/account.rs,src/commands/mod.rs,src/commands/portfolio.rs; skipped: none; constraints: nonerust:implementation-tests— complete (constrained); inspected 5 assigned files (10 inspected across reviewers):src/commands/portfolio.rs,tests/cli_surface.rs,tests/fixture_shapes.rs,tests/fixtures/account/positions-portfolio.json,tests/fixtures/portfolio/property-listings.json; skipped: none; constraints: nonepolicies:conventions— complete (constrained); inspected 5 assigned files (10 inspected across reviewers):AGENTS.md,README.md,src/commands/account.rs,src/commands/mod.rs,src/commands/portfolio.rs; skipped: none; constraints: No sibling cli-common/.github checkouts were present in this workbench, so shared Open CLI Collective docs could not be checked; relied on repo-local AGENTS.md/README.md as the only available convention source.architecture:solid— complete (constrained); inspected 4 assigned files (10 inspected across reviewers):src/commands/account.rs,src/commands/mod.rs,src/commands/portfolio.rs,tests/fixtures/README.md; skipped: none; constraints: Both settled threads were re-checked against head 3b2e33a. The RentStatus enum (portfolio.rs:15) and the shared held() helper (portfolio.rs:74, used at account.rs:151) resolve them, so neither is re-raised. Did not run make verify, clippy, or cargo test: shell commands in the checkout need interactive approval, which this session lacks. Build and test status comes from the PR description, not from a reproduction.
Inspected files (10)
AGENTS.mdREADME.mdsrc/commands/account.rssrc/commands/mod.rssrc/commands/portfolio.rstests/cli_surface.rstests/fixture_shapes.rstests/fixtures/README.mdtests/fixtures/account/positions-portfolio.jsontests/fixtures/portfolio/property-listings.json
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 56s | $3.30 | claude-sonnet-5, claude-opus-5-5 | cr 0.10.314
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5-5 |
| Reviewers | rust:implementation, rust:implementation-tests, policies:conventions, architecture:solid |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 56s wall · 2m 19s compute |
| Cost | $3.30 |
| Tokens | 54 in / 8.4k out |
Per-workstream usage
rust:implementation— claude-sonnet-5- In: 8
- Out: 1.1k
- Cache read: 390.7k
- Cache create: 19.4k
- Cost: $0.47
- Duration: 14s
rust:implementation-tests— claude-sonnet-5- In: 14
- Out: 2.4k
- Cache read: 622.6k
- Cache create: 20.4k
- Cost: $0.55
- Duration: 40s
policies:conventions— claude-sonnet-5- In: 16
- Out: 2.5k
- Cache read: 932.1k
- Cache create: 21.8k
- Cost: $0.81
- Duration: 38s
architecture:solid— claude-opus-5-5- In: 10
- Out: 2.0k
- Cache read: 482.1k
- Cache create: 22.9k
- Cost: $1.18
- Duration: 33s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 371
- Cache read: 187.7k
- Cache create: 3.5k
- Cost: $0.30
- Duration: 12s
What
New read-only command
lofty account portfolio(--json→account-portfolio/v1). It answers "where is my money, and what pays rent?":How the numbers are derived
currentValuefrom/account/positions(tokens × current price; verified equal on live data). Fully sold positions (0 tokens) are skipped.projected_annual_cash_flow÷ issuedtokens× tokens held ÷ 365.monthly_rentis not used because it reads0on occupied, distributing properties.is_occupied: false(vacant),is_delinquent: true, or projects no cash flow.rentStatusreports which.unknownlocation (with a stderr note), so the other shares are not inflated.Inputs: one
/account/positionscall plus one/properties/{id}per held property. No writes.Tests
src/commands/portfolio.rsover new synthetic fixtures (tests/fixtures/account/positions-portfolio.json,tests/fixtures/portfolio/property-listings.json): state/city/property weights, rent-share exclusion, renting share by count and value, missing listing, zero cash flow, empty portfolio.city,state,tokens,projected_annual_cash_flow,is_occupied,is_delinquent) against the capturedproperty-get.json.account portfolio --helprenders.make verifygreen locally. Run once against the live API with the installed key: exit 0, schema and field types as documented.