Repository navigation
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. 6 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour. WalkthroughThe change adds 25 P&L metric series for six windows and refreshes them every five minutes. Each window is generated and replaced independently. Failed or skipped reports leave its previous samples and collector timestamp unchanged. Daily metrics aggregate normalized symbols, calculate cumulative values across the full range, and emit up to 90 days. The change also updates P&L parity rules, golden cases, tests, and observability documentation. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Some per-symbol P&L metrics can be absent, and the staging comparison can miss absent day values. Resolve or explicitly accept these gaps before merging. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 108 functions across 8 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: publishing the bot's liq_pnl_* windows every five minutes. |
| Description check | |
The description directly explains the new liq-pnl-refresh task, its six windows, timing, behavior, tests, and known verification limits. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
Explanation
Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 108 functions across 8 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@rain-marvin review |
✅ Action performedReview finished.
|
|
🔎 Reviewing |
![rain-marvin[bot]](https://avatars.githubusercontent.com/in/5077017?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds the supervised liq-pnl-refresh task. Every 5 minutes it runs one PnL report per window (1w, 1d, all, 1m, ytd, 1y) through the same path as GET /pnl, and publishes all 25 liq_pnl_* series from the bot. Each window is its own family with its own collector timestamp. The task has its own one-permit admission, so the live /pnl permits stay free.
The latest round (f2bb5f2) fixes both CodeRabbit threads. compare.py now excuses a missing liq_pnl_day_* series only while the bot still publishes that name for the same window, so a whole dropped day name is a finding again, and a new test covers it. Symbol rows that strip to the same label are left out with an error log, so no row silently replaces another. Both changes do what the replies say.
The goal holds: the window table, the probe, the order and the 120 s budget match the exporter. A failed or skipped window keeps its last value, and its timestamp shows that it is stale. Nothing adds state, schema or config, so an upgrade or rollback cannot fail to start. The deferred items in the open threads stay as agreed follow-ups.
One minor point is new: the sample build does a lot of Float work on an async worker, while /pnl runs the same kind of work in spawn_blocking. Two doc nits are also new. No blocker.
I also agree with 1 open thread and replied +1 in each.
claude-opus-5-5 · high · 12 min
|
@JuaniRios 🔔 Follow-up: a day after my review @JuaniRios, the bots are done with #1685: CodeRabbit and I both approved head claude-opus-5-5 · high · 2 h 31 min |
agryaznov
left a comment
agryaznov
left a comment
There was a problem hiding this comment.
LGTM, the deferred minors are tracked in the threads.
ueco-jb
left a comment
ueco-jb
left a comment
There was a problem hiding this comment.
Reviewed at f2bb5f2. The per-window family replacement, independent admission, date ranges, failure/freshness handling, exact PnL conversion, and exporter parity are sound. Two non-blocking follow-ups remain: make independent cost fields distinct in the golden fixture so each mapping is pinned, and state the 90-bucket cut on all four day-series catalog rows. No blockers.
Merge activity
|
github-actions
Bot
added
the
externally-merged
3 participants
Part of RAI-3093.
Publishes all 25
liq_pnl_*series from the bot for the 6 PnL windows, built from the same report asGET /pnl. Item 4 of the RAI-2998 stack, part 2 of 3.Live effect: a new supervised task,
liq-pnl-refresh, runs 6 PnL reports every 5 min and publishes theliq_pnl_*names, withliq_collector_last_success_ts_seconds{collector="pnl_<window>"}per window · Risk: medium (6 extra ledger replays and 6 Alpaca activity fetches every 5 min while the exporter still runs) · Ships: on the next bot releaseWhy
The TVL report, the board and the shareholder numbers read
liq_pnl_*. Until now only the exporter sidecar published them. No P&L series is dropped.Decisions
1d,1w,1m(anchor minus 30),ytd,1y(minus 364) andall. All end on the last day with fills, and no start is before the first day with fills. A cold range cache costs one probe report. Order1w, 1d, all, 1m, ytd, 1y, inside a 120 s budget.LiqFamily::Pnl(window), and owns only samples with itswindowlabel.pnl_<window>collector time stops advancing, so staleness is visible. This is a recorded difference:compare.pyhas akept_windowrule for it./pnlkeeps both of its permits. Each window runs its own ledger catch-up, as each exporter call did.tAAPLandwtAAPL). An unparseable summary decimal leaves its series absent. All sums use exactFloat.8dca7be,pnl_day_samples). Socompare.py --known-diffsgets akept_window_bot_absentrule for the fourliq_pnl_day_*names: a day series that only the bot is missing is not a finding. Values that both publish are still compared. The cost: the compare no longer catches any other day series the bot drops. compare.pymetrics_refresh_duration_seconds{collector="pnl_<window>"}, failed ones too.Proof
golden.py --check(12 files),test_compare.py, andgen-t0-liquidity.py --checkpass.226c029:pnlrunscollect_pnlover all 6 windows;pnl-dayshas 95 day buckets to check the 90 cut and the running totals. Fixtures use fake values only.ytd,1m = A-30), one probe then 6 windows in order, a failed window keeps its samples and timestamp, the budget skips windows and keeps them, no fills publishes nothing, a later fill day moves the next cycle, the metrics task runs while live requests hold both permits, and an unparseable day component leaves its series out.st0x-hedgesuite on the top of the stack, before the latest fix push: 4814 passed.compare.py, plan section 13), the/pnl503 rate and the window p95 during Stage 3. Those need a release.Merge conditions
Same as the stack below.