Skip to content

Excel add-in: Bloomberg-grade overhaul of the bond pricing surface - #127

Merged
sujitn merged 4 commits into
mainfrom
excel-bloomberg-overhaul
Jul 12, 2026
Merged

Excel add-in: Bloomberg-grade overhaul of the bond pricing surface#127
sujitn merged 4 commits into
mainfrom
excel-bloomberg-overhaul

Conversation

@sujitn

@sujitn sujitn commented Jul 12, 2026

Copy link
Copy Markdown
Owner

Reworks the Excel add-in around how a terminal user actually works: reference bonds by ticker, get full analysis grids from one call, see live updates, and never lose the session to an engine fault.

Safety & correctness

  • Every FFI export runs inside catch_unwind; shipping cdylibs build with the new [profile.excel] (panic=unwind), so a stray engine panic becomes an in-cell error instead of terminating Excel. The Java workflows build with the same profile for the same reason.
  • UDF failures surface as native Excel errors (#VALUE!/#REF!/#NUM!/#NAME?/#N/A) so IFERROR/ISERROR work; full detail per cell via =CX.LASTERROR(cell), =CX.DIAG(), and a ribbon Error Log with jump-to-cell.
  • Parallel ranges (call schedules, curve tenors/values, bootstrap instruments) parse strictly and culture-invariantly: embedded blanks or bad cells error with their position instead of silently misaligning; date strings never parse under the machine locale.

Performance

  • Idempotent object registration: a builder cell recalculating with unchanged inputs gets the same handle back (content-hash of the canonicalized spec), so Excel's dependency cutoff stops full-sheet recalc cascades. Edited specs still evict and re-mint so dependents recompute.
  • A generation counter (convex_generation) gates a client-side response cache keyed (verb, request): a five-field price block costs one engine call, and repeated recalcs are near-free.
  • Requests parse once at the FFI boundary (handlers consume the parsed value); response marshalling uses a raw pointer scan instead of per-byte interop.

Usability

  • Ticker referencing — every analytics UDF accepts CUSIP/ISIN/name or curve name (=CX.PRICE("912828YK0", …)), resolved against a registry alias table; unknown names return #REF!. Auto rebuild on add-in load heals reopened workbooks; ribbon Rebuild covers workbooks opened mid-session.
  • CX.YAS — one-call yield & spread analysis grid (street/current/simple/MM yields, G/Z/benchmark/ASW spreads, durations, convexity, DV01, settlement invoice). CX.SCENARIO — a whole shift ladder (parallel/steepener/flattener/key-rate/credit) in one call, zero-shift row anchored exactly at the mark; the Scenario form keeps a curveless yield-shift fallback.
  • Live cells.LIVE variants of PRICE/RISK/SPREAD/YAS subscribe via RTD and recompute automatically when any referenced object is rebuilt or released.
  • IntelliSense argument tooltips packed into the .xll; tickets and browsers are modeless and single-instance; hedge advisor exposed (CX.RISKPROFILE, CX.HEDGE).

Engine

  • ZSpreadCalculator now returns spreads rounded to 1e-4 bp instead of whole basis points, so repricing at the solved spread reproduces the input price to well under a cent (previously a DV01-scale gap on long-duration bonds).

Testing

  • Rust: 42 tests (cargo test -p convex-ffi) — registry idempotency/aliases/generation, panic guards, ticker resolution end-to-end, YAS and scenario smoke, JSON field-order hash stability.
  • C#: new headless test project, 46 tests (dotnet test excel/Convex.Excel.Tests) — strict parsing, culture invariance under a dd/MM locale, cache generation gating incl. a mutation-races-the-insert case, error-code mapping.
  • CI builds both sides, runs both suites, and packages the add-in from the panic=unwind profile.
  • Manual Excel checklist extended in excel/SMOKE_TEST.md (ticker refs, live cells, recalc cutoff, workbook reopen, modeless forms) — not yet run against a live Excel.

Summary by CodeRabbit

  • New Features

    • Added one-call YAS analysis, scenario ladders, live pricing/risk/spread updates, risk profiles, and hedge calculations.
    • Excel formulas now accept bond and curve identifiers, with improved caching and automatic refresh behavior.
    • Added ribbon tools for rebuilding, diagnostics, and error logs, plus enhanced settings and ticker-style references.
    • Added support for make-whole callable bonds and auditable pricing formulas stamped to worksheets.
  • Bug Fixes

    • Improved Excel error reporting with native error values and detailed per-cell diagnostics.
    • Prevented outdated curve charts and live results from overwriting newer selections.
    • Improved native library loading and panic handling to avoid host crashes.

Correctness and safety:
- Catch panics at every FFI export and ship cdylibs with the new
  [profile.excel] (panic=unwind), so an engine panic surfaces as an
  in-cell error instead of killing the host; Excel and Java builds
  both use the profile
- Native Excel errors (#VALUE!/#REF!/#NUM!/#NAME?/#N/A) with per-cell
  detail via CX.LASTERROR, CX.DIAG, and a ribbon error log
- Strict culture-invariant parsing: embedded blanks or unparseable
  cells in parallel ranges error with their position instead of
  silently misaligning; dates never parse under the machine locale

Performance (targets 1k-10k pricing cells per workbook):
- Idempotent registration: rebuilding an unchanged spec returns the
  same handle with no generation bump, so Excel dependency cutoff
  stops recalc cascades; edited specs still evict and re-mint
- Generation-gated response cache keyed (verb, request): multi-field
  blocks cost one engine call, repeated recalcs are near-free
- One JSON parse per RPC; pointer-scan UTF-8 marshalling

Usability:
- Ticker-style referencing: analytics UDFs accept CUSIP/ISIN/name or
  curve names via a registry alias table; auto rebuild on add-in load
  heals reopened workbooks (ribbon Rebuild for manual use)
- CX.YAS one-call yield & spread analysis grid; CX.SCENARIO curve
  ladder in one call, zero-shift row anchored at the mark, with a
  curveless yield-shift fallback in the Scenario form
- CX.PRICE/RISK/SPREAD/YAS .LIVE variants: RTD-backed cells that
  recompute automatically when referenced objects change
- IntelliSense tooltips packed into the .xll; modeless single-instance
  tickets; hedge advisor exposed (CX.RISKPROFILE, CX.HEDGE)

Engine: Z-spread solves now round to 1e-4 bp instead of whole bp, so
repricing at the returned spread reproduces the input price to well
under a cent.

Tests: headless suites on both sides (42 Rust incl. registry
idempotency, panic guards, ticker resolution, YAS/scenario smoke;
46 C# covering parsing, culture invariance, cache generation gating,
error mapping). Manual Excel checklist extended in SMOKE_TEST.md.
@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@sujitn, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 31 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

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, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: eb9bbbdf-4645-42c0-816f-87dfb81b4589

📥 Commits

Reviewing files that changed from the base of the PR and between def9bdc and 46aea16.

📒 Files selected for processing (28)
  • crates/convex-analytics/src/spreads/zspread.rs
  • crates/convex-ffi/src/build.rs
  • crates/convex-ffi/src/dispatch.rs
  • crates/convex-ffi/src/lib.rs
  • crates/convex-ffi/src/registry.rs
  • crates/convex-ffi/src/schemas.rs
  • crates/convex-ffi/tests/smoke.rs
  • excel/Convex.Excel.Tests/CxCacheTests.cs
  • excel/Convex.Excel.Tests/CxParseTests.cs
  • excel/Convex.Excel/Convex.Excel.dna
  • excel/Convex.Excel/Cx.cs
  • excel/Convex.Excel/CxCache.cs
  • excel/Convex.Excel/CxCaller.cs
  • excel/Convex.Excel/CxErrorStore.cs
  • excel/Convex.Excel/CxLive.cs
  • excel/Convex.Excel/CxParse.cs
  • excel/Convex.Excel/CxSettings.cs
  • excel/Convex.Excel/Functions.cs
  • excel/Convex.Excel/NativeLoader.cs
  • excel/Convex.Excel/RibbonController.cs
  • excel/Convex.Excel/forms/CurveViewerForm.cs
  • excel/Convex.Excel/forms/ErrorLogForm.cs
  • excel/Convex.Excel/forms/PricingTicketForm.cs
  • excel/Convex.Excel/forms/ScenarioForm.cs
  • excel/Convex.Excel/helpers/ComboReload.cs
  • excel/Convex.Excel/helpers/SheetHelpers.cs
  • excel/README.md
  • excel/SMOKE_TEST.md
📝 Walkthrough

Walkthrough

This change adds an Excel-specific unwinding Cargo profile, alias-aware idempotent FFI registration, new YAS and scenario analytics, panic-safe native boundaries, generation-aware Excel caching, live UDFs, diagnostics, expanded forms, and corresponding tests and documentation.

Changes

Excel and FFI integration

Layer / File(s) Summary
Build profiles and native packaging
.github/workflows/*, Cargo.toml, excel/Convex.Excel/*
CI, release, Java, and Excel builds use the excel Cargo profile and its output paths.
Analytics contracts and registry identity
crates/convex-analytics/*, crates/convex-ffi/src/build.rs, crates/convex-ffi/src/registry.rs, crates/convex-ffi/src/schemas.rs
YAS and scenario wire types, schema definitions, precise Z-spread rounding, canonical hashes, aliases, idempotent registration, eviction, and generation tracking are added.
Reference resolution and panic-safe RPCs
crates/convex-ffi/src/dispatch.rs, crates/convex-ffi/src/lib.rs
RPCs resolve bond and curve aliases before deserialization, expose YAS and scenario operations, and convert panics into safe FFI results.
Excel bridge state and errors
excel/Convex.Excel/Cx*.cs, excel/Convex.Excel/NativeLoader.cs
The add-in adds generation-aware caching, strict parsing, live subscriptions, caller tracking, structured error storage, native loading diagnostics, and persisted settings.
Excel UDFs and UI
excel/Convex.Excel/Functions.cs, excel/Convex.Excel/forms/*, excel/Convex.Excel/RibbonController.cs
New live, YAS, scenario, risk-profile, hedge, diagnostic, and error-log functionality is wired into Excel functions, forms, and ribbon actions.
Validation and documentation
crates/convex-ffi/tests/*, excel/Convex.Excel.Tests/*, excel/README.md, excel/SMOKE_TEST.md
Coverage and documentation are updated for aliases, idempotency, scenarios, YAS, strict parsing, native Excel errors, live cells, and rebuild behavior.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.37% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the broad Excel add-in bond pricing and analytics overhaul described by the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch excel-bloomberg-overhaul

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Cut narration, design essays, and restated rationale across the add-in
and FFI crates; comments now carry only what the code cannot show
(threading rules, panic-profile requirement, locale traps, race
windows, eviction semantics). Also refreshed the stale UDF-surface
banner in Functions.cs.

@cubic-dev-ai cubic-dev-ai Bot 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.

2 issues found and verified against the latest diff

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="crates/convex-analytics/src/dto.rs">

<violation number="1" location="crates/convex-analytics/src/dto.rs:623">
P2: Schema consumers cannot discover or validate `convex_scenario`: `convex_schema("ScenarioRequest")` fails because these DTOs were not added to the curated schema registry. Add request/response schema definitions and lookup arms alongside this public wire contract.</violation>
</file>

<file name="excel/README.md">

<violation number="1" location="excel/README.md:299">
P2: The README's error-mapping table documents `#REF!` for unknown handles/tickers, but the SMOKE_TEST expects `#VALUE!` for unknown handle strings until ticker lookup is fully functional. If a user tries `=CX.PRICE("BAD_HANDLE", …)` and gets `#VALUE!`, they'll find the README says unknown handles yield `#REF!` — there's no note explaining the transitional state. Either update the README to reflect the current error mapping (and note that ticker-resolution will switch the error code), or add a transitional footnote so users aren't misled.</violation>
</file>

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic

Comment thread excel/Convex.Excel/CxCache.cs Outdated
Comment thread excel/Convex.Excel/CxParse.cs Outdated
Comment thread crates/convex-ffi/src/dispatch.rs Outdated
Comment thread excel/Convex.Excel/forms/ErrorLogForm.cs Outdated
Comment thread excel/Convex.Excel/CxCache.cs Outdated
Comment thread excel/Convex.Excel/helpers/ComboReload.cs Outdated
Comment thread excel/README.md
|---|---|
| `#VALUE!` | malformed input — bad mark, unparseable cell, misaligned parallel ranges |
| `#NAME?` | unknown keyword — frequency, day count, spread type, field/metric name |
| `#REF!` | handle (or ticker) not registered, or wrong object kind |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: The README's error-mapping table documents #REF! for unknown handles/tickers, but the SMOKE_TEST expects #VALUE! for unknown handle strings until ticker lookup is fully functional. If a user tries =CX.PRICE("BAD_HANDLE", …) and gets #VALUE!, they'll find the README says unknown handles yield #REF! — there's no note explaining the transitional state. Either update the README to reflect the current error mapping (and note that ticker-resolution will switch the error code), or add a transitional footnote so users aren't misled.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At excel/README.md, line 299:

<comment>The README's error-mapping table documents `#REF!` for unknown handles/tickers, but the SMOKE_TEST expects `#VALUE!` for unknown handle strings until ticker lookup is fully functional. If a user tries `=CX.PRICE("BAD_HANDLE", …)` and gets `#VALUE!`, they'll find the README says unknown handles yield `#REF!` — there's no note explaining the transitional state. Either update the README to reflect the current error mapping (and note that ticker-resolution will switch the error code), or add a transitional footnote so users aren't misled.</comment>

<file context>
@@ -237,9 +286,23 @@ Codes:
+|---|---|
+| `#VALUE!` | malformed input — bad mark, unparseable cell, misaligned parallel ranges |
+| `#NAME?`  | unknown keyword — frequency, day count, spread type, field/metric name |
+| `#REF!`   | handle (or ticker) not registered, or wrong object kind |
+| `#NUM!`   | solver non-convergence or domain error |
+| `#N/A`    | native library not loaded — run `=CX.DIAG()` |
</file context>

Comment thread excel/Convex.Excel/forms/ScenarioForm.cs Outdated
Comment thread excel/README.md
Comment thread crates/convex-analytics/src/spreads/zspread.rs

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 4

🧹 Nitpick comments (2)
excel/Convex.Excel/forms/PricingTicketForm.cs (1)

155-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

BuildRequest and BuildFormula/StampRef independently re-derive the same fields.

Both methods separately pull bond/curve/settle/mark/frequency from the UI. If a new field is ever added to one and not the other, the ticket could validate one request while stamping a formula for a different one.

Consider extracting a single "gather ticket inputs" step (bond/curve refs, settle date, mark, frequency) that both the JSON request builder and the formula builder consume, so the two representations can't drift.

Also applies to: 189-220

🤖 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 `@excel/Convex.Excel/forms/PricingTicketForm.cs` around lines 155 - 169, The
ticket inputs are independently gathered by BuildRequest and
BuildFormula/StampRef, allowing their representations to diverge. Extract a
shared input-gathering method or model for the bond and optional curve
references, settlement date, mark, and quote frequency, then have BuildRequest
and the formula/stamping flow consume that single result while preserving the
existing validation and optional-curve behavior.
crates/convex-ffi/src/dispatch.rs (1)

116-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a guard test for resolver tables

These *_REFS lists are still hand-maintained, so a new Handle/Option<Handle> field can slip past alias resolution and degrade to a generic serde_json::from_value error. A small test that asserts each request DTO’s handle-bearing fields are covered would make drift much harder; SPREAD_REFS’ nested params.* paths make this especially easy to miss.

🤖 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 `@crates/convex-ffi/src/dispatch.rs` around lines 116 - 131, Add a guard test
alongside the resolver-table definitions that inspects every request DTO’s
handle-bearing fields and asserts they are represented in the corresponding
*_REFS table, including nested params paths such as SPREAD_REFS. Cover all
listed request types and both Handle and Option<Handle> fields so future DTO
changes fail the test until alias resolution is updated.
🤖 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-ffi/src/build.rs`:
- Around line 427-438: Update curve_name_and_aliases so registry_key and name
are trimmed and treated as absent when empty before selecting reg_name. Preserve
the existing alias filtering, and ensure blank inputs cannot produce an empty
registry name or cause unrelated curves to share the same eviction slot.

In `@crates/convex-ffi/src/registry.rs`:
- Around line 91-98: Update Inner.tables and all registry accessors used by
register(), release(), clear_all(), and read paths to avoid permanent RwLock
poisoning: either replace it with a non-poisoning lock or recover poisoned
read/write guards explicitly. Ensure panic guards still return errors while
subsequent registry operations can continue after a panic.

In `@excel/Convex.Excel/CxCache.cs`:
- Line 43: Replace the concatenated string key in the cache-key construction
with a collision-safe tuple containing both verb and requestJson. Update the
surrounding cache access in CxCache so the tuple is used consistently for lookup
and storage.

In `@excel/README.md`:
- Line 103: Update the CX.SPREAD documentation and its associated settings
dropdown so the accepted ASW/ASW_PAR aliases and CREDIT option are represented
consistently; either document both aliases and CREDIT clearly or add the missing
selectable options, while preserving the existing default field behavior.

---

Nitpick comments:
In `@crates/convex-ffi/src/dispatch.rs`:
- Around line 116-131: Add a guard test alongside the resolver-table definitions
that inspects every request DTO’s handle-bearing fields and asserts they are
represented in the corresponding *_REFS table, including nested params paths
such as SPREAD_REFS. Cover all listed request types and both Handle and
Option<Handle> fields so future DTO changes fail the test until alias resolution
is updated.

In `@excel/Convex.Excel/forms/PricingTicketForm.cs`:
- Around line 155-169: The ticket inputs are independently gathered by
BuildRequest and BuildFormula/StampRef, allowing their representations to
diverge. Extract a shared input-gathering method or model for the bond and
optional curve references, settlement date, mark, and quote frequency, then have
BuildRequest and the formula/stamping flow consume that single result while
preserving the existing validation and optional-curve behavior.
🪄 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: 517c0077-126a-42ef-ad73-d49935a8ca2f

📥 Commits

Reviewing files that changed from the base of the PR and between c5ed220 and def9bdc.

📒 Files selected for processing (44)
  • .github/workflows/ci.yml
  • .github/workflows/java-release.yml
  • .github/workflows/java.yml
  • .github/workflows/release.yml
  • .gitignore
  • Cargo.toml
  • crates/convex-analytics/src/dto.rs
  • crates/convex-analytics/src/spreads/zspread.rs
  • crates/convex-ffi/src/build.rs
  • crates/convex-ffi/src/dispatch.rs
  • crates/convex-ffi/src/lib.rs
  • crates/convex-ffi/src/registry.rs
  • crates/convex-ffi/src/schemas.rs
  • crates/convex-ffi/tests/smoke.rs
  • excel/Convex.Excel.Tests/Convex.Excel.Tests.csproj
  • excel/Convex.Excel.Tests/CxCacheTests.cs
  • excel/Convex.Excel.Tests/CxParseTests.cs
  • excel/Convex.Excel/Convex.Excel.csproj
  • excel/Convex.Excel/Convex.Excel.dna
  • excel/Convex.Excel/Cx.cs
  • excel/Convex.Excel/CxCache.cs
  • excel/Convex.Excel/CxCaller.cs
  • excel/Convex.Excel/CxErrorStore.cs
  • excel/Convex.Excel/CxLive.cs
  • excel/Convex.Excel/CxParse.cs
  • excel/Convex.Excel/CxSettings.cs
  • excel/Convex.Excel/Functions.cs
  • excel/Convex.Excel/NativeLoader.cs
  • excel/Convex.Excel/RibbonController.cs
  • excel/Convex.Excel/forms/BondBuilderForm.cs
  • excel/Convex.Excel/forms/CurveBuilderForm.cs
  • excel/Convex.Excel/forms/CurveViewerForm.cs
  • excel/Convex.Excel/forms/ErrorLogForm.cs
  • excel/Convex.Excel/forms/ObjectBrowserForm.cs
  • excel/Convex.Excel/forms/PricingTicketForm.cs
  • excel/Convex.Excel/forms/ScenarioForm.cs
  • excel/Convex.Excel/forms/SchemaBrowserForm.cs
  • excel/Convex.Excel/forms/SettingsForm.cs
  • excel/Convex.Excel/forms/SpreadTicketForm.cs
  • excel/Convex.Excel/helpers/ComboReload.cs
  • excel/Convex.Excel/helpers/FormUi.cs
  • excel/Convex.Excel/helpers/SheetHelpers.cs
  • excel/README.md
  • excel/SMOKE_TEST.md

Comment on lines +427 to +438
fn curve_name_and_aliases(
registry_key: Option<String>,
name: Option<String>,
) -> (Option<String>, Vec<String>) {
let aliases: Vec<String> = name
.iter()
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty())
.collect();
let reg_name = registry_key.or(name);
(reg_name, aliases)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

curve_name_and_aliases doesn't filter empty-string keys before using them as the eviction slot.

aliases is trimmed and empty-filtered, but reg_name = registry_key.or(name) isn't — if either arrives as Some("") (e.g. a blank Excel argument marshaled as an empty string rather than omitted/null), it's used as-is as the registry name. Two unrelated curves both landing on registry_key: Some("") would collide in t.names, and register()'s idempotent-eviction logic would evict one when the other rebuilds, since it matches on the exact name string.

🐛 Proposed fix
 fn curve_name_and_aliases(
     registry_key: Option<String>,
     name: Option<String>,
 ) -> (Option<String>, Vec<String>) {
-    let aliases: Vec<String> = name
-        .iter()
-        .map(|s| s.trim().to_string())
-        .filter(|s| !s.is_empty())
-        .collect();
-    let reg_name = registry_key.or(name);
+    let registry_key = registry_key.map(|s| s.trim().to_string()).filter(|s| !s.is_empty());
+    let name = name.map(|s| s.trim().to_string()).filter(|s| !s.is_empty());
+    let aliases: Vec<String> = name.clone().into_iter().collect();
+    let reg_name = registry_key.or(name);
     (reg_name, aliases)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fn curve_name_and_aliases(
registry_key: Option<String>,
name: Option<String>,
) -> (Option<String>, Vec<String>) {
let aliases: Vec<String> = name
.iter()
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty())
.collect();
let reg_name = registry_key.or(name);
(reg_name, aliases)
}
fn curve_name_and_aliases(
registry_key: Option<String>,
name: Option<String>,
) -> (Option<String>, Vec<String>) {
let registry_key = registry_key
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty());
let name = name
.map(|s| s.trim().to_string())
.filter(|s| !s.is_empty());
let aliases: Vec<String> = name.clone().into_iter().collect();
let reg_name = registry_key.or(name);
(reg_name, aliases)
}
🤖 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 `@crates/convex-ffi/src/build.rs` around lines 427 - 438, Update
curve_name_and_aliases so registry_key and name are trimmed and treated as
absent when empty before selecting reg_name. Preserve the existing alias
filtering, and ensure blank inputs cannot produce an empty registry name or
cause unrelated curves to share the same eviction slot.

Comment on lines 91 to 98
struct Inner {
next_handle: AtomicU64,
/// Bumped on every mutation (register of new content, release, clear) and
/// NOT on idempotent re-registration. Callers key caches on this: same
/// generation ⇒ every handle resolves to the same object it did before.
generation: AtomicU64,
tables: RwLock<Tables>,
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the registry implementation and panic guards.
git ls-files crates/convex-ffi/src/registry.rs crates/convex-ffi/src/lib.rs

printf '\n--- registry.rs (outline) ---\n'
ast-grep outline crates/convex-ffi/src/registry.rs --view expanded || true

printf '\n--- lib.rs (outline) ---\n'
ast-grep outline crates/convex-ffi/src/lib.rs --view expanded || true

printf '\n--- registry.rs relevant sections ---\n'
sed -n '1,220p' crates/convex-ffi/src/registry.rs
printf '\n--- registry.rs later sections ---\n'
sed -n '220,420p' crates/convex-ffi/src/registry.rs

printf '\n--- lib.rs relevant sections ---\n'
sed -n '1,240p' crates/convex-ffi/src/lib.rs

Repository: sujitn/convex

Length of output: 26300


Avoid lock poisoning in REGISTRY.tables
register(), release(), and clear_all() can poison the RwLock on any panic, and the later .read().unwrap() / .write().unwrap() calls will keep failing for the rest of the process. The panic guards only convert the first failure into an error; they do not recover the poisoned lock. Switch to a non-poisoning lock or recover the poison explicitly.

🤖 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 `@crates/convex-ffi/src/registry.rs` around lines 91 - 98, Update Inner.tables
and all registry accessors used by register(), release(), clear_all(), and read
paths to avoid permanent RwLock poisoning: either replace it with a
non-poisoning lock or recover poisoned read/write guards explicitly. Ensure
panic guards still return errors while subsequent registry operations can
continue after a panic.

}
}

var key = verb + "" + requestJson;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Cache key has no real delimiter between verb and requestJson.

verb + "" + requestJson is equivalent to verb + requestJson — the empty string contributes nothing. This risks key collisions between different (verb, requestJson) pairs that happen to concatenate to the same string (e.g. if a future verb name is a prefix/suffix overlap of another, or a request body isn't a JSON object). In a response cache for pricing/spread results, a collision means silently returning a cached result for the wrong request.

🔧 Proposed fix: use a tuple key instead of string concatenation
-        private static readonly ConcurrentDictionary<string, string> _cache = new();
+        private static readonly ConcurrentDictionary<(string Verb, string RequestJson), string> _cache = new();
@@
-            var key = verb + "" + requestJson;
+            var key = (verb, requestJson);
🤖 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 `@excel/Convex.Excel/CxCache.cs` at line 43, Replace the concatenated string
key in the cache-key construction with a collision-safe tuple containing both
verb and requestJson. Update the surrounding cache access in CxCache so the
tuple is used consistently for lookup and storage.

Comment thread excel/README.md
| `CX.PRICE(bond, settle, mark, [curve], [quoteFreq], [field])` | Returns clean / dirty / accrued / YTM / Z-bps. `field`: `clean` (default), `dirty`, `accrued`, `ytm`, `z_spread`, `grid`. |
| `CX.YAS(bond, settle, mark, curve, [govtCurve], [quoteFreq], [swapCurve])` | **One-call Bloomberg-YAS-style grid**: every yield convention, G/Z/benchmark/ASW spreads, durations, convexity, DV01, and the settlement invoice — a single engine computation. |
| `CX.RISK(bond, settle, mark, [curve], [metric], [quoteFreq], [krdTenors])` | `metric`: `grid` (default), `mod_dur`, `mac_dur`, `convexity`, `dv01`, `spread_dur`, `krd`. |
| `CX.SPREAD(bond, curve, settle, mark, [type], [vol], [field])` | Z / G / I / ASW / ASW_PROC / OAS / DM / Credit. `field`: `bps` (default), `grid`. |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the actual spread type tokens accepted by the UDF and the ErrorCodes definition.
rg -rn "ASW_PAR|ASW\b|\"Credit\"" --type=cs -g '!**/bin/**' -g '!**/obj/**' -C2
# Also check the Rust DTO for spread type enum variants.
rg -rn "Asw|AswPar|Credit" --type=rust -C2

Repository: sujitn/convex

Length of output: 151


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the relevant files and map their contents lightly.
git ls-files | rg '(^|/)(SettingsForm\.cs|README\.md|.*spread.*|.*CX.*|.*UDF.*|.*Settings.*)$'

# Find every occurrence of the spread token names and dropdown labels.
rg -n --hidden --glob '!**/bin/**' --glob '!**/obj/**' --glob '!**/.git/**' \
  'ASW_PAR|ASW\b|ASW_PROC|Credit|CX\.SPREAD|spread type|spreadType|SpreadType' .

# If SettingsForm.cs exists, print the surrounding lines around the reported label mapping.
if git ls-files | rg -q 'SettingsForm\.cs$'; then
  file=$(git ls-files | rg 'SettingsForm\.cs$' | head -n 1)
  echo "== $file =="
  rg -n -C3 'ASW_PAR|ASW\b|Credit|spread' "$file"
fi

# Show the README snippet around the documented CX.SPREAD line.
if git ls-files | rg -q 'excel/README\.md$'; then
  file=excel/README.md
  echo "== $file =="
  rg -n -C2 'CX\.SPREAD' "$file"
fi

Repository: sujitn/convex

Length of output: 50369


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== excel/Convex.Excel/CxParse.cs =="
rg -n -C4 'AsSpreadType|ASW_PAR|CREDIT|AssetSwapPar|AssetSwapProceeds' excel/Convex.Excel/CxParse.cs

echo
echo "== excel/Convex.Excel/forms/SettingsForm.cs =="
rg -n -C4 'Items\.AddRange|Default spread|DefaultSpreadType|ASW_PAR|ASW_PROC|Credit' excel/Convex.Excel/forms/SettingsForm.cs

echo
echo "== crates/convex-core/src/types/mark.rs =="
rg -n -C4 'parse_spread_type|ASW_PAR|CREDIT|AssetSwapPar|AssetSwapProceeds' crates/convex-core/src/types/mark.rs

echo
echo "== excel/README.md context =="
rg -n -C2 'Spread types:|CX\.SPREAD\(' excel/README.md

Repository: sujitn/convex

Length of output: 8004


Clarify the ASW alias and missing default option
CX.SPREAD accepts both ASW and ASW_PAR, but the settings dropdown only exposes ASW_PAR, and CREDIT is accepted by the parser yet not selectable there. Either document the aliases or add the missing UI option so the terminology stays consistent.

🤖 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 `@excel/README.md` at line 103, Update the CX.SPREAD documentation and its
associated settings dropdown so the accepted ASW/ASW_PAR aliases and CREDIT
option are represented consistently; either document both aliases and CREDIT
clearly or add the missing selectable options, while preserving the existing
default field behavior.

sujitn added 2 commits July 12, 2026 07:41
- YAS: G-/I-spread marks now price against the supplied government/swap
  curve instead of always the spot curve (with an end-to-end test)
- All-digit tickers ("037833100") resolve as names; bare digit strings
  are no longer misparsed as numeric handles
- CxCache: entries are generation-tagged and validated on every hit
  (closes the stale-hit race), and Lazy values give single-flight so
  concurrent identical calls share one engine call
- Live cells record errors under their owning cell (captured at
  subscribe time), so CX.LASTERROR works after timer-driven refreshes
- Curve viewer: renders invalidate outstanding sweeps, obsolete sweeps
  stop mid-loop, and the close/BeginInvoke race is guarded
- Error log jump handles sheet names containing apostrophes; combo
  labels keep names containing the display separator intact
- Scenario form gains key_rate with a pivot/key-tenor field
- Pre-dynamic-array Excel: stamped grids fall back to a legacy CSE
  array formula instead of showing one cell
- ScenarioRequest/Response added to the schema registry; Z-spread
  Decimals built with from_f64 (clean digits); docs aligned (#REF! for
  unknown tickers, panic → #VALUE!)

@cubic-dev-ai cubic-dev-ai Bot 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.

4 issues found across 21 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="crates/convex-ffi/src/schemas.rs">

<violation number="1" location="crates/convex-ffi/src/schemas.rs:297">
P2: Schema-based clients can validate a scenario bump then receive `invalid_input` from `convex_scenario`, because required fields are conditional on `kind` but the schema requires only `kind`. Represent each bump variant as a conditional/`oneOf` schema with its required inputs.</violation>
</file>

<file name="excel/Convex.Excel/CxLive.cs">

<violation number="1" location="excel/Convex.Excel/CxLive.cs:25">
P2: When identical `.LIVE` formulas appear in multiple cells, an RPC or RTD-push error is attached only to the cell that created the shared topic, so `CX.LASTERROR` for the other error cells is empty or misleading. Keep per-subscriber cell context for error recording, or use a cell-specific observation subscription while preserving the response cache.</violation>
</file>

<file name="excel/Convex.Excel/helpers/SheetHelpers.cs">

<violation number="1" location="excel/Convex.Excel/helpers/SheetHelpers.cs:51">
P2: Legacy Excel stamping fails for valid long ticker/name inputs because `Range.FormulaArray` accepts at most 255 characters. Validate this before the fallback and provide a supported legacy path or a clear, input-specific error rather than attempting the assignment.</violation>

<violation number="2" location="excel/Convex.Excel/helpers/SheetHelpers.cs:51">
P1: On pre-dynamic-array Excel, stamping into a cell with data in any of the next four rows or second column overwrites that data with the CSE formula. Check the target 5×2 range is empty (or require explicit confirmation) before assigning `FormulaArray`, matching the non-destructive `#SPILL!` behavior on newer Excel.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

catch
{
dynamic range = anchor.Resize(spillRows, spillCols);
range.FormulaArray = formula;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: On pre-dynamic-array Excel, stamping into a cell with data in any of the next four rows or second column overwrites that data with the CSE formula. Check the target 5×2 range is empty (or require explicit confirmation) before assigning FormulaArray, matching the non-destructive #SPILL! behavior on newer Excel.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At excel/Convex.Excel/helpers/SheetHelpers.cs, line 51:

<comment>On pre-dynamic-array Excel, stamping into a cell with data in any of the next four rows or second column overwrites that data with the CSE formula. Check the target 5×2 range is empty (or require explicit confirmation) before assigning `FormulaArray`, matching the non-destructive `#SPILL!` behavior on newer Excel.</comment>

<file context>
@@ -33,6 +33,26 @@ public static string WriteFormulaAtSelection(string formula)
+            catch
+            {
+                dynamic range = anchor.Resize(spillRows, spillCols);
+                range.FormulaArray = formula;
+            }
+            return anchor.Address;
</file context>

"name": {"type": ["string","null"]},
"bumps": {"type": "array", "items": {
"type": "object",
"required": ["kind"],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Schema-based clients can validate a scenario bump then receive invalid_input from convex_scenario, because required fields are conditional on kind but the schema requires only kind. Represent each bump variant as a conditional/oneOf schema with its required inputs.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/convex-ffi/src/schemas.rs, line 297:

<comment>Schema-based clients can validate a scenario bump then receive `invalid_input` from `convex_scenario`, because required fields are conditional on `kind` but the schema requires only `kind`. Represent each bump variant as a conditional/`oneOf` schema with its required inputs.</comment>

<file context>
@@ -275,6 +277,61 @@ const YAS_RESPONSE: &str = r##"{
+        "name": {"type": ["string","null"]},
+        "bumps": {"type": "array", "items": {
+          "type": "object",
+          "required": ["kind"],
+          "properties": {
+            "kind": {"enum": ["parallel","steepener","flattener","key_rate","credit_spread"]},
</file context>

var key = caller == null ? null : CxCaller.StableKey(caller);
var address = caller == null ? null : CxCaller.Describe(caller);
return ExcelAsyncUtil.Observe("CX.LIVE:" + verb, requestJson,
() => new RpcObservable(verb, requestJson, select, key, address));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: When identical .LIVE formulas appear in multiple cells, an RPC or RTD-push error is attached only to the cell that created the shared topic, so CX.LASTERROR for the other error cells is empty or misleading. Keep per-subscriber cell context for error recording, or use a cell-specific observation subscription while preserving the response cache.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At excel/Convex.Excel/CxLive.cs, line 25:

<comment>When identical `.LIVE` formulas appear in multiple cells, an RPC or RTD-push error is attached only to the cell that created the shared topic, so `CX.LASTERROR` for the other error cells is empty or misleading. Keep per-subscriber cell context for error recording, or use a cell-specific observation subscription while preserving the response cache.</comment>

<file context>
@@ -16,34 +16,44 @@ internal static class CxLive
-                CxErrorStore.Record(ex);
-                return ErrorMapper.ToExcelError(ex);
-            }
+                () => new RpcObservable(verb, requestJson, select, key, address));
         }
 
</file context>

catch
{
dynamic range = anchor.Resize(spillRows, spillCols);
range.FormulaArray = formula;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Legacy Excel stamping fails for valid long ticker/name inputs because Range.FormulaArray accepts at most 255 characters. Validate this before the fallback and provide a supported legacy path or a clear, input-specific error rather than attempting the assignment.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At excel/Convex.Excel/helpers/SheetHelpers.cs, line 51:

<comment>Legacy Excel stamping fails for valid long ticker/name inputs because `Range.FormulaArray` accepts at most 255 characters. Validate this before the fallback and provide a supported legacy path or a clear, input-specific error rather than attempting the assignment.</comment>

<file context>
@@ -33,6 +33,26 @@ public static string WriteFormulaAtSelection(string formula)
+            catch
+            {
+                dynamic range = anchor.Resize(spillRows, spillCols);
+                range.FormulaArray = formula;
+            }
+            return anchor.Address;
</file context>

@sujitn
sujitn merged commit acc357b into main Jul 12, 2026
10 checks passed
@sujitn
sujitn deleted the excel-bloomberg-overhaul branch July 12, 2026 06:57
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.

1 participant