Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate review findings remain in executor.py and event_aware.py.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR formalizes T73 as an explicit USD payer European swaption contract with dual-leg schedules, shared model-clock handling, and target-specific validation.
Changes:
- Adds fixed/floating schedule support across pricing paths.
- Adds semantic runtime, manifest, tolerance, and promotion validation.
- Adds T73 regression coverage and updates pricing, limitation, and migration documentation.
File summaries
| File | Reviewed change |
|---|---|
trellis/models/rate_style_swaption.py |
Resolves dual-leg swaption inputs, basis adjustments, and MC payloads. |
trellis/models/rate_style_swaption_tree.py |
Propagates model-time conventions into European tree specs. |
trellis/models/monte_carlo/event_aware.py |
Builds fixed/floating event-aware payoff payloads. Moderate (1 vote): omitted floating timelines alter legacy accrual conversion and require compatibility preservation. |
trellis/models/calibration/rates.py |
Constructs separate leg timelines and swaption terms. |
trellis/models/bermudan_swaption_tree.py |
Applies model-time day counts to lattice schedules. |
trellis/conventions/schedule.py |
Handles schedule and convention construction. |
trellis/agent/task_runtime.py |
Compiles structured task fields into runtime routes. |
trellis/agent/task_manifest_validation.py |
Validates manifests and target tolerance completeness. |
trellis/agent/planner.py |
Defines semantic planning contracts. |
trellis/agent/knowledge/promotion.py |
Validates promotion candidates against their tolerances. |
trellis/agent/executor.py |
Generates executable specs from semantic fields. Critical (1 vote): unsupported convention strings can generate invalid Python; resolve actual enum members or fail closed. |
trellis/agent/benchmark_contracts.py |
Defines benchmark targets and tolerance controls. |
trellis/agent/assembly_tools.py |
Assembles and validates generated artifacts. |
tests/test_tasks/test_t73_european_swaption.py |
Covers exact T73 replay and acceptance criteria. |
tests/test_models/test_rate_style_swaption.py |
Covers swaption pricing and model-clock behavior. |
tests/test_models/test_monte_carlo/test_event_aware.py |
Covers event-aware MC payloads and compatibility. |
tests/test_contracts/test_planner_contracts.py |
Tests planner contract validation. |
tests/test_agent/test_task_runtime.py |
Tests task runtime compilation. |
tests/test_agent/test_task_manifest_validation.py |
Tests manifest and tolerance validation. |
tests/test_agent/test_promotion_candidates.py |
Tests promotion candidates and allowance behavior. |
tests/test_agent/test_platform_requests.py |
Tests platform request handling. |
tests/test_agent/test_executor.py |
Tests generated execution behavior. |
tests/test_agent/test_build_loop.py |
Tests build-loop integration. |
tests/test_agent/test_benchmark_contracts.py |
Tests benchmark contract behavior. |
tests/test_agent/test_assembly_tools.py |
Tests artifact assembly. |
TASKS_PROOF_LEGACY.yaml |
Stores legacy proof task definitions. |
TASKS_PROOF_LEGACY_BASELINE.yaml |
Stores the legacy baseline issue inventory. |
MARKET_SCENARIOS.yaml |
Stores market scenario definitions. |
LIMITATIONS.md |
Documents pricing scope and exclusions. |
docs/user_guide/pricing.rst |
Documents pricing usage. |
docs/quant/pricing_stack.rst |
Documents pricing architecture and controls. |
docs/developer/implementation_journey_prompt_to_price.md |
Documents prompt-to-price implementation. |
doc/plan/active__task-manifest-integrity.md |
Tracks manifest integrity work. |
doc/plan/active__legacy-task-migration-map.md |
Maps legacy task migration. |
Review details
Suppressed comments (1)
trellis/models/monte_carlo/event_aware.py:549
- When
floating_timelineis omitted,floating_periodsaliases the fixed periods, but this path now converts forwards with the model-clock interval instead of the historicalforward_rate * accrual_fraction(and the discount-only branch likewise drops the accrual/model-time ratio). A legacy single schedule withday_count=ACT/360andmodel_time_day_count=30/360therefore changes its basis and MC payoff despite the docstring promising that omission preserves single-timeline behavior. Preserve the accrual-based conversion for the omitted-timeline case and use the model-clock conversion only for an explicitly supplied floating timeline.
- Files reviewed: 34/34 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 2f8fb5c24e
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The critical omitted-timeline compatibility issue remains, and focused moderate Bermudan model-clock regression coverage is still needed.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
trellis/models/bermudan_swaption_tree.py:131
- The Bermudan helpers now switch every lattice-time, exercise-frequency, and tenor calculation to the optional
model_time_day_count, but the existing Bermudan model tests only construct specs with the legacyday_countand contain no assertion for a distinct model clock. A regression in this new propagation would therefore pass the suite; add a focused case with (for example) ACT/360 accrual and 30/360 model time covering resolved horizon and compiled coupon timing.
model_time_day_count = _model_time_day_count(spec)
exercise_frequency = _infer_schedule_frequency(
exercise_dates,
model_time_day_count,
)
- Files reviewed: 34/34 changed files
- Comments generated: 1
- Review effort level: Lite
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42cb5eb844
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
Four moderate compatibility findings remain unresolved in legacy single-schedule floating conversion and generated-route handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
trellis/agent/executor.py:8060
- This generated route always materializes a fallback floating schedule and passes it to
build_discounted_swap_pv_payload, so ordinary specs that omit the new floating/model-clock fields never reach the payload's legacy conversion path. That changes the generated single-schedule Monte Carlo behavior despite the compatibility promise; only build/require the floating timeline when those conventions are explicitly authored, and otherwise passNone.
float_frequency = spec.float_frequency or spec.swap_frequency
float_day_count = spec.float_day_count or spec.day_count
model_time_day_count = spec.model_time_day_count or spec.day_count
fixed_payment_timeline = tuple(
trellis/models/rate_style_swaption.py:275
- The payload now has an explicit legacy mode when
floating_timelineis omitted, but this caller always suppliesfloating_payment_timeline. For specs that do not provide the new floating/model-clock fields, the old implementation used the single-timeline accrual conversion; this unconditionally opts them into the new model-time conversion and breaks the stated compatibility behavior. Pass the floating timeline only for an explicitly authored dual-leg/model-clock spec.
trellis/models/rate_style_swaption.py:435 - As in the curve-basis helper, this unconditionally passes a floating timeline even when the caller supplied a legacy single-schedule spec. The previous Monte Carlo path omitted that argument and therefore used the historical accrual-fraction conversion; the new call silently switches such callers to model-clock conversion. Preserve the fallback by passing
Noneunless one of the optional floating/model-clock conventions was explicitly provided.
- Files reviewed: 39/39 changed files
- Comments generated: 1
- Review effort level: Lite
| float_pv = 0.0 | ||
| for period in floating_timeline: | ||
| if period.end_date <= market_state.settlement: | ||
| continue | ||
| t_start = max(float(period.t_start or 0.0), 1e-6) | ||
| t_end = float(period.t_end or 0.0) | ||
| t_payment = float(period.t_payment or t_end) | ||
| df = float(market_state.discount.discount(t_payment)) | ||
| fwd = float(fwd_curve.forward_rate(t_start, t_end)) | ||
| # Time-based forwards are annualized on the model clock. Converting | ||
| # to the coupon day count and then accruing cancels its year fraction. | ||
| float_pv += fwd * (t_end - t_start) * df |
# Conflicts: # doc/plan/active__task-manifest-integrity.md
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
trellis/trellis/agent/semantic_contracts.py
Line 3479 in a447284
Keep valuation-only obligations out of settlement lowering
In the exercise_value_only path, the contract still puts the exercise date in settlement_dates; moreover, family_lowering_ir.py:1709-1717 lowers every obligation to an event_kind="settlement" without inspecting the new settlement_kind="valuation", and dsl_lowering.py:2409-2410 consequently advertises a settlement timeline role. Fresh evidence after the prior narrowing is this generic lowering path: T73's compiled semantic artifact still asserts a settlement event even though its request and documentation explicitly disclaim any contractual settlement lifecycle. Model this value outside settlement obligations, or teach the timeline/lowerers to preserve valuation-only events.
AGENTS.md reference: AGENTS.md:L599-L600
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
A critical benchmark-contract propagation issue and a moderate valuation-only settlement-semantics issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
trellis/agent/semantic_contracts.py:3467
- Although this branch selects
exercise_value_onlyand the obligation is valuation-kind, the product still populatestimeline.settlement_dateswith the exercise schedule. Generic lowering turns that role into anevent_kind="settlement"event, so the semantic IR still describes a contractual settlement event instead of the documented valuation-only exercise value. Make_default_semantic_timelinedistinguish an omitted settlement argument from an explicitly empty one, then pass an empty settlement role for this mode; merely changing this call tosettlement_dates=()is insufficient because the helper currently defaults falsy values to the final schedule date.
settlement_rule = (
"exercise_value_only" if exercise_value_only else "cash_settle_at_exercise"
)
event_transitions = (
_SWAPTION_EXERCISE_VALUE_TRANSITIONS
if exercise_value_only
else ("price_swaption_at_exercise", "settle_at_exercise")
- Files reviewed: 38/38 changed files
- Comments generated: 1
- Review effort level: Lite
| if str(contract.get("comparison_model_parameter_set") or "").strip(): | ||
| overrides.update( | ||
| { | ||
| "valuation_date": _parse_date(contract.get("settle_date")), | ||
| "float_frequency": _frequency(contract.get("float_frequency")), | ||
| "float_day_count": _day_count(contract.get("float_day_count")), | ||
| "model_time_day_count": _day_count( | ||
| contract.get("model_time_day_count") | ||
| ), | ||
| "rate_index": contract.get("rate_index"), | ||
| "exercise_value_convention": contract.get("exercise_value_convention"), | ||
| "valuation_measure": contract.get("valuation_measure"), | ||
| "output_unit": contract.get("output_unit"), | ||
| "output_currency": contract.get("output_currency"), | ||
| } | ||
| ) |
T73 previously supplied only a title and relied on hidden swaption economics and numerical defaults. This change gives it one exact, validated USD payer European swaption contract, a named market, explicit model and sampling controls, and target-specific acceptance criteria. The runtime compiles those structured fields directly, independently of the display title.
The pricing paths now preserve distinct semiannual 30/360 fixed and quarterly ACT/360 floating legs on the authored 30/360 model clock. Floating forwards are converted consistently to coupon amounts, with a same-curve zero-basis regression. Existing single-schedule specs and omitted-start compatibility behavior are preserved. Incomplete per-target tolerance maps fail before pricing, and promotion reviews use the candidate's own tolerance.
The proof remains bounded: the positive payer underlying-swap NPV at exercise, discounted to valuation, with constant notional, flat named discount/projection curves, constant-parameter Hull-White, 120 tree steps, and 20,000 MC paths / 64 steps / seed 42. No contractual cash/physical settlement convention or delivery lifecycle is modeled. The semantic contract carries a valuation-only exercise obligation and rejects contradictory settlement declarations. Black76 is normalized to the Hull-White tree; it is not an independent market-volatility oracle. General calendar/stub, stochastic basis, calibration, and external-parity claims remain excluded and documented.
Review fixes resolve convention strings against actual enum members before emitting code, preserve the historical accrual conversion for low-level callers omitting a floating timeline, and report T73's exercised MC seed from its authored target override. Other task-seed inconsistencies are tracked separately in QUA-1263.
The renewed review also closes the higher-level legacy bypass: calibration, rate-style helpers, and generated MC retain accrual-based conversion when all optional leg/model-clock fields are absent, including non-additive 30/360 month ends. Ordinary F006 contracts now propagate explicit floating conventions without requiring a T73 comparison parameter set. T73's semantic timeline has no settlement dates, valuation obligations remain valuation events, and family/DSL signatures carry no settlement role. Explicit event-machine dictionaries/YAML are hydrated without discarding guards/actions or silently deriving a different lifecycle.
Validation:
Docs: quant pricing stack, developer prompt-to-price journey, user pricing guide, limitations L64/L67, and task migration plan.
Implements QUA-1254. Generic bootstrap deletion remains QUA-1259; removal of the legacy no-map 5% fallback remains QUA-1252.
The plan mirror also records T89 as In Progress and tracks newly observed shared numeric-output and mutable-provenance admission gaps as QUA-1264/QUA-1265. These are not closed by T73 or the bounded T82/T89 repairs.