Skip to content

fix(trade-journal): only claim the Futu format when its value columns exist - #1766

Merged
warren618 merged 1 commit into
HKUDS:mainfrom
remote-controlled-man:codex/vibe-trading-trade-journal-header-aliasing
Oct 10, 2026
Merged

warren618 merged 1 commit into
HKUDS:mainfrom
remote-controlled-man:codex/vibe-trading-trade-journal-header-aliasing

Conversation

@remote-controlled-man

Copy link
Copy Markdown
Contributor

Summary

  • detect_format() returned "futu" for any frame whose columns include the exact-case set Date, Symbol and Side (or Direction). That is an ordinary English journal header, not a Futu-specific signature.
  • parse_futu() then reads its value columns by exact spelling (Quantity, Price, Amount, Commission, Platform Fee), and _to_float(None) returns its 0.0 default. A journal headed Date,Symbol,Side,Qty,Price therefore parsed into records with zero quantity and zero amount, while parse_file() still returned success.
  • Require a side column and one of the value columns parse_futu() actually reads before claiming the Futu format. Everything else now reaches parse_generic(), which already matches headers case-insensitively and documents the qty / size / volume aliases.

Why

Self-discovered while reading the parser; no existing issue.

analyze_trade_journal is documented in the agent context (src/agent/context.py) as the entry point for "parse + profile + behavior diagnostics". Over an affected journal it returns status: ok with total_roundtrips: 0, win_rate: 0.0, total_pnl: 0.0 and format_detected: "futu", so the agent reports an empty but successful analysis of a journal that contains real fills. shadow_account/extractor.py and shadow_account/backtester.py reach the same code and raise No complete buy→sell roundtrips found in journal.

Identical rows, only the header spelling differs — measured on main at 2620a39:

Date,Symbol,Side,Qty,Price  ->  detected: futu     qty [0.0, 0.0]    amount [0.0, 0.0]
date,symbol,side,qty,price  ->  detected: generic  qty [10.0, 10.0]  amount [1000.0, 1200.0]

The capitalised form is the one that breaks, because the Futu branch is tested before the case-insensitive generic branch. This is exactly the failure _dividend_amount's docstring says the module avoids: a zero is meant to be "surfaced downstream, never silently treated as a real zero payout".

Existing tests did not catch it because every end-to-end fixture in test_trade_journal.py uses lowercase headers, which misses the case-sensitive Futu branch.

Changes

  • agent/src/tools/trade_journal_parsers.py (+13/−1): add _FUTU_SIDE_COLUMNS and _FUTU_VALUE_COLUMNS, and require both in the Futu branch of detect_format().
  • agent/tests/test_trade_journal.py (+79): three regression tests — the detection signature, parse_file() economics across three alias spellings, and the analyze_trade_journal envelope.

Deliberately out of scope:

  • No new parser, no new detection signature, no new vendor format. This narrows an over-broad signature so the existing generic parser handles frames the Futu parser cannot read. It is not a format addition of the kind fix(trade_journal): support markdown tables + Longbridge format #460 proposed.
  • parse_futu() is unchanged, so a genuine Futu export — which always carries Quantity — parses exactly as before. Both pinned detection tests use that shape and still pass.
  • No alias table was added inside parse_futu(). That alternative duplicates logic parse_generic() already documents, and would push a 703-line file toward the 800-line hard cap in CONTRIBUTING.md; this fix takes it to 715.
  • No reformatting. black --check already fails on both pristine files under black 26.10.0, and CONTRIBUTING.md asks not to mix unrelated formatting cleanup into a focused PR. ruff check passes on both changed files, and black's proposed changes to the patched files touch only pre-existing lines.

A re-routed frame cannot regress: if it carries neither Quantity nor Amount, parse_futu() could only ever return zero quantity and zero amount for it, so handing it to the alias-aware generic parser cannot do worse. The returned format name has no behavioural consumers — trade_journal_tool.py only reports it as format_detected (now accurate), extractor.py only interpolates it into an error string, and backtester.py discards it. Nothing branches on fmt == "futu" outside parse_file()'s own fmt == "unknown" check.

Test Plan

  • Existing tests pass (pytest --ignore=agent/tests/e2e_backtest --tb=short -q)
  • New tests added
  • Tested manually (described below)

Environment: isolated venv, pip install -e ".[dev]", pandas 2.3.3 / numpy 2.4.6 per requirements-lock.txt, Python 3.11.7 on Windows.

  • RED first: the new tests fail on unmodified main — 5 failed, 118 deselected, every failure assert 'futu' == 'generic'.
  • pytest agent/tests/test_trade_journal.py -q → 123 passed (118 baseline + 5 new).
  • Full invalidation scope — the 26 test files that import trade_journal_parsers / trade_journal_tool / shadow_account or reference detect_format / parse_file → 521 passed.
  • ruff check on both changed files → clean.
  • Manual: the two-header reproduction above, run against parse_file() before and after.
  • Full offline suite (pytest --ignore=agent/tests/e2e_backtest --ignore=agent/tests/test_e2e_harness_v2.py): 45 failed, 16975 passed, 168 skipped, 4 errors on this Windows host. Every one of those failures is pre-existing: re-running the 25 affected files with both changed files reverted to origin/main gives 47 failed / 393 passed / 4 errors, and the patched failure set is a strict subset of that baseline (node-ID diff). The two extra baseline failures are test_channel_authoring_contract.py[slack]/[telegram], which also fail when that file is run alone on the patched source, so they are a test-isolation artifact. The failures are Windows symlink-privilege guards, a Unix-domain-socket test, packaging-metadata assertions against an editable uv install, and console-encoding collection errors on very large unicode parametrizations — none touches the trade journal.
  • Not run: the frontend build/tests and the desktop-electron-windows / Windows background-lifecycle CI jobs, and CI on other operating systems. No live broker, MCP, network, credential or deployment path is exercised — this is pure offline CSV parsing, and pytest-socket is in the dev extra.

Risk: none beyond the parse routing described above. Rollback: git revert.

Checklist

  • No changes to protected areas (src/agent/, src/session/, src/providers/) without prior discussion
  • No hardcoded values (API keys, file paths, magic numbers)
  • Code follows CONTRIBUTING.md guidelines
  • Documentation updated (if user-facing change) — no README change proposed: no interface, flag, connector or documented format list changes, and AGENT_CONTRIBUTOR_GUIDE.md scopes the README rule to user-facing CLI/Web/MCP/connector/provider/safety-boundary changes.

… exist

detect_format() returned "futu" for any frame carrying the exact-case
columns Date, Symbol and Side (or Direction). That is an ordinary English
journal header, not a Futu signature, and parse_futu() reads Quantity,
Price, Amount, Commission and Platform Fee by exact spelling while
_to_float(None) defaults to 0.0. A journal headed Date,Symbol,Side,Qty,Price
therefore parsed to records with zero quantity and zero amount, and
parse_file() still reported success, so analyze_trade_journal answered
status ok with total_pnl 0.0 and format_detected "futu" over real fills.

Require a side column and one of the value columns parse_futu() actually
reads before claiming the format. Frames without one now reach
parse_generic(), which already matches headers case-insensitively and
documents the qty/size/volume aliases. A genuine Futu export always
carries Quantity, so it is unaffected; a re-routed frame cannot regress,
because parse_futu() could only ever return zero quantity and zero amount
for it.

Signed-off-by: Steven Huang <134844631+remote-controlled-man@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 9, 2026 14:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@warren618
warren618 merged commit 9a4744c into HKUDS:main Oct 10, 2026
10 checks passed
@remote-controlled-man
remote-controlled-man deleted the codex/vibe-trading-trade-journal-header-aliasing branch October 13, 2026 01:27
warren618 added a commit that referenced this pull request Oct 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants