feat(backtest): expand trade detail to 14 cols and fix profit two-sided commission#1
Open
xuesongjun wants to merge 20 commits into
Open
feat(backtest): expand trade detail to 14 cols and fix profit two-sided commission#1xuesongjun wants to merge 20 commits into
xuesongjun wants to merge 20 commits into
Conversation
…ed commission - Expand trade-detail output to 14 Chinese-headed columns covering both the console table and --csv export: code, buy/sell dates, buy-day open, buy price, sell-day close, sell price, volume, total commission, net profit, return%, holding days, post-trade net equity, and reason. - Fix latent bug in BacktestEngine where profit only deducted the sell-side order.commission; buy-side commission was silently dropped, causing ETF profit to be systematically over-optimistic. Now profit = (sell_price - buy_price) * volume - buy_commission - sell_commission. - _buy_records now stores buy-side commission and buy-day open price so sell leg can reconstruct the full cost basis. - Unify column schema via TRADE_COLUMNS constant in run_backtest.py, reused by the console renderer and CSV export rename/keep-keys logic. - Verified on 513090 ETF 2023-01-01~2026-04-07 (393 trades); net profit now agrees with equity_curve daily diff. - Add root-level research.md for accumulated research notes; append a "9. 迭代日志" section to plan.md to track iterations.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitched slippage to percentage, introduced pending next-open execution, changed trade recording to per-day aggregation with both-side commission accounting, made overnight_long emit independent BUY (close) and SELL (next_open) signals, added deterministic paper account IDs, tightened sell-volume rules, and updated CLI/CSV/docs/tests. Changes
sequenceDiagram
participant Strategy as OvernightLongStrategy
participant Engine as BacktestEngine / PaperEngine
participant Repo as MarketDataRepo
participant Account as Positions/Account
participant Output as CSV/Console/DB
Strategy->>Engine: on_bar(bar) -> SELL check -> emit SIGNAL(next_open)
Strategy->>Engine: on_bar(bar) -> BUY check -> emit SIGNAL(close)
Engine->>Account: schedule pending SELL (next_open) / execute BUY (close)
Engine->>Repo: query fills / prices for execution
Account-->>Engine: fill events (include buy/sell commissions)
Engine->>Engine: aggregate daily actions -> finalize daily trade (commission=buy+sell)
Engine->>Output: append/export daily trade record (TRADE_COLUMNS mapping)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/run_backtest.py (1)
224-237: Robust cell rendering with good error handling.The null/NaN detection at line 228 (
raw != raw) is a valid idiom for float NaN. The try/except fallback tostr(raw)at lines 231-234 handles unexpected formatter failures gracefully.💡 Optional: Add `strict=True` to zip for extra safety
Per Ruff B905, adding
strict=Truedocuments the expectation thatvaluesandTRADE_COLUMNShave equal length:line = "|" + "|".join( - _pad_str(val, w) for val, (_, _, w, _) in zip(values, TRADE_COLUMNS) + _pad_str(val, w) for val, (_, _, w, _) in zip(values, TRADE_COLUMNS, strict=True) ) + "|"This is safe since both are derived from the same iteration, but
strict=Truemakes the contract explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run_backtest.py` around lines 224 - 237, The code handles None/NaN and formatter errors for TRADE_COLUMNS correctly, but make the contract explicit by using zip(..., strict=True) when pairing values with TRADE_COLUMNS to ensure they have equal length; update the expression that builds line (the zip used with _pad_str and TRADE_COLUMNS) to pass strict=True so mismatches raise immediately, referencing TRADE_COLUMNS, values, fmt and _pad_str to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/run_backtest.py`:
- Around line 224-237: The code handles None/NaN and formatter errors for
TRADE_COLUMNS correctly, but make the contract explicit by using zip(...,
strict=True) when pairing values with TRADE_COLUMNS to ensure they have equal
length; update the expression that builds line (the zip used with _pad_str and
TRADE_COLUMNS) to pass strict=True so mismatches raise immediately, referencing
TRADE_COLUMNS, values, fmt and _pad_str to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 735cba73-0637-441c-957d-75ab28d9b608
📒 Files selected for processing (6)
README.mdbacktest/engine.pyplan.mdprocess.txtresearch.mdscripts/run_backtest.py
Rework on_bar decision logic from if/elif to two independent if branches. Previously the strategy held position only every other day (buy → overnight → sell → idle one day → buy), yielding ~50% exposure. New logic generates both SELL (next_open) and BUY (close) signals on the same bar, so the account rolls over continuously night-to-night at ~100% exposure, matching the "overnight factor" intent. Add limit_pct parameter (default 0.10 for mainboard/cross-border ETFs, 0.20 for ChiNext/STAR) for one-limit-down detection: if bar.open <= pre_close × (1 - limit_pct) + 1e-6, both SELL (cannot be matched) and BUY (funds still locked by position) are skipped for the day. Narrow min_drop_pct / max_rise_pct filter semantics to BUY only — SELL is a position-exit condition unaffected by entry filters. Rely on [SELL, BUY] list ordering and engine's list[Signal] processing (fixed in previous iteration) so SELL frees cash before BUY consumes it. Fix logic bug caught by test_position_with_zero_available_skips_sell: initial implementation treated "no sellable shares" as empty, missing the T+1 frozen case where position exists but available=0. Use explicit currently_empty = (pos is None or pos.volume == 0). Tests: 8 → 11 cases; amended 4 existing assertions, added 3 new (one_limit_down_blocks_both / limit_down_recovery_next_day / custom_limit_pct_changes_limit_threshold).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
research.md (1)
26-43: Add language specifier to pseudo-code block.The code block at line 29 is missing a language specifier.
📝 Proposed fix
-``` +```python has_sellable = pos and pos.available > 0 sell_blocked = has_sellable and (bar.open <= limit_down_price + 1e-6)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@research.md` around lines 26 - 43, The fenced code block containing the pseudo-code (lines showing has_sellable, sell_blocked, currently_empty, will_sell_all and the two independent if branches) is missing a language specifier; add the language tag "python" to the opening fence (i.e., change the opening triple backticks to ```python) and ensure the block still ends with a closing triple backticks so the pseudo-code is properly highlighted and rendered.plan.md (1)
516-527: Add language specifier to fenced code block.The code block at line 518-520 is missing a language specifier, which triggers markdownlint MD040.
📝 Proposed fix
-``` +```text 代码 | 买入日 | 卖出日 | 开盘价 | 买入价 | 收盘价 | 卖出价 | 份额 | 佣金 | 净盈 | 收益率% | 持仓天数 | 净值 | 卖出原因</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@plan.mdaround lines 516 - 527, The fenced code block under the
"输出字段设计（中文表头，14 列）" heading is missing a language specifier and triggers
markdownlint MD040; update the triple-backtick fence that contains the header
row "代码 | 买入日 | 卖出日 | 开盘价 | 买入价 | 收盘价 | 卖出价 | 份额 | 佣金 | 净盈 | 收益率% | 持仓天数 | 净值 |
卖出原因" to include a language tag (e.g., ```text) on the opening fence so the
block is explicitly typed.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@plan.md:
- Around line 516-527: The fenced code block under the "输出字段设计（中文表头，14 列）"
heading is missing a language specifier and triggers markdownlint MD040; update
the triple-backtick fence that contains the header row "代码 | 买入日 | 卖出日 | 开盘价 |
买入价 | 收盘价 | 卖出价 | 份额 | 佣金 | 净盈 | 收益率% | 持仓天数 | 净值 | 卖出原因" to include a language
tag (e.g., ```text) on the opening fence so the block is explicitly typed.In
@research.md:
- Around line 26-43: The fenced code block containing the pseudo-code (lines
showing has_sellable, sell_blocked, currently_empty, will_sell_all and the two
independent if branches) is missing a language specifier; add the language tag
"python" to the opening fence (i.e., change the opening triple backticks tothe pseudo-code is properly highlighted and rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
e2802e10-9e99-4b8f-bc89-44ffaac58a69📒 Files selected for processing (8)
README.mdconfig/strategies.yamldocs/superpowers/specs/2026-04-19-overnight-long-design.mdplan.mdprocess.txtresearch.mdstrategy/technical/overnight_long.pytests/test_overnight_long.py✅ Files skipped from review due to trivial changes (1)
- config/strategies.yaml
added 6 commits
April 20, 2026 15:28
- rename settings.yaml key slippage (0.01) -> slippage_rate (0.0 default) - rename BacktestConfig.slippage -> BacktestConfig.slippage_rate - rename BacktestEngine ctor arg slippage -> slippage_rate, use `is not None` instead of `or` so explicit 0 is not swallowed - change formula: exec_price +/- slippage -> exec_price * (1 +/- rate) with round(., 3) to fit A-share min tick (0.001 yuan) - short-circuit `if self.slippage_rate:` so rate=0 exactly preserves bar prices (skip the multiply+round entirely) - add CLI flag --slippage-rate to scripts/run_backtest.py (default None so yaml default still wins when flag omitted) - sync docs: README / plan.md / research.md / process.txt
Previously each row in the trade detail output represented one round-trip (buy_date -> sell_date), so row 1 spanned two calendar days (e.g. buy 01-05, sell 01-06), which was easy to misread as "both buy and sell happened on 01-05" for an overnight strategy. New schema: one row per trading day, with an explicit "action" column labeling the row as 建仓 (buy only) / 换仓 (sell-then-buy) / 平仓 (sell only). Empty fields on entry/exit rows express "no such action today" directly. Engine changes: - add self._daily_actions as per-day buffer keyed by code - _process_signal only places the order and buffers the action - run() calls a new _finalize_daily_trade(td, bar_map) at the end of each day (after record_equity so net_equity is current) Column changes (14 -> 15 columns): - buy_date + sell_date merged into single "日期" - volume split into "卖出份额" + "买入份额" (unequal on swap days since the same cash buys a different share count at a new price) - added "动作" column for the row type - "卖出原因" renamed to "动作备注" Commission vs profit intentionally have different semantics now: commission is cash-flow view (all fills on the day), profit is round-trip view (prev buy -> this sell, both-side commissions deducted). Swap-day new-entry commission carries over to the next close-out's round-trip. Also fix a minor pandas quirk in the CSV exporter: int columns with None values were upcast to float and written as "9000.0"; format them explicitly as int strings or empty.
Put 卖出价 right after 开盘价, and 买入价 right after 收盘价. The execution price of each action sits next to its reference bar price now (sell_price ~ open; buy_price ~ close), which matches the 9:25 sell / 14:55 buy event timeline and makes the table easier to scan.
…action open and close are the reference bar prices for the day; they should be filled for every row, not only when a matching buy/sell happened. buy_price / sell_price remain action-specific. Previously close was left blank on 建仓 rows (leftover of the round-trip schema where close meant "sell day close"). Treat close the same as open now — always bar.close.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backtest/engine.py (3)
363-365: Simplify the dictionary access.The key check before access is redundant. Consider using
get()with a default value.♻️ Proposed fix
- # 清除最近一条买入记录（round-trip 已闭合） - if signal.code in self._buy_records and self._buy_records[signal.code]: - self._buy_records[signal.code].pop() + # 清除最近一条买入记录（round-trip 已闭合） + buy_list = self._buy_records.get(signal.code) + if buy_list: + buy_list.pop()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backtest/engine.py` around lines 363 - 365, The explicit key existence check is redundant; use self._buy_records.get(signal.code, []) to retrieve the list safely and then pop if it's non-empty. Replace the if block referencing self._buy_records and signal.code with a retrieval like lst = self._buy_records.get(signal.code, []) and call lst.pop() only when lst is truthy to avoid KeyError and simplify the logic.
42-52: Use explicitOptionaltype hint forslippage_rateparameter.PEP 484 prohibits implicit
Optional. The parameter should explicitly indicate it can beNone.♻️ Proposed fix
def __init__( self, strategy: BaseStrategy, stock_codes: list[str], start_date: date, end_date: date, - initial_capital: float = None, - slippage_rate: float = None, + initial_capital: float | None = None, + slippage_rate: float | None = None, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backtest/engine.py` around lines 42 - 52, The slippage_rate parameter must explicitly allow None per PEP 484: update the function/method signature that declares slippage_rate (the constructor where self.slippage_rate is set) to use Optional[float] instead of float, add the necessary import of Optional from typing, and keep the current fallback logic that assigns self.slippage_rate = slippage_rate if slippage_rate is not None else BacktestConfig.slippage_rate unchanged; ensure hints for other related params remain consistent.
299-312: Consider reusing the existing signal instead of creating a new one for validation.The code creates a new
Signalobject with the same values as the original signal (except usingexec_pricefor price). Since the originalsignalalready has most of these values, this could be simplified.♻️ Proposed simplification
else: - valid, reason = TradingRules.validate_order( - Signal( - code=signal.code, - direction=signal.direction, - trade_date=signal.trade_date, - price=exec_price, - volume=volume, - ), + # Create a validation signal with the computed exec_price and volume + validation_signal = Signal( + code=signal.code, + direction=signal.direction, + trade_date=signal.trade_date, + price=exec_price, + volume=volume, + ) + valid, reason = TradingRules.validate_order( + validation_signal, bar, self.account.cash, pos_volume, pos_available, )Alternatively, if
TradingRules.validate_ordercould accept individual parameters instead of a Signal object, that would be cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backtest/engine.py` around lines 299 - 312, The code constructs a new Signal just to change price for validation; instead reuse the existing signal by passing it to TradingRules.validate_order with the updated exec_price (either by creating a lightweight copy/replace of signal that only changes price or by adding an optional price_override parameter to TradingRules.validate_order) and keep the other args (bar, self.account.cash, pos_volume, pos_available) the same; update validate_order signature if you choose the price_override approach so callers can pass exec_price without reconstructing Signal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/sources/tushare_source.py`:
- Line 183: The comment string "# tushare 指数代码格式：" contains a fullwidth colon
which triggers Ruff RUF003; replace the fullwidth colon `：` with an ASCII colon
`:` so the line reads "# tushare 指数代码格式:" (locate the comment near the tushare
index code handling, e.g., the comment above the index code parsing logic) to
resolve the lint warning.
In `@plan.md`:
- Line 674: Update the document metadata string "*文档版本：v1.1 | 创建日期：2026-03-17 |
最后更新：2026-04-19*" so the "最后更新" field reflects the actual latest edits (change
to "最后更新：2026-04-20") and ensure the top-of-file metadata and any other
occurrences of that exact metadata string in plan.md are synchronized with the
new date to avoid inconsistent version info.
In `@scripts/run_paper_trade.py`:
- Around line 62-78: build_account_id is producing different IDs for
semantically identical numeric params because parse_params can return ints or
floats (e.g., 3 vs 3.0); to fix this, canonicalize numeric param values before
hashing in build_account_id (or in parse_params) by detecting numeric
types/strings and converting them to a consistent representation (e.g., always
cast to float and format with a stable string format or normalize with Decimal
and a fixed precision), then use that normalized params dict when creating raw
for json.dumps; reference build_account_id and parse_params when making the
change so numeric keys like min_drop_pct are consistently serialized.
---
Nitpick comments:
In `@backtest/engine.py`:
- Around line 363-365: The explicit key existence check is redundant; use
self._buy_records.get(signal.code, []) to retrieve the list safely and then pop
if it's non-empty. Replace the if block referencing self._buy_records and
signal.code with a retrieval like lst = self._buy_records.get(signal.code, [])
and call lst.pop() only when lst is truthy to avoid KeyError and simplify the
logic.
- Around line 42-52: The slippage_rate parameter must explicitly allow None per
PEP 484: update the function/method signature that declares slippage_rate (the
constructor where self.slippage_rate is set) to use Optional[float] instead of
float, add the necessary import of Optional from typing, and keep the current
fallback logic that assigns self.slippage_rate = slippage_rate if slippage_rate
is not None else BacktestConfig.slippage_rate unchanged; ensure hints for other
related params remain consistent.
- Around line 299-312: The code constructs a new Signal just to change price for
validation; instead reuse the existing signal by passing it to
TradingRules.validate_order with the updated exec_price (either by creating a
lightweight copy/replace of signal that only changes price or by adding an
optional price_override parameter to TradingRules.validate_order) and keep the
other args (bar, self.account.cash, pos_volume, pos_available) the same; update
validate_order signature if you choose the price_override approach so callers
can pass exec_price without reconstructing Signal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86ecb152-3d67-421e-a3ba-b7d5f76978db
📒 Files selected for processing (21)
.gitignoreREADME.mdbacktest/engine.pybacktest/metrics.pybacktest/rules.pyconfig/__init__.pyconfig/settings.yamlconfig/strategies.yamldata/sources/tushare_source.pyplan.mdprocess.txtresearch.mdscripts/run_backtest.pyscripts/run_paper_trade.pystrategy/registry.pystrategy/technical/overnight_long.pytests/test_overnight_long.pytests/test_overnight_long_engines.pytests/test_paper_trade_helpers.pytests/test_trading_rules.pytrading/paper_engine.py
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- config/strategies.yaml
| # tushare 指数代码格式 | ||
| if code.startswith("0"): | ||
| ts_code = f"{code}.SH" | ||
| # tushare 指数代码格式： |
There was a problem hiding this comment.
Replace fullwidth colon in comment to avoid Ruff RUF003.
Line 183 uses ： (fullwidth colon), which Ruff flags as ambiguous. Use ASCII : instead.
Proposed fix
- # tushare 指数代码格式：
+ # tushare 指数代码格式:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # tushare 指数代码格式： | |
| # tushare 指数代码格式: |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 183-183: Comment contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data/sources/tushare_source.py` at line 183, The comment string "# tushare
指数代码格式：" contains a fullwidth colon which triggers Ruff RUF003; replace the
fullwidth colon `：` with an ASCII colon `:` so the line reads "# tushare
指数代码格式:" (locate the comment near the tushare index code handling, e.g., the
comment above the index code parsing logic) to resolve the lint warning.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plan.md (1)
474-476: Markdown 格式优化：代码块语言标识与表格空行。静态分析工具提示了几处可选的 Markdown 格式改进：
- 第 474-476 行和 648-650 行的代码块缺少语言标识符（建议添加
text或省略围栏改用缩进）- 第 481 行的表格前后缺少空行（Markdown 最佳实践建议表格前后各留一空行）
这些改进可提升文档渲染一致性，但不影响实际功能。
✨ 建议的格式改进
代码块添加语言标识：
-``` +```text 代码 日期 动作 开盘价 买入价 收盘价 卖出价 卖出份额 买入份额 佣金 净盈 收益率% 持仓天数 净值 动作备注以及： ```diff -``` +```text 代码 | 买入日 | 卖出日 | 开盘价 | 买入价 | 收盘价 | 卖出价 | 份额 | 佣金 | 净盈 | 收益率% | 持仓天数 | 净值 | 卖出原因**表格前后添加空行：** ```diff **核心改动**： + | 文件 | 改动 | |------|------| | `backtest/engine.py` | 新增 `_daily_actions` 暂存 + `_finalize_daily_trade` 日末聚合；`_process_signal` 不再直接 append `_trades` | + | `scripts/run_backtest.py` | TRADE_COLUMNS 重排 + CSV 整数列空值处理（避免 `9000.0`） |Also applies to: 481-485, 648-650
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plan.md` around lines 474 - 476, The markdown has fenced code blocks without a language specifier (the block containing the line "代码 日期 动作 开盘价 买入价 收盘价 卖出价 卖出份额 买入份额 佣金 �净盈 收益率% 持仓天数 净值 动作备注" and the similar block with "代码 | 买入日 | 卖出日 | 开盘价 | 买入价 | 收盘价 | 卖出价 | 份额 | 佣金 | 净盈 | 收益率% | 持仓天数 | 净值 | 卖出原因") — add a language identifier (e.g., ```text) or replace fenced blocks with indented code blocks; additionally ensure there is a blank line before and after the markdown table that starts around the "表格" section (the table referenced near the snippet around the row with 买入日/卖出日) so the table is separated by one empty line above and below; apply the same fixes to the other occurrences noted (the blocks around the other ranges of the document).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plan.md`:
- Around line 474-476: The markdown has fenced code blocks without a language
specifier (the block containing the line "代码 日期 动作 开盘价 买入价 收盘价 卖出价 卖出份额 买入份额 佣金
�净盈 收益率% 持仓天数 净值 动作备注" and the similar block with "代码 | 买入日 | 卖出日 | 开盘价 | 买入价 |
收盘价 | 卖出价 | 份额 | 佣金 | 净盈 | 收益率% | 持仓天数 | 净值 | 卖出原因") — add a language identifier
(e.g., ```text) or replace fenced blocks with indented code blocks; additionally
ensure there is a blank line before and after the markdown table that starts
around the "表格" section (the table referenced near the snippet around the row
with 买入日/卖出日) so the table is separated by one empty line above and below; apply
the same fixes to the other occurrences noted (the blocks around the other
ranges of the document).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53d12637-8c06-4fb6-9b42-64123ac549e9
📒 Files selected for processing (2)
plan.mdtests/test_paper_trade_helpers.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_paper_trade_helpers.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plan.md (1)
831-831:⚠️ Potential issue | 🟡 MinorFooter update date is stale.
Line 831 still shows
最后更新：2026-04-19while this document contains 2026-04-20 updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plan.md` at line 831, Update the footer string "*文档版本：v1.1 | 创建日期：2026-03-17 | 最后更新：2026-04-19*" to reflect the correct last-updated date "最后更新：2026-04-20" so the document footer matches the content updates.scripts/run_paper_trade.py (1)
248-250:⚠️ Potential issue | 🟡 MinorCanonicalize numeric params before hashing
account_id.
parse_params()still preserves3and3.0as different values, so semantically identical configs can map to different account IDs and silently split--status/--historylookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run_paper_trade.py` around lines 248 - 250, The account_id diverges because parse_params preserves numeric distinctions like "3" vs "3.0" before build_account_id; canonicalize numeric param values (e.g., normalize integers and floats to a consistent representation such as converting numeric-like strings to numbers and formatting floats consistently or casting numeric floats with integer value to ints) and use those canonicalized params when calling build_account_id; update either parse_params to normalize numeric values or insert a normalization step between parse_params(args.params) and build_account_id(args.strategy, args.codes, params) so semantically identical configs produce the same account_id.
🧹 Nitpick comments (1)
api/dependencies.py (1)
24-46: Extract account-id param parsing into a shared helper.
_split_params()is now a second copy of the CLI parsing rules inscripts/run_paper_trade.py::parse_params(). Since both paths feedbuild_account_id(), any drift here will make the dashboard resolve a different account than the CLI created.Also applies to: 49-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/dependencies.py` around lines 24 - 46, The _split_params implementation duplicates CLI parsing logic from scripts/run_paper_trade.py::parse_params which can cause account-id drift; refactor by extracting the parsing logic into a shared helper (e.g., params.parse_params or utils.parse_params) and have api.dependencies._split_params and scripts.run_paper_trade.parse_params both call that single function, ensuring the helper returns the same dict shape used by build_account_id; remove the duplicate code blocks in api.dependencies and in the script and update imports to reference the new shared helper name to keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@trading/account_id.py`:
- Around line 27-28: The current account-id construction uses sha1 truncation
(digest = hashlib.sha1(...).hexdigest()[:10]) which would break existing records
if you switch to SHA256; instead implement a migration-safe approach: introduce
a versioned id format (e.g., prefix the returned id with a version token like
"v1:"/ "v2:" in the function that currently returns f"{strategy_key}:{digest}"),
and update lookup logic to perform dual-read—first check for the new versioned
key, then fall back to the legacy sha1 key (using the existing digest generation
code) and, if found, write back the record under the new versioned id to migrate
it; keep the old sha1 generation code (digest and strategy_key usage) for reads
until migration completes.
---
Duplicate comments:
In `@plan.md`:
- Line 831: Update the footer string "*文档版本：v1.1 | 创建日期：2026-03-17 |
最后更新：2026-04-19*" to reflect the correct last-updated date "最后更新：2026-04-20" so
the document footer matches the content updates.
In `@scripts/run_paper_trade.py`:
- Around line 248-250: The account_id diverges because parse_params preserves
numeric distinctions like "3" vs "3.0" before build_account_id; canonicalize
numeric param values (e.g., normalize integers and floats to a consistent
representation such as converting numeric-like strings to numbers and formatting
floats consistently or casting numeric floats with integer value to ints) and
use those canonicalized params when calling build_account_id; update either
parse_params to normalize numeric values or insert a normalization step between
parse_params(args.params) and build_account_id(args.strategy, args.codes,
params) so semantically identical configs produce the same account_id.
---
Nitpick comments:
In `@api/dependencies.py`:
- Around line 24-46: The _split_params implementation duplicates CLI parsing
logic from scripts/run_paper_trade.py::parse_params which can cause account-id
drift; refactor by extracting the parsing logic into a shared helper (e.g.,
params.parse_params or utils.parse_params) and have
api.dependencies._split_params and scripts.run_paper_trade.parse_params both
call that single function, ensuring the helper returns the same dict shape used
by build_account_id; remove the duplicate code blocks in api.dependencies and in
the script and update imports to reference the new shared helper name to keep
behavior identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20fe882f-91c7-483d-b3e4-04aaf5160a31
📒 Files selected for processing (17)
.gitignoreapi/dependencies.pyapi/main.pyapi/routers/__init__.pyapi/routers/dashboard.pyapi/schemas.pydata/storage/repository.pyfrontend/package.jsonfrontend/src/api/dashboard.tsfrontend/src/types.tsfrontend/src/vite-env.d.tsfrontend/tsconfig.jsonfrontend/vite.config.tsplan.mdresearch.mdscripts/run_paper_trade.pytrading/account_id.py
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- frontend/package.json
- frontend/src/vite-env.d.ts
- frontend/tsconfig.json
- api/schemas.py
- frontend/src/types.ts
Comment on lines
+27
to
+28
| digest = hashlib.sha1(raw.encode("utf-8")).hexdigest()[:10] | ||
| return f"{strategy_key}:{digest}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify where build_account_id/account_id is used as an identity key.
rg -nP --type=py -C3 '\bbuild_account_id\s*\(|\baccount_id\b'
# Verify PaperAccount unique-key assumptions in ORM model.
rg -nP --type=py -C5 'class\s+PaperAccount|strategy_name\s*=|unique=True' data/models.pyRepository: xuesongjun/Apex
Length of output: 15636
🏁 Script executed:
# Check for migration logic or version handling of account_id format
rg -nP --type=py 'migrate|version|compatible|account_id.*format' --max-count=20
# Check if account_id is ever used as lookup key (would break with hash change)
rg -nP --type=py 'get_paper_account|by.*account_id|where.*strategy_name' --max-count=20
# Check PaperRepository for account lookup implementation
fd -type f -name '*repository*' | xargs -I {} sh -c 'wc -l {} && head -50 {}'Repository: xuesongjun/Apex
Length of output: 843
🏁 Script executed:
# View the get_paper_account implementation
sed -n '390,420p' data/storage/repository.py
# Check if there's any index or constraint on strategy_name as unique key
rg -nP 'strategy_name.*unique|unique.*strategy_name' --type=py
# See how accounts are inserted (to understand if old format is migrated)
rg -nP 'def.*paper_account|insert.*account|add.*account' data/storage/repository.py -A 5Repository: xuesongjun/Apex
Length of output: 2427
🏁 Script executed:
# Check if there's any migration logic or account lookup by params
rg -nP 'migrate|upgrade|migration' data/ scripts/ --type=py
# Check if paper_account records are queried/updated elsewhere
rg -nP 'PaperAccountORM|paper_account.*query|update_paper' data/storage/repository.py -A 3
# Verify the critical flow: new run would generate new hash, old account not found
grep -n "get_paper_account\|create_paper_account" trading/paper_account.py data/storage/repository.py scripts/run_paper_trade.pyRepository: xuesongjun/Apex
Length of output: 2093
🏁 Script executed:
# Check for any migration tools or schema versioning
fd -type f -name '*migrat*' -o -name 'alembic*' -o -name 'schema*'
# Check if there's any deprecation or versioning of the hash format
rg -nP 'hash|digest|version.*account|account.*version' trading/account_id.py
# Final check: what happens on second run with same params - does it reuse or create new?
cat -n trading/paper_account.py | sed -n '80,100p'Repository: xuesongjun/Apex
Length of output: 1428
🏁 Script executed:
# Verify there's no version-aware lookup or fallback mechanism
rg -nP 'strategy_name.*hash|hash.*version|backward.*compat' trading/ data/ api/ --type=py
# Check if account_id is ever parsed/split for version info
rg -nP 'split.*:.*\[|parse.*account' trading/ api/ --type=pyRepository: xuesongjun/Apex
Length of output: 202
Account ID hash truncation risks data loss if changed without migration strategy.
The 40-bit digest (SHA1[:10]) does present collision risk for account isolation. However, the suggested SHA256 upgrade breaks backward compatibility: existing accounts are keyed by strategy_name="{strategy_key}:{old_digest}" in the database. Changing the hash function orphans all existing accounts, causing duplicate account creation on next run with same parameters.
A safe fix requires either:
- Dual-read migration: Support both old and new hash formats during lookup, then gradually migrate records.
- Accept current risk: If account count stays below ~1M combinations, collision probability remains low by birthday paradox. Document this assumption.
- Separate account ID registry: Decouple strategy identity from hash, using a stable integer or UUID primary key instead.
Do not apply the suggested diff without a migration path.
🧰 Tools
🪛 Ruff (0.15.10)
[error] 27-27: Probable use of insecure hash functions in hashlib: sha1
(S324)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@trading/account_id.py` around lines 27 - 28, The current account-id
construction uses sha1 truncation (digest = hashlib.sha1(...).hexdigest()[:10])
which would break existing records if you switch to SHA256; instead implement a
migration-safe approach: introduce a versioned id format (e.g., prefix the
returned id with a version token like "v1:"/ "v2:" in the function that
currently returns f"{strategy_key}:{digest}"), and update lookup logic to
perform dual-read—first check for the new versioned key, then fall back to the
legacy sha1 key (using the existing digest generation code) and, if found, write
back the record under the new versioned id to migrate it; keep the old sha1
generation code (digest and strategy_key usage) for reads until migration
completes.
added 11 commits
April 23, 2026 01:09
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--csv导出统一），新增字段：开盘价（买入日）、收盘价（卖出日）、佣金合计、净值（卖出后账户总资产）。backtest/engine.py:267的profit计算 bug：原公式只扣了卖出端order.commission，漏掉买入端佣金，导致 ETF 场景profit系统性偏乐观。TRADE_COLUMNS常量统一控制台渲染与 CSV 导出 schema，避免双份维护。Field Schema (14 cols, Chinese)
`代码 | 买入日 | 卖出日 | 开盘价 | 买入价 | 收盘价 | 卖出价 | 份额 | 佣金 | 净盈 | 收益率% | 持仓天数 | 净值 | 卖出原因`
(卖出价 - 买入价) × 份额 - 佣金合计account.total_equityWhy
用户运行 overnight_long 回测后指出交易明细信息太少，要求列全关键价格点 + 费用 + 资产变动。同时暴露了 engine
_trades字段里 profit 公式只扣单边佣金的历史 bug（对 ETF 误差更明显，因为没有印花税/过户费这些本应吃掉乐观偏差的额外费用）。Test Plan
python scripts/run_backtest.py --strategy overnight_long --codes 513090 --start 2023-01-01 --end 2026-04-07 --csv trades.csv成功输出 393 笔交易代码,买入日,卖出日,开盘价,...,卖出原因（UTF-8 BOM ✓）(1.059-1.077)*882000 - 188.39 = -16064.39与 CSV 首行净盈=-16064.39完全对得上Scope
backtest/engine.py,scripts/run_backtest.pyREADME.md,plan.md,research.md(new),process.txtbacktest/account.py,backtest/fee.py,backtest/metrics.py, all strategy files, config filesSummary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests