PnL attribution narrator (attribute_pnl + narrate_attribution)#109
Open
sujitn wants to merge 13 commits into
Open
PnL attribution narrator (attribute_pnl + narrate_attribution)#109sujitn wants to merge 13 commits into
sujitn wants to merge 13 commits into
Conversation
Phase 1-3 design artifacts for the PnL attribution narrator extension to the hedge advisor. Key findings: - Gap 0 retired: price_from_mark already takes the valuation date (settlement), so the pricing core is untouched. - Zero new crates: new risk::pnl module tree + two convex-mcp tools. - Locked: ISO-8601 Date strings, fixed-maturity swap model, decomposition as an analytics function, path-ordered waterfall.
New risk::pnl module (sibling of risk::hedging). Commit 1 of 6. - Attribution / PositionAttribution / FactorPnl / PnlFactor / CurveBreakdown — schema-derived wire types, units in field names, Decimal via schemars(with="f64"), serde(default) on optional vecs. - SwapPnlSpec: fixed-maturity/fixed-rate swap (pinned at trade) — distinct from the constant-maturity hedge-advisor InterestRateSwap. - AttributionConfig: two knobs only (pivot, analysis grid). - AttributionProvenance: deterministic, no timestamp. - types module kept private to pnl to avoid a prelude glob collision with hedging::types; items re-exported. 6 tests (serde round-trip, snake_case, schema derivation). Clippy + fmt clean.
Gate decision (Commit 1 review): name the type to pair explicitly with the hedge advisor's InterestRateSwap and to signal it is the fixed-maturity counterpart used by attribution. No vanilla/butterfly taxonomy — a swap butterfly is modeled as multiple book positions, not an instrument variant.
Commit 2 of 6. risk::pnl::decompose.
Inverse of the forward shock primitives: projects the realised
Δr(τ) = r_t1(τ) − r_t0(τ) onto a {level, slope, curvature} basis by
least squares (3×3 normal equations via convex_math::solve_linear_system
+ nalgebra — the same pattern key_rate_futures uses). The unexplained
per-tenor part is the reported fit residual.
- Single-source basis_row() used by both the fit and component-curve
reconstruction (component_shift_decimal) so they cannot drift.
- Slope = linear-about-pivot; curvature = symmetric tent (belly +1,
far wing -1). Loadings in bp.
- convex-curves untouched (component curves rebuilt via the closure
the engine will hand to ScenarioBump::custom in commit 3).
7 tests: pure parallel/slope/curvature recovery, identical→zero,
kink→nonzero residual, exactness (components+residual==move to 1e-12),
too-few-tenors error. clippy (neg_cmp fix) + fmt clean.
Commit 3 of 6 (the heart). risk::pnl::engine. Path-ordered waterfall per position: carry -> roll-down -> curve (parallel/slope/curvature/residual) -> spread -> residual. Reuses the pricing core unchanged (price_from_mark already takes the valuation date); held-spread reprices via ZSpreadCalculator::price_with_spread (no root-find). - Held spread taken EXACTLY from a Z-spread mark (no rounding) so the residual is machine-zero for spread-marked positions; price/yield marks fall back to the rounded solve (documented, lands in residual). - Swap = its fixed leg priced Z-flat, run through the identical bond waterfall, signed by side (PayFixed short the fixed leg). Fixed maturity pinned at trade (gap-4 fix). Floating ~ par at reset. - Curve sub-factors repriced on curve_t0 + ScenarioBump::custom component shifts from the decomposition (single basis source). - Book roll-up: per-factor sum, spread expanded per benchmark, PnlFactor::ORDER, single-currency validation. ResolvedPosition::Bond boxed (large_enum_variant). 5 tests: zero-move=>zero, identity closure, parallel=>curve_parallel (within 10% of analytic DV01), pay-fixed swap offsets long bond on a rate rise (the hero-moment guard), currency/empty rejection. convex-analytics: 350/350 lib tests, clippy + fmt clean.
The hedge-advisor book + the pay-fixed EUR swap, May 7->8 2026, rates +6bp with mild steepening. Asserts: 4 positions, factor identity closes, long sovereigns lose, pay-fixed swap gains and offsets (the hero moment), swap spread factor ~ 0. PNL_DUMP=1 prints the Attribution JSON. Doubles as regression guard.
Commit 4 of 6. risk::pnl::narrate::narrate_attribution.
Clones the hedge-advisor narrate() style verbatim: pre-sized String,
std::fmt::Write, Currency::code(), bp {:.2} / ccy {:.0}, provenance
disclosure tail. No style enum — matches the shipped narrator (a
one-variant enum is the speculative generality the plan's anti-slop
review rejects; deviates from the prompt's "optional style enum" by
design).
States: total (ccy + bp), biggest driver, curve decomposition, per
non-zero benchmark spread move (widened/tightened), and the hero
clause — pay-fixed swap absorbing N% of the bonds' move, fired
deterministically when a swap's PnL opposes the bonds'. Zero/None
spread rows the engine keeps are filtered by the narrator (engine
complete, narrator selective).
8 tests: total in ccy+bp, biggest driver, BTP/OAT widening (exact
count, no false DE.BUND match), hero clause, bonds-only (no clause),
determinism, empty positions, provenance. 27 pnl tests, clippy + fmt
clean.
Commit 5 of 6. Two #[tool] methods on the existing #[tool_router] impl,
mechanically identical to the four hedge-advisor tools.
- AttributePnlParams: flat positions: Vec<PnlPositionParams> (tagged
bond|swap like HedgeInstrument) + base_currency + t0/t1 ISO strings
+ curve_t0/curve_t1 CurveRef + optional config. Bonds resolved via
the existing resolve_bond (Fixed direct, Callable via base bond);
marks parsed via Mark::from_str; reuses finite_decimal/resolve_curve.
- narrate_attribution returns NarrationOutput { text }.
- pnl public API threaded through risk::mod, analytics prelude, and the
convex umbrella's explicit risk re-export list.
e2e test pnl_narrator_e2e_oat_book: the 4-position demo book (inline
EUR curves + bonds + swap) through attribute_pnl -> narrate_attribution;
asserts 4 positions, provenance, factor identity closes, long sovereigns
lose, pay-fixed swap gains and offsets, narration surfaces the hero
moment.
convex-mcp 32/32, convex-analytics 350/350 (schemars), clippy + fmt
clean across convex-analytics/convex/convex-mcp.
Commit 6 of 6. Definition-of-done wrap. - benches/hedge_advisor.rs: bench_pnl group on the 4-position demo book (extends the existing bench — no new [[bench]] entry). Medians: attribute_pnl_demo_book ~128 µs, +narrate ~138 µs (well under 1 ms). - docs/perf-baselines.md: PnL numbers recorded. Regression check — risk_profile_apple_10y flat at ~22.6 µs (the shared pricing/risk/KRD path the PnL engine reuses is provably unaffected). propose_five / end_to_end drift vs the stale 0.12.1 doc baseline is pre-existing (dep-bump PR #107, predates this branch; bench code byte-identical to main) — re-baselined to 0.13.0 HEAD with the attribution spelled out. No >5% regression caused by this PR. - README.md: PnL Narrator section + feature bullet, mirrors Hedge Advisor. - docs/pnl-narrator-investigation.md: v1-shipped status banner, gaps flipped to closed.
Self-review remediation. Two defects + slop cleanup. #1 Narrator no longer editorializes. It stated "last week's hedge working as designed" — asserting intent it cannot know. Now reports the measured fact only: "Swap positions contributed X, offsetting Y% of the bonds' rate-driven move." "hero moment" prose removed from doc comments and test names. #2 Z-rounding fixed at source. ZSpreadCalculator::calculate rounded the implied Z to integer bp (zspread.rs), silently degrading every consumer — including the shipped hedge advisor's compute_position_risk (held that rounded z for all KRD bumps). Now returns full precision; callers round for display if needed. This let the PnL engine drop its Z-mark special-case entirely (held_spread is now one path). Blast radius verified: convex-analytics 351, convex-ffi 52, convex-mcp 32, workspace build all green (the zspread tests use tolerances). #3 Least squares now uses nalgebra SVD, not normal equations (BtB squares the condition number). Basis documented honestly as a reporting parameterization; bucketed-KRD attribution noted as the more standard v2 alternative. #4-7 slop cut: - Deleted the entire types.rs test module (6 serde/derive/language tests) + 2 trivial narrate tests (deterministic, discloses_provenance). - Trimmed engine.rs module doc 29->11 lines; removed PNL_DUMP env branch from the fixture test. - Dropped unused Eq/Hash from PnlFactor. - Removed AttributionConfig.analysis_tenors (YAGNI + a latent grid-vs-pillars inconsistency); the move is always decomposed on curve_t0's own pillars. - Made CurveDecomposition.residual_by_tenor private — the surfaced diagnostic is the L1 norm; per-tenor detail nothing consumed. - Hoisted the curve decomposition out of the per-position loop (it is bond-independent — was recomputed N times).
📝 WalkthroughWalkthroughAdds a two-date PnL attribution feature: SVD-based curve decomposition, sequential-repricing waterfall for bonds and fixed‑maturity swaps, deterministic narration, MCP tools, benches, and documentation; exposes stable wire types and crate-level re-exports. ChangesPnL Narrator End-to-End Implementation
Sequence Diagram: PnL Attribution WorkflowsequenceDiagram
participant Client
participant MCP as MCP Server
participant Engine as attribute_pnl
participant Decomp as decompose_curve_move
participant Narrate as narrate_attribution
Client->>MCP: attribute_pnl(positions, t0, t1, curve_t0, curve_t1)
MCP->>Engine: attribute_pnl(resolved_book, ...)
Engine->>Decomp: decompose_curve_move(curve_t0, curve_t1, ...)
Decomp-->>Engine: CurveDecomposition (parallel/slope/curvature + residual)
Engine->>Engine: per-position sequential repricing waterfall
Engine-->>MCP: Attribution (factors, totals, provenance)
MCP-->>Client: Attribution JSON
Client->>MCP: narrate_attribution(attribution)
MCP->>Narrate: narrate_attribution(&attribution)
Narrate-->>MCP: narration text
MCP-->>Client: narration text
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/convex-analytics/src/risk/pnl/decompose.rs`:
- Around line 115-133: Ensure the decomposition validates the basis before
solving in decompose_curve_move: verify analysis_tenors contains at least 3
distinct values (not just len >= 3) and compute distinct_count from
analysis_tenors; if distinct_count < 3 return AnalyticsError::InvalidInput. Also
ensure the pivot value is strictly inside the tenor span (pivot > tmin && pivot
< tmax) because a pivot outside or on the bounds makes curvature affine; if not,
return AnalyticsError::InvalidInput. These checks (on analysis_tenors
distinctness and pivot in (tmin,tmax)) should be performed before forming the
design matrix / calling the SVD so you fail fast rather than getting a
minimum-norm SVD solution with non-unique factor loadings.
In `@crates/convex-analytics/src/risk/pnl/engine.rs`:
- Around line 91-97: The bps calculation (function bps and other *_bps usages)
is unstable because it uses market_value_t0 / book_market_value_t0 which can be
zero/negative for swaps, shorts, or netted books; change it to use a single
explicit positive base denominator (e.g., denom = max(abs(base_ccy),
SMALL_POSITIVE_EPS)) and apply that same rule consistently wherever *_bps is
computed (position-level and book-level, including uses of market_value_t0 and
book_market_value_t0) so the basis points are always relative to a positive,
non-zero base; update the bps helper and all call sites (the *_bps computations
referenced in the diff) to use this sanitized absolute base value.
In `@crates/convex-analytics/src/risk/pnl/narrate.rs`:
- Around line 96-117: The current text incorrectly calls bond_pnl a
"rate-driven" move even though bond_pnl sums full bond totals; update the
message in the write! call (the block that computes swap_pnl, bond_pnl,
offset_pct and writes to out) so it either compares swap_pnl to a curve-only
bond PnL field (if one exists on positions — e.g., replace bond_pnl with a
computed bond_curve_pnl from a curve-only value or factor) or, if no curve-only
field is available, simply remove the phrase "rate-driven" and phrasing implying
rates-only (e.g., change "rate-driven move" to "move" or "total move") so the
output accurately reflects what bond_pnl represents.
- Around line 67-94: The code currently infers market direction
("widened"/"tightened") from the sign of FactorPnl.pnl_ccy in the spreads
handling (spreads: Vec<&super::types::FactorPnl> and loop over f) which is
incorrect; change the narration to a neutral factual phrase (e.g. "contributed"
or "impact") instead of inferring direction, or alternatively populate and use
an explicit Δspread_bps field in the FactorPnl payload if available; update the
write! call that uses dir and pnl_ccy (and remove dir logic) to emit a neutral
sentence and/or append the actual delta spread field (Δspread_bps) when present.
In `@crates/convex-mcp/src/server.rs`:
- Around line 1622-1626: The match in resolve_bond/attribute_pnl currently
converts StoredBond::Callable(c) into a FixedRateBond via c.base_bond().clone(),
which silently drops optionality; instead, detect StoredBond::Callable and
return an explicit error (or propagate an Err) so callers fail fast for callable
positions. Update the match arm handling StoredBond::Callable to return an Err
with a clear message (e.g., "callable bonds not supported for attribute_pnl")
from the function (or map to the function's error type) rather than unwrapping
to FixedRateBond; reference StoredBond::Callable, FixedRateBond,
self.resolve_bond and attribute_pnl to locate where to change the behavior.
In `@crates/convex/src/lib.rs`:
- Around line 35-45: The pub re-export list in the convex facade (pub use
convex_analytics::risk::{...}) omits the new PnL API symbols
DEFAULT_PIVOT_TENOR_YEARS and FACTOR_MODEL_NAME; add those two identifiers to
the export list so the top-level convex::* surface mirrors
convex_analytics::risk and downstream callers can access
DEFAULT_PIVOT_TENOR_YEARS and FACTOR_MODEL_NAME via the convex facade (update
the export grouping that includes aggregate_risk_profiles, Attribution, etc., to
include DEFAULT_PIVOT_TENOR_YEARS and FACTOR_MODEL_NAME).
In `@docs/pnl-narrator-investigation.md`:
- Line 53: Several table rows contain unescaped pipe characters inside inline
code spans (e.g., the expression `bond_yield = curve_par_rate@maturity + spread`
and the `SpreadType::ISpread | GSpread` snippet) which breaks Markdown table
rendering; fix by escaping the pipe(s) inside those code spans (replace `|` with
`\|`) or by moving the expression out of the inline code span into plain text or
a code block for those table cells, and apply the same fix to the other affected
table rows referenced in the review.
🪄 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: a33f750a-43cd-40a7-ae56-0033ac2a6508
📒 Files selected for processing (16)
README.mdcrates/convex-analytics/benches/hedge_advisor.rscrates/convex-analytics/src/lib.rscrates/convex-analytics/src/risk/mod.rscrates/convex-analytics/src/risk/pnl/decompose.rscrates/convex-analytics/src/risk/pnl/engine.rscrates/convex-analytics/src/risk/pnl/mod.rscrates/convex-analytics/src/risk/pnl/narrate.rscrates/convex-analytics/src/risk/pnl/types.rscrates/convex-analytics/src/spreads/zspread.rscrates/convex-mcp/src/server.rscrates/convex/src/lib.rsdocs/perf-baselines.mddocs/pnl-narrator-gaps.mddocs/pnl-narrator-investigation.mddocs/pnl-narrator-plan.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/pnl-narrator-plan.md">
<violation number="1" location="docs/pnl-narrator-plan.md:143">
P3: `PositionAttribution.kind` is documented as `&'static str`, but the wire DTO uses `String`; keep docs aligned with the actual serializable schema.</violation>
<violation number="2" location="docs/pnl-narrator-plan.md:284">
P2: The narrator example uses intent language ("working as designed"), which contradicts the measured-facts requirement and existing tests. Keep the swap clause factual only.</violation>
</file>
<file name="crates/convex-analytics/src/risk/pnl/narrate.rs">
<violation number="1" location="crates/convex-analytics/src/risk/pnl/narrate.rs:82">
P2: Inferring "widened/tightened" from spread PnL sign is not reliable and can misstate market direction. Narrate the measured spread contribution instead of direction.</violation>
<violation number="2" location="crates/convex-analytics/src/risk/pnl/narrate.rs:114">
P3: `bond_pnl` here is total bond PnL, not a rates-only figure; reword the sentence to avoid labeling it as a `rate-driven` move.</violation>
</file>
<file name="crates/convex-analytics/src/risk/pnl/decompose.rs">
<violation number="1" location="crates/convex-analytics/src/risk/pnl/decompose.rs:116">
P2: Validate the number of distinct analysis tenors, not just total length; duplicate tenors can make the 3-factor decomposition underdetermined and produce unreliable factor loadings.</violation>
<violation number="2" location="crates/convex-analytics/src/risk/pnl/decompose.rs:129">
P2: Validate `pivot_tenor_years` against the analysis tenor span before fitting. An out-of-range pivot makes the basis rank-deficient and the parallel/slope/curvature loadings become non-unique.</violation>
</file>
<file name="crates/convex-analytics/src/risk/pnl/types.rs">
<violation number="1" location="crates/convex-analytics/src/risk/pnl/types.rs:48">
P2: `InterestRateSwapPnlSpec.notional` is documented as strictly positive but not validated; negative values can silently flip attribution direction.</violation>
<violation number="2" location="crates/convex-analytics/src/risk/pnl/types.rs:137">
P2: `kind` is a free-form `String`; invalid values are accepted and silently misclassified in narration logic. Use a closed enum for `bond|swap` to enforce valid inputs.</violation>
</file>
<file name="crates/convex-mcp/src/server.rs">
<violation number="1" location="crates/convex-mcp/src/server.rs:1625">
P2: Callable bonds are converted to their base fixed bond, which drops optionality and can break callable/OAS attribution semantics.</violation>
</file>
<file name="crates/convex-analytics/src/risk/pnl/engine.rs">
<violation number="1" location="crates/convex-analytics/src/risk/pnl/engine.rs:361">
P2: Reject non-positive swap notional values in attribution; otherwise PnL scaling/sign can be wrong because `side` already encodes direction.</violation>
<violation number="2" location="crates/convex-analytics/src/risk/pnl/engine.rs:401">
P2: Book-level bp normalization should include swap exposure; excluding swaps from `book_mv` causes non-zero swap PnL to collapse to `0 bp` at book level.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| Must state, in order: total PnL (bp **and** ccy); the **largest-magnitude | ||
| factor** by name; the BTP-Bund (and OAT-Bund) **spread move**; an explicit | ||
| **swap clause** — e.g. *"the pay-fixed EUR swap contributed €X, absorbing N% | ||
| of the book's curve move — the hedge from last week working as designed."* |
There was a problem hiding this comment.
P2: The narrator example uses intent language ("working as designed"), which contradicts the measured-facts requirement and existing tests. Keep the swap clause factual only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/pnl-narrator-plan.md, line 284:
<comment>The narrator example uses intent language ("working as designed"), which contradicts the measured-facts requirement and existing tests. Keep the swap clause factual only.</comment>
<file context>
@@ -0,0 +1,453 @@
+Must state, in order: total PnL (bp **and** ccy); the **largest-magnitude
+factor** by name; the BTP-Bund (and OAT-Bund) **spread move**; an explicit
+**swap clause** — e.g. *"the pay-fixed EUR swap contributed €X, absorbing N%
+of the book's curve move — the hedge from last week working as designed."*
+The swap clause fires whenever the book contains a swap whose curve PnL sign
+opposes the bonds' (deterministic `if`, no heuristics).
</file context>
| pub struct PositionAttribution { | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub position_id: Option<String>, | ||
| pub kind: String, |
There was a problem hiding this comment.
P2: kind is a free-form String; invalid values are accepted and silently misclassified in narration logic. Use a closed enum for bond|swap to enforce valid inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/convex-analytics/src/risk/pnl/types.rs, line 137:
<comment>`kind` is a free-form `String`; invalid values are accepted and silently misclassified in narration logic. Use a closed enum for `bond|swap` to enforce valid inputs.</comment>
<file context>
@@ -0,0 +1,196 @@
+pub struct PositionAttribution {
+ #[serde(default, skip_serializing_if = "Option::is_none")]
+ pub position_id: Option<String>,
+ pub kind: String,
+ #[cfg_attr(feature = "schemars", schemars(with = "f64"))]
+ pub market_value_t0: Decimal,
</file context>
| pub side: SwapSide, | ||
| /// Strictly positive; direction lives on `side`. | ||
| #[cfg_attr(feature = "schemars", schemars(with = "f64"))] | ||
| pub notional: Decimal, |
There was a problem hiding this comment.
P2: InterestRateSwapPnlSpec.notional is documented as strictly positive but not validated; negative values can silently flip attribution direction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/convex-analytics/src/risk/pnl/types.rs, line 48:
<comment>`InterestRateSwapPnlSpec.notional` is documented as strictly positive but not validated; negative values can silently flip attribution direction.</comment>
<file context>
@@ -0,0 +1,196 @@
+ pub side: SwapSide,
+ /// Strictly positive; direction lives on `side`.
+ #[cfg_attr(feature = "schemars", schemars(with = "f64"))]
+ pub notional: Decimal,
+ pub currency: Currency,
+}
</file context>
| freq, | ||
| &decomp, | ||
| )?; | ||
| let notional = to_f64(spec.notional, "swap notional")?; |
There was a problem hiding this comment.
P2: Reject non-positive swap notional values in attribution; otherwise PnL scaling/sign can be wrong because side already encodes direction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/convex-analytics/src/risk/pnl/engine.rs, line 361:
<comment>Reject non-positive swap notional values in attribution; otherwise PnL scaling/sign can be wrong because `side` already encodes direction.</comment>
<file context>
@@ -0,0 +1,879 @@
+ freq,
+ &decomp,
+ )?;
+ let notional = to_f64(spec.notional, "swap notional")?;
+ // PayFixed is short the fixed leg → negate.
+ let sign = match spec.side {
</file context>
| } | ||
|
|
||
| book_pnl += pos_pnl_ccy; | ||
| book_mv += if kind == "bond" { base_ccy } else { 0.0 }; |
There was a problem hiding this comment.
P2: Book-level bp normalization should include swap exposure; excluding swaps from book_mv causes non-zero swap PnL to collapse to 0 bp at book level.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/convex-analytics/src/risk/pnl/engine.rs, line 401:
<comment>Book-level bp normalization should include swap exposure; excluding swaps from `book_mv` causes non-zero swap PnL to collapse to `0 bp` at book level.</comment>
<file context>
@@ -0,0 +1,879 @@
+ }
+
+ book_pnl += pos_pnl_ccy;
+ book_mv += if kind == "bond" { base_ccy } else { 0.0 };
+
+ positions.push(PositionAttribution {
</file context>
| pub pivot_tenor_years: f64, pub fit_residual_l1_bps: f64 } | ||
|
|
||
| pub struct PositionAttribution { | ||
| pub position_id: Option<String>, pub kind: &'static str, // "bond" | "swap" |
There was a problem hiding this comment.
P3: PositionAttribution.kind is documented as &'static str, but the wire DTO uses String; keep docs aligned with the actual serializable schema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/pnl-narrator-plan.md, line 143:
<comment>`PositionAttribution.kind` is documented as `&'static str`, but the wire DTO uses `String`; keep docs aligned with the actual serializable schema.</comment>
<file context>
@@ -0,0 +1,453 @@
+ pub pivot_tenor_years: f64, pub fit_residual_l1_bps: f64 }
+
+pub struct PositionAttribution {
+ pub position_id: Option<String>, pub kind: &'static str, // "bond" | "swap"
+ pub total_pnl_ccy: Decimal, pub total_pnl_bps: f64,
+ pub factors: Vec<FactorPnl>, pub curve: CurveBreakdown }
</file context>
All seven verified against current code; six had real impact, one cosmetic. Minimal fixes, validated. 1. decompose_curve_move: validate ≥3 *distinct* tenors (not just len) and a strictly-interior pivot before forming the design matrix — a non-distinct grid or boundary/exterior pivot makes the basis rank-deficient/affine and the SVD returns non-unique loadings. Fail fast. (pub API; the engine path was already safe.) 2. bps(): divide by the absolute base, not the signed one — a short or net-short book previously got sign-inverted bp. Near-zero base still returns 0 rather than dividing by 1e-9 (avoids the explosion the literal max(abs,eps) would cause); the sign-stability is the fix. 3. narrator no longer infers "widened/tightened" from the spread PnL sign (wrong for short credit; no Δspread on FactorPnl) — reports the per-benchmark PnL contribution neutrally. 4. swap-offset clause: "rate-driven move" → "total move" (bond_pnl sums full position totals, not a curve-only component). 5. attribute_pnl rejects callable bonds explicitly instead of silently pricing them as their bullet (v1 has no OAS path here). 6. convex facade: export DEFAULT_PIVOT_TENOR_YEARS / FACTOR_MODEL_NAME for surface parity (cosmetic; no current consumer). 7. investigation.md: escape unescaped pipes in three table code spans (lines 53/80/207) that broke GFM table rendering; the others flagged were delimiters or already escaped. convex-analytics 352, convex-ffi 52, convex-mcp 32; clippy + fmt clean; workspace build green.
Remove prose that restates the design docs or other doc comments; keep every comment that carries a non-obvious contract or rationale. - mod.rs: drop the architecture re-argument (sibling-of-hedging, pricing-core-reused) — it lives in docs/pnl-narrator-*.md; keep the one-line "what it does" + the submodule-privacy maintenance note. - types.rs: tighten the wire-types header; condense the InterestRateSwapPnlSpec rationale 5→4 lines; drop two field docs that restated their struct doc (maturity, fit_residual_l1_bps). Kept (required, not excessive): the sign convention, the PnlFactor taxonomy, the no-timestamp provenance rationale, the decompose basis formula + no-drift invariant + reporting-parameterization caveat, and the field contract docs. Pure doc change — build/clippy/fmt clean, pnl tests 20/20.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/pnl-narrator-investigation.md`:
- Line 18: The blockquote in the markdown contains a blank line causing MD028
failures; fix it by removing the empty line inside the blockquote or replace
that empty line with a quoted blank line using a single '>' so every line in the
quote is prefixed with '>'; update the blockquote in the file (the offending
blockquote that triggers MD028) accordingly and re-run markdown lint to verify
the MD028 error is resolved.
🪄 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: 7433bb6a-14c5-4b8b-a847-9fe20380907b
📒 Files selected for processing (8)
crates/convex-analytics/src/risk/pnl/decompose.rscrates/convex-analytics/src/risk/pnl/engine.rscrates/convex-analytics/src/risk/pnl/mod.rscrates/convex-analytics/src/risk/pnl/narrate.rscrates/convex-analytics/src/risk/pnl/types.rscrates/convex-mcp/src/server.rscrates/convex/src/lib.rsdocs/pnl-narrator-investigation.md
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/convex-analytics/src/risk/pnl/mod.rs
- crates/convex-analytics/src/risk/pnl/engine.rs
- crates/convex-analytics/src/risk/pnl/decompose.rs
- crates/convex-analytics/src/risk/pnl/types.rs
- crates/convex-analytics/src/risk/pnl/narrate.rs
- crates/convex-mcp/src/server.rs
| > `narrate_attribution`), single currency, static book, two dates, full | ||
| > revaluation + factor decomposition, template narrator. Everything else is | ||
| > explicitly deferred. | ||
|
|
There was a problem hiding this comment.
Fix blockquote break to satisfy markdown linting.
There is a blank line inside the blockquote; use a quoted blank line (>) or remove the gap to avoid MD028 failures.
Suggested diff
> **Scope reminder.** v1 = two MCP tools (`attribute_pnl`,
> `narrate_attribution`), single currency, static book, two dates, full
> revaluation + factor decomposition, template narrator. Everything else is
> explicitly deferred.
-
+>
> **v1 status (post-implementation).** Shipped on `feat/pnl-narrator` in 6🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 18-18: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/pnl-narrator-investigation.md` at line 18, The blockquote in the
markdown contains a blank line causing MD028 failures; fix it by removing the
empty line inside the blockquote or replace that empty line with a quoted blank
line using a single '>' so every line in the quote is prefixed with '>'; update
the blockquote in the file (the offending blockquote that triggers MD028)
accordingly and re-run markdown lint to verify the MD028 error is resolved.
Extends the MCP demo for PnL attribution, mirroring the hedge advisor's
Claude Desktop smoke test.
- docs/pnl-narrator-smoke.md: setup (reuses the same server), the
canonical OAT/BTP/Bund + pay-fixed-swap paste prompt, a per-tool
verification checklist, three variants (price/yield marks, callable
rejection, the closed loop with the hedge advisor), and a triage
table. Offline equivalent: cargo test -p convex-mcp --lib
pnl_narrator_e2e.
- demo/data/eur-govt-curve-2026-05-0{7,8}.json: the two illustrative
EUR govt zero curves (t1 a bear steepener vs t0), same JSON shape as
treasury-curve-live.json, clearly marked not-live, so the demo is
reproducible from files.
- README: point the PnL section at the smoke doc.
Docs/data only — no code change. Tables verified free of pipe-in-code
breakage; JSON valid; e2e green.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/pnl-narrator-smoke.md (1)
23-24: ⚡ Quick winAvoid a user-specific absolute path in shared setup docs.
The hardcoded
C:\Users\sujit\...path is likely to fail for other users. Use a placeholder or a relative instruction.Suggested fix
- "command": "C:\\Users\\sujit\\source\\convex\\target\\release\\convex-mcp-server.exe" + "command": "C:\\Users\\<your-username>\\source\\convex\\target\\release\\convex-mcp-server.exe"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/pnl-narrator-smoke.md` around lines 23 - 24, The docs currently hardcode a user-specific absolute path in the JSON "command" value; replace that value with a generic placeholder or relative path (e.g. "<path-to-convex-mcp-server>" or "./target/release/convex-mcp-server.exe") and update the surrounding text to instruct readers to substitute their own install/build location so the "command" entry is portable for all users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/pnl-narrator-smoke.md`:
- Around line 15-17: The fenced code block containing the command "cargo build
-p convex-mcp --release" is missing a language tag which triggers markdownlint
MD040; update the opening fence from ``` to ```bash (or ```sh) so the block
becomes a bash/ shell fenced block to satisfy the linter and preserve
highlighting.
---
Nitpick comments:
In `@docs/pnl-narrator-smoke.md`:
- Around line 23-24: The docs currently hardcode a user-specific absolute path
in the JSON "command" value; replace that value with a generic placeholder or
relative path (e.g. "<path-to-convex-mcp-server>" or
"./target/release/convex-mcp-server.exe") and update the surrounding text to
instruct readers to substitute their own install/build location so the "command"
entry is portable for all users.
🪄 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: b3506a82-d733-4b70-8189-e91b04eef244
📒 Files selected for processing (4)
README.mddemo/data/eur-govt-curve-2026-05-07.jsondemo/data/eur-govt-curve-2026-05-08.jsondocs/pnl-narrator-smoke.md
✅ Files skipped from review due to trivial changes (3)
- demo/data/eur-govt-curve-2026-05-08.json
- demo/data/eur-govt-curve-2026-05-07.json
- README.md
Comment on lines
+15
to
+17
| ``` | ||
| cargo build -p convex-mcp --release | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced command block.
This code fence is missing a language identifier, which triggers markdownlint MD040.
Suggested fix
- ```
+ ```bash
cargo build -p convex-mcp --release
```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/pnl-narrator-smoke.md` around lines 15 - 17, The fenced code block
containing the command "cargo build -p convex-mcp --release" is missing a
language tag which triggers markdownlint MD040; update the opening fence from
``` to ```bash (or ```sh) so the block becomes a bash/ shell fenced block to
satisfy the linter and preserve highlighting.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/pnl-narrator-smoke.md">
<violation number="1" location="docs/pnl-narrator-smoke.md:23">
P2: The setup example hardcodes a machine-specific MCP server path, which will break for most users copying the instructions.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ```json | ||
| "mcpServers": { | ||
| "convex": { | ||
| "command": "C:\\Users\\sujit\\source\\convex\\target\\release\\convex-mcp-server.exe" |
There was a problem hiding this comment.
P2: The setup example hardcodes a machine-specific MCP server path, which will break for most users copying the instructions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/pnl-narrator-smoke.md, line 23:
<comment>The setup example hardcodes a machine-specific MCP server path, which will break for most users copying the instructions.</comment>
<file context>
@@ -0,0 +1,134 @@
+ ```json
+ "mcpServers": {
+ "convex": {
+ "command": "C:\\Users\\sujit\\source\\convex\\target\\release\\convex-mcp-server.exe"
+ }
+ }
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
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
Extends the hedge advisor with a two-date, single-currency, static-book PnL
attribution narrator: two MCP tools that decompose a book's
t0 → t1valuechange by full revaluation and narrate it deterministically.
attribute_pnl(positions, base_currency, t0, t1, curve_t0, curve_t1, [config]) → Attribution— per-position and book-level factor breakdown with fullprovenance.
narrate_attribution(attribution) → { text }— deterministic template(no LLM).
Path-ordered waterfall per position: carry → roll-down → curve
(parallel/slope/curvature/residual) → spread (per benchmark) → residual.
The closing residual is reported, never hidden.
Demo book (OAT €10mm + BTP €5mm + Bund €10mm + pay-fixed €10mm 10Y EUR swap,
May 7 → May 8 2026, +6 bp): the long sovereigns lose ≈ €192k on the rate
move; the pay-fixed swap gains ≈ €80k, offsetting ~42% — last week's hedge
showing up in this week's PnL.
Design
Four gated phases, documented in
docs/pnl-narrator-{investigation,gaps,plan}.md:parameter in pricing) does not exist —
price_from_markalready takessettlement. The pricing core is not modified; sequential repricing ispure orchestration over it.
convex-analytics::risk::pnlmodule tree (siblingof
risk::hedging) + two#[tool]methods on the existing MCP router —the same footprint as the hedge advisor.
Key engineering decisions
interest_rate_swap_riskis constant-maturity (re-issues the swap at the valuation date). PnL needs
the swap to age, so
InterestRateSwapPnlSpecpins maturity/rate at trade.ZSpreadCalculator::calculaterounded the implied Z-spread to integer bp, silently degrading every
consumer — including the shipped hedge advisor's
compute_position_risk(held the rounded z across all KRD bumps). Now full precision.
level/slope/curvature basis, documented as a reporting parameterization;
bucketed-KRD attribution is noted as the more standard alternative.
(e.g. that a swap was placed as a hedge).
Validation
convex-analytics351 lib tests,convex-mcp32 (incl. an end-to-endpnl_narrator_e2eround-trip),convex-ffi52 — all green.cargo clippyclean,cargo fmtclean, fullcargo build --workspace.docs/perf-baselines.md:attribute_pnlon the4-position demo book ≈ 128 µs. No regression on existing paths caused by
this change (the shared pricing/risk bench is flat; the propose-path drift
vs the stale 0.12.1 baseline is pre-existing and attributed in the doc).
Out of scope (deferred)
Multi-period chained attribution; position changes mid-period; FX /
cross-currency; performance vs benchmark; issue-level spread beyond benchmark
category; LLM narrator.
Summary by CodeRabbit
New Features
Bug Fixes / Accuracy
Performance & Tests
Documentation
Demo Data