feat: FIX-like text protocol adapter (#29)#131
Conversation
…ollow-up) Resolve the Codex review findings left on `main` by PRs #127 and #128: - PROGRESS.md: remove the stale "Next action remains" block that still steered /resume to the merged PR #125 on `perf/linux-host-artifact-refresh`; replace with the v0.2.0 between-releases state (no active milestone; #94/#90 gated). - AGENTS.md: bring it into sync with CLAUDE.md's v0.2.0 partial-PMU reframe. The constraints bullet, the "correct claim" block, and the "M29 perf evidence status" subsection no longer label the artifacts "constrained Docker validation"; the stale `perf/linux-host-artifact-refresh` follow-up line is updated (also in CLAUDE.md) to the released state. - docs/perf_analysis.md: narrow the PMU claim so it no longer implies the Apple Blizzard (E-core) PMU carries live counts. The `apple_blizzard_pmu/...` rows read `<not counted>` in results/perf_stat_linux.txt because the single-threaded benchmark stays on the Avalanche P-cores — expected scheduling, not a counter. Docs/memory only; no code or artifacts changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add `make flamegraph`, the missing-flamegraph follow-up tracked by issue #32. The perf stat/record text workflow already existed; this renders a perf call-graph flamegraph. - scripts/flamegraph.sh: records `perf record --call-graph dwarf -F 4000 -g -e cpu-clock` on qsl-bench and writes results/flamegraph.svg plus a results/flamegraph.txt provenance/classification companion (top folded stacks). Mirrors perf_record.sh: Linux-only, reuses qsl_common.sh provenance + qsl_publish_artifact, and honours QSL_PERF_ALLOW_PARTIAL for constrained hosts. DWARF call graphs unwind correctly despite the Release `bench` preset omitting frame pointers. - scripts/flamegraph.py: dependency-free (stdlib-only) stackcollapse + SVG renderer, so the artifact is reproducible from the repo without vendoring the Perl FlameGraph toolkit. Deterministic: frames sorted by name, colors a pure function of the name, no RNG/timestamps in the drawn body. - tests/shell/test_flamegraph.sh: CTest-registered (python3-only, skips cleanly if absent) — folding (offset/dso stripping, perf-order reversal, comm-at-base, count aggregation, sortedness), SVG well-formedness, XML escaping, determinism, empty-input handling. - docs (perf_analysis.md, results/README.md), command lists (CLAUDE.md, AGENTS.md), MILESTONES.md backlog, PROGRESS.md log. `make check` 242/242. Full hardware cache-PMU evidence stays in #90. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
results/flamegraph.svg + results/flamegraph.txt generated by `make flamegraph` from the clean committed tree on the bare-metal Apple M2 (aarch64) Fedora Asahi host: 397 cpu-clock samples, 171 folded stacks, `Dirty inputs: no`. The hot paths resolve to real engine symbols (OrderBook::modify/cancel/add_limit, the dispatch_storage cancel path, decode_new_order, the gateway Session path, replay::generate_flow). Software cpu-clock sampling hot-symbol profile — not a latency/throughput claim; full hardware cache-PMU evidence stays in #90. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a human-readable `tag=value` text protocol alongside the binary codec, mapping the same internal message structs (issue #29, reprioritized by the human from the backlog). - include/qsl/protocol/fix.hpp + src/protocol/fix.cpp: SOH-framed `tag=value` adapter with genuine FIX framing — 8 BeginString / 9 BodyLength / 35 MsgType / ... / 10 mod-256 CheckSum — for the client->gateway order path: NewOrderSingle (35=D) -> NewOrder, OrderCancelRequest (35=F) -> CancelOrder. Decoding is total, deterministic, and noexcept (fixed field table, std::from_chars, std::string_view; no heap on the decode path) and reports every malformed input through a FixError taxonomy mirroring the binary codec's DecodeError. Documented, deliberate simplifications: Symbol (55) carries the numeric SymbolId; Price (44) carries integer ticks and is always present, so NewOrder<->FIX is a lossless bijection like the binary codec (price is never a float). - tests/unit/test_fix_protocol.cpp: mirrors the binary required tests and adds a cross-codec equivalence test (binary and FIX decode the same order to identical structs across all Side x OrdType x TIF), a byte-pinned fixture, and rejection of malformed framing / unsupported BeginString / unknown-or-wrong MsgType / BodyLength mismatch / CheckSum mismatch / missing field / invalid field / invalid enum / out-of-range / oversized messages. - docs/fix_protocol.md (+ pointer from docs/binary_protocol.md); MILESTONES.md and PROGRESS.md updated. make check 260/260; make asan 260/260 (the adapter parses untrusted text). Closes #29. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 41 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Address two Codex review findings in scripts/flamegraph.py::parse_collapsed: - Prefer a tab separator when present so a tab-separated folded line whose stack contains spaces (C++ signatures) splits on the trailing count instead of an interior space and is silently dropped. - Ignore non-positive sample counts, so hand-crafted --from-collapsed input with 0/negative counts cannot render a misleading SVG (all-non-positive input now fails with exit 1 via the existing empty-folded guard). Adds test coverage in tests/shell/test_flamegraph.sh (19/19). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59f0fc3227
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (const FixError e = require_int(p, kTagOrigClOrdID, out.order_id); e != FixError::None) { | ||
| return {e, {}}; | ||
| } | ||
| if (const FixError e = require_int(p, kTagSymbol, out.symbol); e != FixError::None) { | ||
| return {e, {}}; |
There was a problem hiding this comment.
When decoding a client-supplied 35=F message, this path only requires 34, 41, and 55. A frame that omits the documented required 11=ClOrdID still returns ok(), even though encode(CancelOrder) and docs/fix_protocol.md define tag 11 as required for cancel requests. That means malformed FIX cancels can be accepted instead of returning MissingField/InvalidField; require and validate tag 11 before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already enforced (3e4c8e3, before this round): decode_cancel_order requires tag 11 via r.integer(kTagClOrdID, clord_id) → MissingField when absent, covered by the "FIX cancel without required ClOrdID rejects" test. No further change needed.
flamegraph.py is a provenance input, so regenerate results/flamegraph.svg + .txt from the clean tree to keep the Source digest consistent (423 samples, Dirty inputs: no). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
decode_cancel_order validated only OrigClOrdID (41) and Symbol (55), so a 35=F message missing ClOrdID (tag 11) — which FIX requires and encode() emits — was accepted. Validate tag 11 (present and numeric) on decode without storing it, keeping decode symmetric with encode. Adds a rejection test and clarifies the docs/fix_protocol.md note for tag 11. make check 261/261; make asan 261/261. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
CodeScene's delta gate scored scripts/flamegraph.py at 7.81 (render_svg: Large Method + Excess Arguments + complexity; fold_perf_script: Bumpy Road / nested complexity). Restructure without changing output: - fold_perf_script: move per-line state into a small _Folder helper so the parsing loop is a flat if/elif/else instead of a nested block. - render_svg: bundle styling knobs into a FlameOptions dataclass (2 args, was 7) and extract _append_chrome, _frame_svg, _truncate; geometry constants (_SIDE/_PAD_TOP/_PAD_BOTTOM) hoisted to module scope and a _Canvas dataclass carries derived geometry. Emitted SVG/collapsed bytes are unchanged; tests/shell/test_flamegraph.sh 19/19. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
flamegraph.py is a provenance input; regenerate results/flamegraph.svg + .txt from the clean tree (402 samples, Dirty inputs: no). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e4c8e3cc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| malformed framing / unsupported BeginString / unknown-or-wrong MsgType / BodyLength mismatch / | ||
| CheckSum mismatch / missing field / invalid field / invalid enum / out-of-range / oversized. Docs | ||
| in `docs/fix_protocol.md` (+ pointer from `docs/binary_protocol.md`). `make check` 260/260 and | ||
| `make asan` 260/260 clean (the parser handles untrusted text). Closes #29. Do not merge from |
There was a problem hiding this comment.
This entry marks #29 as closed, but the resume/current-state anchors still list #29 as open backlog in PROGRESS.md lines 47 and 55 and in HANDOFF.md lines 85, 93, and 110. Because /resume is explicitly driven from these files, the next session will be told to pursue the FIX adapter even though this PR just added it; update those anchors alongside this closure so the project state is not contradictory.
Useful? React with 👍 / 👎.
| const Field *type = find_field(p, kTagMsgType); | ||
| if (type == nullptr || type->value.size() != 1 || type->value.front() != kMsgNewOrderSingle) { | ||
| return {FixError::UnknownMsgType, {}}; |
There was a problem hiding this comment.
Because this searches for tag 35 anywhere in the parsed field table, a client can send a frame whose first body field is something else, e.g. 8/9/34/35/.../10, and it still decodes successfully despite the documented standard envelope 8/9/35/.../10. That means malformed FIX headers are accepted rather than rejected; require out.fields[2].tag == 35 during envelope validation (or otherwise enforce tag 35 as the first body field) before typed decoders route the message.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f213ee7: check_envelope_shape now requires p.fields[2].tag == kTagMsgType, so a header like 8/9/34/35/.../10 is rejected as Malformed. New test: "FIX MsgType must be the first body field".
CodeScene's delta gate scored src/protocol/fix.cpp at 8.02 (decode_new_order: Complex Method / Complex Conditional / Overall Code Complexity). Restructure without changing behavior: - Extract map_side / map_ord_type / map_tif (the enum-code switches) and expect_msg_type (the tag-35 check) as small helpers. - Add a FieldReader accumulator that reads required integer/coded fields and short-circuits on the first error, so decode_new_order and decode_cancel_order become a flat fluent chain plus a single error check instead of a long if-return ladder with three inline switches. make check 261/261; make asan 261/261; FIX tests 19 cases / 140 assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Clear the last two CodeScene flags on scripts/flamegraph.py: - _clean_symbol: replace the balanced-paren dso scan (a deep nested loop) with a flat regex _DSO_RE. perf prints a space before the "(dso)" and dso strings never contain parens, so a non-nested " (...)$" match is exact and won't strip a C++ signature's own parentheses. - _layout: drop the unused `total` parameter (5 args -> 4). Output unchanged; tests/shell/test_flamegraph.sh 19/19. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Provenance input changed; regenerate from clean tree (416 samples, Dirty inputs: no). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ene) Clear the remaining CodeScene flags on src/protocol/fix.cpp (Code Duplication, Complex Method, Complex Conditional): - Extract a decode_typed<T> skeleton (validate envelope -> confirm MsgType -> fill body via FieldReader -> error check) so decode_new_order and decode_cancel_order collapse to just their field maps, removing the duplicated prologue/epilogue. - Split parse_envelope into tokenize / check_envelope_shape / verify_length_and_checksum, each a small single-purpose function, and fold the 8/9/10 ordering check into a named bool. Behavior unchanged: make check 261/261, make asan 261/261, FIX tests 19 cases / 140 assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Clear the last two CodeScene advisory flags on src/protocol/fix.cpp:
- Code Duplication: the three near-identical map_side/map_ord_type/map_tif
switches are replaced by one generic FieldReader::coded(tag, out, table) that
looks a code up in a small constexpr {code, enum} table; the decoder supplies
the per-enum tables. No per-enum mapping duplication remains.
- Complex Conditional: expect_msg_type's 3-term `||` is split so each branch has
a single operator.
make check 261/261; make asan 261/261; FIX tests 19 cases / 140 assertions.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb8f99ce9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [[nodiscard]] const Field *find_field(const Parsed &p, unsigned tag) noexcept { | ||
| for (std::size_t i = 0; i < p.count; ++i) { | ||
| if (p.fields[i].tag == tag) { | ||
| return &p.fields[i]; | ||
| } |
There was a problem hiding this comment.
For messages that repeat a required business tag, all typed decoders take the first occurrence via this lookup and silently ignore later conflicting values (for example 55=2 followed by 55=999 still decodes as symbol 2). Since this adapter does not implement repeating groups and documents a fixed one-to-one field map for these order messages, accepting duplicate tags makes malformed client input ambiguous instead of returning a deterministic parse error; track seen tags during tokenization/envelope validation and reject duplicates outside any explicitly supported repeated context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f213ee7: tokenize now rejects any repeated tag (no repeating groups in this adapter), so 55=2 then 55=999 is Malformed rather than first-wins. New test: "FIX duplicate tag rejects deterministically".
The decision-log entry and bottom "Next action remains" block already covered this follow-up, but the top `## Current state` bullets — the first thing `/resume` reads — still presented the v0.2.0 release (PR #127) as the "Last action". A resuming agent could therefore miss that this resume-anchor / PMU sync already happened and duplicate it. Record the sync as the current "Last action" and "Last completed docs sync", demoting the v0.2.0 release detail to a "Prior action" bullet. Docs-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address five Codex review findings on the flamegraph driver: 1. Classify `zero-sized data` (perf script's no-sample report) as a perf limitation, matching scripts/perf_record.sh, so the documented QSL_PERF_ALLOW_PARTIAL=1 constrained-host path works instead of tripping the unexpected-failure exit. 2. Remove any prior results/flamegraph.svg when a partial run captures no folded stacks, so a constrained rerun cannot leave a previous host's SVG beside a .txt that says there is no sample report. 3. Accept perf's `(~N samples)` estimate marker (optional `~`), and base the minimum-sample gate on the authoritative folded sample total rather than perf record's self-described estimate. Report both counts. 4. Capture flamegraph.py --collapse-only's exit status instead of `|| true`; a renderer/parser failure now exits 4 (unmaskable) rather than being published as a constrained-environment artifact. 5. Derive the sampling-kind label/caveat from the selected event (software cpu-clock/task-clock vs hardware-PMU) so the artifact type, SVG comment, and text companion stay consistent for QSL_FLAMEGRAPH_EVENT=cycles etc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bare-metal Apple M2 (aarch64) Fedora Asahi, cpu-clock @ 4000Hz: 329 folded samples / 159 stacks, classified `flamegraph (software cpu-clock sampling hot-symbol profile)`, `Dirty inputs: no`. Source digest now covers the hardened scripts/flamegraph.sh; the .txt reports both the folded total and perf record's estimate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…odex #131) Two Codex review findings on the FIX adapter's parser strictness: - check_envelope_shape now requires MsgType (35) as the first body field, immediately after BodyLength, so a non-standard header like 8/9/34/35/.../10 is rejected as Malformed instead of decoding via a first-match scan. - tokenize now rejects any repeated tag. This adapter maps each business tag exactly once (no repeating groups), so a duplicate such as `55=2` then `55=999` is an ambiguous/malformed frame rather than a silently-ignored later value. Adds a deterministic rejection test for each. (The earlier ClOrdID-required finding was already resolved by 3e4c8e3 and is covered by an existing test.) make check 263/263, make asan 263/263. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The decision log marks #29 (FIX adapter) closed by this PR, but the current-state / resume anchors in PROGRESS.md and HANDOFF.md still listed it as open backlog, so /resume could send the next session to re-implement work this PR just added. Remove #29 from those backlog lists and note it as delivered in this PR. (#32 stays listed here; the v0.2.1 release PR removes it as part of its release sweep.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Propagate the #129/#130/#131 Codex fixes up the stack and resolve the v0.2.1 release-anchor Codex findings (#133): - Reconcile the PROGRESS/HANDOFF backlog conflict to the release state: #29 (FIX adapter) and #32 (flamegraph) are both done, shipped in v0.2.1; no open legacy backlog remains. - Sync AGENTS.md and CLAUDE.md released-state anchors from v0.2.0 to v0.2.1, recording the #129/#130/#131 content (the canonical files a resume reads). - Refresh docs/release_readiness.md for v0.2.1: 263/263 tests, the FIX-adapter and flamegraph coverage, the bare-metal flamegraph artifact, and the next release re-pointed to v0.2.1. - Update the v0.2.1 current-state test count 261 -> 263 (the two new FIX rejection tests) and add a decision-log entry for this second Codex round. make check / make asan 263/263 on the bare-metal Apple M2 Fedora Asahi host. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… profile (Codex #129) The constraints bullet labeled all perf artifacts as partial hardware PMU evidence, but only results/perf_stat_linux.txt carries real PMU counters (cycles/instructions/branches/branch-misses). results/perf_report_linux.txt is a software cpu-clock sampling profile, not PMU evidence. Scope the claim to the perf-stat artifact and call out perf-record separately, identically in AGENTS.md and CLAUDE.md so the two memories stay in sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The flamegraph artifact, generator, provenance companion, and docs already existed but no page actually displayed the SVG — it was only referenced by filename. Embed the rendered results/flamegraph.svg as a visible image under the Benchmarks section, with a caption that classifies it honestly as a software cpu-clock sampling hot-symbol profile (not PMU evidence), names the hot frames, and links the provenance .txt and docs/perf_analysis.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…l-adapter # Conflicts: # PROGRESS.md
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
…), anchor sweep (#133) * docs: sync resume anchors and PMU claims to v0.2.0 (Codex #127/#128 follow-up) Resolve the Codex review findings left on `main` by PRs #127 and #128: - PROGRESS.md: remove the stale "Next action remains" block that still steered /resume to the merged PR #125 on `perf/linux-host-artifact-refresh`; replace with the v0.2.0 between-releases state (no active milestone; #94/#90 gated). - AGENTS.md: bring it into sync with CLAUDE.md's v0.2.0 partial-PMU reframe. The constraints bullet, the "correct claim" block, and the "M29 perf evidence status" subsection no longer label the artifacts "constrained Docker validation"; the stale `perf/linux-host-artifact-refresh` follow-up line is updated (also in CLAUDE.md) to the released state. - docs/perf_analysis.md: narrow the PMU claim so it no longer implies the Apple Blizzard (E-core) PMU carries live counts. The `apple_blizzard_pmu/...` rows read `<not counted>` in results/perf_stat_linux.txt because the single-threaded benchmark stays on the Avalanche P-cores — expected scheduling, not a counter. Docs/memory only; no code or artifacts changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: add flamegraph generator and make target (#32) Add `make flamegraph`, the missing-flamegraph follow-up tracked by issue #32. The perf stat/record text workflow already existed; this renders a perf call-graph flamegraph. - scripts/flamegraph.sh: records `perf record --call-graph dwarf -F 4000 -g -e cpu-clock` on qsl-bench and writes results/flamegraph.svg plus a results/flamegraph.txt provenance/classification companion (top folded stacks). Mirrors perf_record.sh: Linux-only, reuses qsl_common.sh provenance + qsl_publish_artifact, and honours QSL_PERF_ALLOW_PARTIAL for constrained hosts. DWARF call graphs unwind correctly despite the Release `bench` preset omitting frame pointers. - scripts/flamegraph.py: dependency-free (stdlib-only) stackcollapse + SVG renderer, so the artifact is reproducible from the repo without vendoring the Perl FlameGraph toolkit. Deterministic: frames sorted by name, colors a pure function of the name, no RNG/timestamps in the drawn body. - tests/shell/test_flamegraph.sh: CTest-registered (python3-only, skips cleanly if absent) — folding (offset/dso stripping, perf-order reversal, comm-at-base, count aggregation, sortedness), SVG well-formedness, XML escaping, determinism, empty-input handling. - docs (perf_analysis.md, results/README.md), command lists (CLAUDE.md, AGENTS.md), MILESTONES.md backlog, PROGRESS.md log. `make check` 242/242. Full hardware cache-PMU evidence stays in #90. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: add generated flamegraph artifact on bare-metal Fedora Asahi (#32) results/flamegraph.svg + results/flamegraph.txt generated by `make flamegraph` from the clean committed tree on the bare-metal Apple M2 (aarch64) Fedora Asahi host: 397 cpu-clock samples, 171 folded stacks, `Dirty inputs: no`. The hot paths resolve to real engine symbols (OrderBook::modify/cancel/add_limit, the dispatch_storage cancel path, decode_new_order, the gateway Session path, replay::generate_flow). Software cpu-clock sampling hot-symbol profile — not a latency/throughput claim; full hardware cache-PMU evidence stays in #90. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat: add FIX-like text protocol adapter (#29) Add a human-readable `tag=value` text protocol alongside the binary codec, mapping the same internal message structs (issue #29, reprioritized by the human from the backlog). - include/qsl/protocol/fix.hpp + src/protocol/fix.cpp: SOH-framed `tag=value` adapter with genuine FIX framing — 8 BeginString / 9 BodyLength / 35 MsgType / ... / 10 mod-256 CheckSum — for the client->gateway order path: NewOrderSingle (35=D) -> NewOrder, OrderCancelRequest (35=F) -> CancelOrder. Decoding is total, deterministic, and noexcept (fixed field table, std::from_chars, std::string_view; no heap on the decode path) and reports every malformed input through a FixError taxonomy mirroring the binary codec's DecodeError. Documented, deliberate simplifications: Symbol (55) carries the numeric SymbolId; Price (44) carries integer ticks and is always present, so NewOrder<->FIX is a lossless bijection like the binary codec (price is never a float). - tests/unit/test_fix_protocol.cpp: mirrors the binary required tests and adds a cross-codec equivalence test (binary and FIX decode the same order to identical structs across all Side x OrdType x TIF), a byte-pinned fixture, and rejection of malformed framing / unsupported BeginString / unknown-or-wrong MsgType / BodyLength mismatch / CheckSum mismatch / missing field / invalid field / invalid enum / out-of-range / oversized messages. - docs/fix_protocol.md (+ pointer from docs/binary_protocol.md); MILESTONES.md and PROGRESS.md updated. make check 260/260; make asan 260/260 (the adapter parses untrusted text). Closes #29. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: harden flamegraph collapsed-stack parsing (Codex review) Address two Codex review findings in scripts/flamegraph.py::parse_collapsed: - Prefer a tab separator when present so a tab-separated folded line whose stack contains spaces (C++ signatures) splits on the trailing count instead of an interior space and is silently dropped. - Ignore non-positive sample counts, so hand-crafted --from-collapsed input with 0/negative counts cannot render a misleading SVG (all-non-positive input now fails with exit 1 via the existing empty-folded guard). Adds test coverage in tests/shell/test_flamegraph.sh (19/19). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: regenerate flamegraph artifact after parser hardening flamegraph.py is a provenance input, so regenerate results/flamegraph.svg + .txt from the clean tree to keep the Source digest consistent (423 samples, Dirty inputs: no). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: enforce FIX-required ClOrdID on OrderCancelRequest (Codex review) decode_cancel_order validated only OrigClOrdID (41) and Symbol (55), so a 35=F message missing ClOrdID (tag 11) — which FIX requires and encode() emits — was accepted. Validate tag 11 (present and numeric) on decode without storing it, keeping decode symmetric with encode. Adds a rejection test and clarifies the docs/fix_protocol.md note for tag 11. make check 261/261; make asan 261/261. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: improve flamegraph.py code health (CodeScene gate) CodeScene's delta gate scored scripts/flamegraph.py at 7.81 (render_svg: Large Method + Excess Arguments + complexity; fold_perf_script: Bumpy Road / nested complexity). Restructure without changing output: - fold_perf_script: move per-line state into a small _Folder helper so the parsing loop is a flat if/elif/else instead of a nested block. - render_svg: bundle styling knobs into a FlameOptions dataclass (2 args, was 7) and extract _append_chrome, _frame_svg, _truncate; geometry constants (_SIDE/_PAD_TOP/_PAD_BOTTOM) hoisted to module scope and a _Canvas dataclass carries derived geometry. Emitted SVG/collapsed bytes are unchanged; tests/shell/test_flamegraph.sh 19/19. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: regenerate flamegraph artifact after code-health refactor flamegraph.py is a provenance input; regenerate results/flamegraph.svg + .txt from the clean tree (402 samples, Dirty inputs: no). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: reduce decode_new_order complexity in fix.cpp (CodeScene gate) CodeScene's delta gate scored src/protocol/fix.cpp at 8.02 (decode_new_order: Complex Method / Complex Conditional / Overall Code Complexity). Restructure without changing behavior: - Extract map_side / map_ord_type / map_tif (the enum-code switches) and expect_msg_type (the tag-35 check) as small helpers. - Add a FieldReader accumulator that reads required integer/coded fields and short-circuits on the first error, so decode_new_order and decode_cancel_order become a flat fluent chain plus a single error check instead of a long if-return ladder with three inline switches. make check 261/261; make asan 261/261; FIX tests 19 cases / 140 assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: flatten flamegraph.py remaining complexity (CodeScene) Clear the last two CodeScene flags on scripts/flamegraph.py: - _clean_symbol: replace the balanced-paren dso scan (a deep nested loop) with a flat regex _DSO_RE. perf prints a space before the "(dso)" and dso strings never contain parens, so a non-nested " (...)$" match is exact and won't strip a C++ signature's own parentheses. - _layout: drop the unused `total` parameter (5 args -> 4). Output unchanged; tests/shell/test_flamegraph.sh 19/19. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: regenerate flamegraph artifact after complexity flattening Provenance input changed; regenerate from clean tree (416 samples, Dirty inputs: no). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: remove decoder duplication and split parse_envelope (CodeScene) Clear the remaining CodeScene flags on src/protocol/fix.cpp (Code Duplication, Complex Method, Complex Conditional): - Extract a decode_typed<T> skeleton (validate envelope -> confirm MsgType -> fill body via FieldReader -> error check) so decode_new_order and decode_cancel_order collapse to just their field maps, removing the duplicated prologue/epilogue. - Split parse_envelope into tokenize / check_envelope_shape / verify_length_and_checksum, each a small single-purpose function, and fold the 8/9/10 ordering check into a named bool. Behavior unchanged: make check 261/261, make asan 261/261, FIX tests 19 cases / 140 assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: table-driven enum maps + simpler msg-type check (CodeScene) Clear the last two CodeScene advisory flags on src/protocol/fix.cpp: - Code Duplication: the three near-identical map_side/map_ord_type/map_tif switches are replaced by one generic FieldReader::coded(tag, out, table) that looks a code up in a small constexpr {code, enum} table; the decoder supplies the per-enum tables. No per-enum mapping duplication remains. - Complex Conditional: expect_msg_type's 3-term `||` is split so each branch has a single operator. make check 261/261; make asan 261/261; FIX tests 19 cases / 140 assertions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * release: v0.2.1 — FIX text protocol adapter, perf flamegraph, anchor sweep Bump the project version 0.2.0 -> 0.2.1 and record the release. - CMakeLists.txt: project VERSION 0.2.1. - CHANGELOG.md: new [0.2.1] section — FIX-like text protocol adapter (#29), `make flamegraph` + dependency-free renderer (#32), and the Codex resume-anchor / PMU-claim consistency sweep (#127/#128 follow-up); [Unreleased] reset. - PROGRESS.md / HANDOFF.md: release and resume anchors brought to the v0.2.1 released state; #29 and #32 marked done. No code or benchmark artifacts change in this release PR. On squash-merge, tag `v0.2.1` on the merge commit and publish the GitHub release. make check 261/261. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: record resume-anchor sync in PROGRESS current-state (Codex #129) The decision-log entry and bottom "Next action remains" block already covered this follow-up, but the top `## Current state` bullets — the first thing `/resume` reads — still presented the v0.2.0 release (PR #127) as the "Last action". A resuming agent could therefore miss that this resume-anchor / PMU sync already happened and duplicate it. Record the sync as the current "Last action" and "Last completed docs sync", demoting the v0.2.0 release detail to a "Prior action" bullet. Docs-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: harden flamegraph.sh classification + sample gating (Codex #130) Address five Codex review findings on the flamegraph driver: 1. Classify `zero-sized data` (perf script's no-sample report) as a perf limitation, matching scripts/perf_record.sh, so the documented QSL_PERF_ALLOW_PARTIAL=1 constrained-host path works instead of tripping the unexpected-failure exit. 2. Remove any prior results/flamegraph.svg when a partial run captures no folded stacks, so a constrained rerun cannot leave a previous host's SVG beside a .txt that says there is no sample report. 3. Accept perf's `(~N samples)` estimate marker (optional `~`), and base the minimum-sample gate on the authoritative folded sample total rather than perf record's self-described estimate. Report both counts. 4. Capture flamegraph.py --collapse-only's exit status instead of `|| true`; a renderer/parser failure now exits 4 (unmaskable) rather than being published as a constrained-environment artifact. 5. Derive the sampling-kind label/caveat from the selected event (software cpu-clock/task-clock vs hardware-PMU) so the artifact type, SVG comment, and text companion stay consistent for QSL_FLAMEGRAPH_EVENT=cycles etc. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * perf: regenerate flamegraph artifact after classification hardening Bare-metal Apple M2 (aarch64) Fedora Asahi, cpu-clock @ 4000Hz: 329 folded samples / 159 stacks, classified `flamegraph (software cpu-clock sampling hot-symbol profile)`, `Dirty inputs: no`. Source digest now covers the hardened scripts/flamegraph.sh; the .txt reports both the folded total and perf record's estimate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: enforce FIX envelope MsgType position + reject duplicate tags (Codex #131) Two Codex review findings on the FIX adapter's parser strictness: - check_envelope_shape now requires MsgType (35) as the first body field, immediately after BodyLength, so a non-standard header like 8/9/34/35/.../10 is rejected as Malformed instead of decoding via a first-match scan. - tokenize now rejects any repeated tag. This adapter maps each business tag exactly once (no repeating groups), so a duplicate such as `55=2` then `55=999` is an ambiguous/malformed frame rather than a silently-ignored later value. Adds a deterministic rejection test for each. (The earlier ClOrdID-required finding was already resolved by 3e4c8e3 and is covered by an existing test.) make check 263/263, make asan 263/263. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: drop delivered #29 from open-backlog anchors (Codex #131) The decision log marks #29 (FIX adapter) closed by this PR, but the current-state / resume anchors in PROGRESS.md and HANDOFF.md still listed it as open backlog, so /resume could send the next session to re-implement work this PR just added. Remove #29 from those backlog lists and note it as delivered in this PR. (#32 stays listed here; the v0.2.1 release PR removes it as part of its release sweep.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: scope partial-PMU claim to perf-stat; perf-record is a software profile (Codex #129) The constraints bullet labeled all perf artifacts as partial hardware PMU evidence, but only results/perf_stat_linux.txt carries real PMU counters (cycles/instructions/branches/branch-misses). results/perf_report_linux.txt is a software cpu-clock sampling profile, not PMU evidence. Scope the claim to the perf-stat artifact and call out perf-record separately, identically in AGENTS.md and CLAUDE.md so the two memories stay in sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs: embed the flamegraph as a visible image in the README The flamegraph artifact, generator, provenance companion, and docs already existed but no page actually displayed the SVG — it was only referenced by filename. Embed the rendered results/flamegraph.svg as a visible image under the Benchmarks section, with a caption that classifies it honestly as a software cpu-clock sampling hot-symbol profile (not PMU evidence), names the hot frames, and links the provenance .txt and docs/perf_analysis.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fc2f35ea5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cache-counter PMU evidence — needs a PMU microarchitecture that exposes cache events, e.g. | ||
| x86_64). Low-signal backlog: #32 (flamegraph), #29 (FIX adapter). | ||
| x86_64). #29 (FIX-like text protocol adapter) is delivered in this PR | ||
| (`feat/fix-text-protocol-adapter`). Low-signal backlog: #32 (flamegraph). |
There was a problem hiding this comment.
When resuming from this commit, this current-state anchor still tells agents that #32 (flamegraph) is remaining even though this stack is based on the flamegraph commit and the same file already records the flamegraph artifact as completed later in the history section. Because /resume is driven from these anchors, the next session can be sent back to a delivered issue; remove #32 from the backlog here or mark it as delivered in the current-state text.
Useful? React with 👍 / 👎.
| in `docs/fix_protocol.md` (+ pointer from `docs/binary_protocol.md`). `make check` 260/260 and | ||
| `make asan` 260/260 clean (the parser handles untrusted text). Closes #29. Do not merge from |
There was a problem hiding this comment.
This new history entry records the FIX adapter as the latest work with make check/make asan at 260/260, but the Current state block above still says the last action was the earlier docs sync and that verification is 241/241. Since /resume reads that top block before the detailed history, the next session will start from stale action and test-count anchors; update the Current state fields alongside this completion entry.
Useful? React with 👍 / 👎.
| append_field(body, kTagMsgSeqNum, seq); | ||
| append_field(body, kTagClOrdID, msg.order_id); | ||
| append_field(body, kTagSymbol, static_cast<std::uint64_t>(msg.symbol)); | ||
| append_field(body, kTagSide, msg.side == Side::Buy ? "1" : "2"); |
There was a problem hiding this comment.
If a caller ever passes a NewOrder with an out-of-range Side value (for example from a bad cast or corrupted test fixture), this conditional emits 54=2 and turns it into a valid Sell order; the same pattern below coerces unknown OrdType/TimeInForce to valid codes. The binary codec preserves enum bytes so a round-trip rejects invalid values, but the FIX path silently changes malformed internal input into a different valid order, which can corrupt experiment results instead of failing deterministically.
Useful? React with 👍 / 👎.
1 participant
Summary
Closes #29 — an optional, human-readable FIX-like
tag=valuetext protocol alongside the binary codec, mapping the same internal message structs. The human reprioritized this backlog item.What's added
include/qsl/protocol/fix.hpp+src/protocol/fix.cpp— a SOH-delimitedtag=valueadapter with genuine FIX framing (8BeginString /9BodyLength /35MsgType / … /10mod-256 CheckSum) for the client→gateway order path:35=D) →NewOrder35=F) →CancelOrdernoexcept: a fixed field table,std::from_chars, andstd::string_view(no heap allocation on the decode path), reporting every malformed input through aFixErrortaxonomy that mirrors the binary codec'sDecodeError.tests/unit/test_fix_protocol.cpp— mirrors the binary codec's required tests and adds:8=FIX.4.2|9=50|35=D|…|10=164|) so any change to field order or the checksum/body-length math fails the build;int64price anduint64id/seq round-trips.docs/fix_protocol.md(+ pointer fromdocs/binary_protocol.md),MILESTONES.md,PROGRESS.md.Deliberate, documented simplifications
Chosen so the adapter is a lossless map onto the simulator's internal model (see
docs/fix_protocol.md):SymbolId(the engine keys on it), not a ticker string.NewOrder ↔ FIXis a bijection exactly like the binary codec. Price is never floating point.Verification
make check→ 260/260.make asan→ 260/260 (ASan/UBSan; the adapter parses untrusted text).🤖 Generated with Claude Code