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. Breezy! 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
Four unresolved findings block approval, including manifest mismatches, incompatible MC semantics, missing strike handling, and omitted output contract fields.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR replaces title-derived E22 cap economics with an authored, validated USD cap-strip contract and deterministic Monte Carlo controls.
Changes:
- Adds exact E22 contract and market validation.
- Propagates schedules, inputs, path count, and seed through semantic/spec hydration.
- Adds regression coverage and updates documentation and migration tracking.
File summaries
| File | Summary and review status |
|---|---|
trellis/agent/task_runtime.py |
Bridges E22 structured terms into semantic contracts. |
trellis/agent/task_manifest_validation.py |
Validates exact E22 economics and market inputs. |
trellis/agent/semantic_contracts.py |
Preserves authored terms; Critical (1 vote): MC lowering still emits the incompatible Hull-White short-rate profile. |
trellis/agent/planner.py |
Adds Monte Carlo controls to generated specifications. |
trellis/agent/executor.py |
Hydrates schedules and controls; Critical (1 vote): smoke construction can remove the required generic strike. |
trellis/agent/benchmark_contracts.py |
Renders benchmark contracts; Moderate (1 vote): omits valuation measure, output unit, and currency. |
tests/test_tasks/test_e22_cap_strip.py |
Adds E22 contract, hydration, pricing, and workflow coverage. |
TASKS_PROOF_LEGACY.yaml |
Defines the authored E22 contract; Critical (1 vote): evaluation manifests retain stale target aliases and references. |
TASKS_PROOF_LEGACY_BASELINE.yaml |
Updates legacy baseline metadata. |
LIMITATIONS.md |
Documents E22’s bounded forward-marginal scope. |
docs/user_guide/pricing.rst |
Updates user-facing pricing guidance. |
docs/quant/pricing_stack.rst |
Documents pricing semantics and Monte Carlo behavior. |
docs/developer/implementation_journey_prompt_to_price.md |
Documents semantic hydration and execution flow. |
doc/plan/active__task-manifest-integrity.md |
Updates manifest-integrity tracking. |
doc/plan/active__legacy-task-migration-map.md |
Updates E22 migration tracking. |
Review details
Suppressed comments (1)
trellis/agent/benchmark_contracts.py:934
task_to_descriptionuses this structured renderer instead of the task's free-form description, but this E22 detail block never rendersvaluation_measure,output_unit, oroutput_currency. Consequently the build request does not state the required USD holder-present-value output; those fields remain only in hidden task/validation metadata. Include these authored output terms in the rendered contract so a non-deterministic or future generated lane cannot silently lose the output contract.
for field in (
"model_time_day_count", "discount_curve_day_count", "forecast_curve_day_count",
"calendar_name", "business_day_adjustment",
"fixing_rule", "payment_rule", "fixing_lag_days", "payment_lag_days",
"mc_distribution", "sampling", "n_paths", "seed",
):
if contract.get(field) is not None:
lines.append(f"{field}: {contract[field]}.")
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Review fixes pushed in ed0f2ad. The three inline findings are fixed and resolved. The additional suppressed finding in the review summary is also fixed: structured cap-strip descriptions now render valuation_measure, output_unit and output_currency, with an exact regression for the authored USD holder-present-value contract. Validation: 77 focused tests, the full offline E22 global workflow, and 527 adjacent regression tests passed; all changed Python lint and diff checks passed. Documentation and LIMITATIONS L68 updated. Draft/In Progress is intentional: integrated full release validation and T73/baseline integration remain pending. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
![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: 24e7f5e996
ℹ️ 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".
| for term_name, expected in ( | ||
| ("model", "black"), ("mc_distribution", process_family), ("sampling", "antithetic"), | ||
| ): |
There was a problem hiding this comment.
When an ordinary cap/floor semantic contract authors n_steps, mean_reversion, or sigma in term_fields, this admission check still returns ok: it only checks model, mc_distribution, and sampling. Those controls are not hydrated into the generated strip spec, and price_rate_cap_floor_strip_monte_carlo explicitly discards them, so a request expressing short-rate/path assumptions is silently priced as independent lognormal forward marginals instead of failing closed. Add these incompatible controls to the profile failures, as the E22-specific manifest validator already effectively requires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
Route admission can accept unsupported controls and incompatible model clocks that may be silently ignored.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
trellis/agent/route_registry.py:1952
- This profile only rejects
model,mc_distribution, andsampling, so a cap/floor semantic contract that carriesn_steps,mean_reversion, orsigmastill passes route admission even though the MC helper explicitly discards those values (trellis/models/rate_cap_floor.py:545). That permits unsupported short-rate controls to be authored and silently ignored; reject these non-Noneterms here (or otherwise fail the route) rather than relying only on the special E22 manifest validator.
# Optional authored terms may narrow the helper, but may not contradict it.
terms = getattr(product, "term_fields", {}) or {}
for term_name, expected in (
("model", "black"), ("mc_distribution", process_family), ("sampling", "antithetic"),
):
if term_name in terms and terms[term_name] != expected:
failures.append(f"unsupported_forward_marginal_profile:{term_name}")
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Lite
| for term_name, expected in ( | ||
| ("model", "black"), ("mc_distribution", process_family), ("sampling", "antithetic"), | ||
| ): |
E22 previously relied on a title to supply cap economics and called its Monte Carlo lane a rate simulation. It now declares the exact USD cap, every fixing/payment period, separate accrual/option/curve clocks, named OIS/SOFR/Black-vol inputs, and seeded antithetic forward-marginal sampling. Both targets report USD holder present value against an explicit 0.5% analytical-reference tolerance.
The structured semantic bridge preserves those terms through method specialization and generated spec hydration. Monte Carlo consumes the authored 100,000 samples and seed 42; exact validation rejects conflicting seed aliases. Smoke fixtures preserve optional strip defaults instead of inventing callable or collar economics. The existing pricing kernels are unchanged.
Validation:
This PR remains draft: the full release gate is pending, and the legacy baseline/L64 counts must be regenerated after integration with the concurrent T73 repair. It does not claim short-rate paths, joint-forward dynamics, calibration or production conventions.
Updated quant, developer and user documentation, L64/L68, and the task migration/plan mirrors. Linear remains In Progress: QUA-1256.