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. |
![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: 16873d92e2
ℹ️ 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 manifest-ordering bypass and two disposition-normalization inconsistencies remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR formalizes T82 as a non-pricing callable-analytics proof hold and blocks unsupported execution.
Changes:
- Adds exact hold validation and runtime guards.
- Adds regression tests for contract integrity and execution blocking.
- Updates baselines, documentation, limitations, and migration tracking.
File summaries
| File | Summary |
|---|---|
trellis/agent/task_runtime.py |
Preserves T82 hold descriptions and semantic inspection. |
trellis/agent/task_manifest_validation.py |
Validates the exact hold contract and execution disposition. |
tests/test_tasks/test_t82_callable_analytics_hold.py |
Tests contract integrity and execution boundaries. |
TASKS_PROOF_LEGACY.yaml |
Authors the T82 proof hold. |
TASKS_PROOF_LEGACY_BASELINE.yaml |
Updates legacy debt metadata. |
LIMITATIONS.md |
Documents callable analytics limitations. |
docs/user_guide/pricing.rst |
Documents T82 user behavior. |
docs/quant/pricing_stack.rst |
Documents callable analytics semantics. |
docs/developer/task_and_eval_loops.rst |
Documents validation and execution boundaries. |
doc/plan/active__legacy-task-migration-map.md |
Updates T82 migration status. |
Review details
Suppressed comments (2)
trellis/agent/task_runtime.py:1206
- The manifest validator normalizes this field with
_text, so a held row withtask_disposition: "proof_hold "is admitted as a valid T82 hold. This new guard compares the raw value, sotask_to_description()falls through to the benchmark/title-derived prompt instead of preserving the authored hold, making the promised title-independent inspection inconsistent. Normalize the disposition here as well (or reject non-canonical whitespace during validation).
if (
str(task.get("id") or "").strip() == "T82"
and task.get("task_disposition") == "proof_hold"
trellis/agent/task_runtime.py:1360
- This second guard has the same normalization mismatch:
_validate_legacy_callable_bond_comparison_contract()accepts the disposition after stripping whitespace, but_effective_task_description()does not. A task that the admission boundary recognizes as a valid hold can therefore still receive a title-derived description when this helper is used directly. Use the same stripped comparison as the validator.
"""Return the task description after applying any canonical bootstrap prompt."""
if (
str(task.get("id") or "").strip() == "T82"
and task.get("task_disposition") == "proof_hold"
- Files reviewed: 10/10 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.
|
@codex review |
|
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
The cached benchmark path bypasses the T82 admission hold, and direct execution does not preserve the originating validation root.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
trellis/agent/task_manifest_validation.py:344
- This new T82 validation is root-sensitive:
_validate_legacy_callable_bond_comparison_contractcompares the materialized market against fixture/scenario data loaded fromroot, but the directrun_task()path still callsassert_executable_task_disposition([task])without a caller root. A valid T82 loaded viaload_task_manifest(..., root=custom_root)can therefore be checked against the checkout's scenario digest/contract and rejected even though it matches its own repository; thread the originating root through the direct execution boundary as well.
contract_issues = _validate_legacy_callable_bond_comparison_contract(
LEGACY_TASKS_MANIFEST,
task,
f"selected_tasks[{index}]",
root=root,
)
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
| Retain the named USD fixed-coupon callable-bond fixture for the requested | ||
| vega, OAS, duration, and scenario analysis. Do not build or price T82 until | ||
| its missing analytics definitions and callable references are authored. | ||
| task_disposition: proof_hold |
| """Reject governed legacy holds at the shared execution boundary.""" | ||
| issues: list[TaskManifestIssue] = [] | ||
| for index, task in enumerate(tasks): | ||
| if _text(task.get("task_definition_manifest")) != LEGACY_TASKS_MANIFEST: | ||
| # T82 is a reserved exact hold. Mutable loader-provenance fields on a | ||
| # direct-call mapping must not opt it out of its admission boundary. | ||
| is_t82 = _text(task.get("id")) == "T82" |
|
Main integration is now pushed as 59782cf. The 309 focused T82/manifest/dated-curve tests pass; all seven manifest audits pass with zero blockers and the unchanged 585-debt baseline. The full local release gate is running. This PR remains draft until release validation and reviews complete. @codex review |
|
Codex Review: Didn't find any major issues. Swish! 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
Cached and generated benchmark entrypoints bypass the T82 admission gate and can reach pricing.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
trellis/agent/task_manifest_validation.py:343
- The new admission boundary is still not applied to the cached benchmark entrypoints.
benchmark_existing_task()(andbenchmark_generated_artifact()) do not callassert_executable_task_disposition;scripts/benchmark_tasks.pyloads the legacy pricing corpus and createsmarket_statebefore invoking them, so selecting T82 there can enterprepare_existing_task()and eventuallyprice_fninstead of returning the authored hold. Please route these benchmark paths through the same exact-disposition gate before market construction, or exclude governed holds at selection; otherwise the statedzero_market_builder_llm_pricer_attemptspolicy is bypassable.
contract_issues = _validate_legacy_callable_bond_comparison_contract(
LEGACY_TASKS_MANIFEST,
task,
f"selected_tasks[{index}]",
root=root,
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| if ( | ||
| str(task.get("id") or "").strip() == "T82" | ||
| and str(task.get("task_disposition") or "").strip() == "proof_hold" | ||
| ): | ||
| return str(task.get("description") or "").strip() |
Summary
proof_holdover the existing three-call-date fixed-coupon fixture and named market.Validation
Integration status
This remains a draft. The coordinating implementation is landing the separate T73 review corrections first; main-branch integration and combined legacy-baseline recomputation will follow. The full release gate has not yet run for this slice. No merge until that validation and automated review are complete.
T82's expected outcome is an admission error with no pricing result, not a successful pricing or honest-block certificate. Future executable analytics require the missing definitions. QUA-1258 owns T89 duration; QUA-1259 owns title-bootstrap cleanup. The consolidated ticket-state plan table is maintained separately.
Linear: https://linear.app/quant-macro/issue/QUA-1262/semantic-callable-analytics-exact-t82-proof-hold