Repository navigation
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: 5e2d1f0798
ℹ️ 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".
| from quant_platform_kit.notifications.renderer_base import render_asset_management_error | ||
| from quant_platform_kit.notifications.unified_contract import NotificationEvent |
There was a problem hiding this comment.
With the repository's ordinary frozen dependencies, this import fails because the new renderer API is unavailable in the QPK revision still pinned by pyproject.toml and uv.lock; the commit's verification notes explicitly confirm this incompatibility. Runtime issue notifications now call this helper even with unified notifications disabled, so existing installations lose error delivery, and the PAPER watchdog's storage-failure fallback can itself raise instead of sending an alert. Update the dependency pin and lock together with this change, or provide a renderer compatible with the current pin, rather than requiring an external source override for these production paths.
Useful? React with 👍 / 👎.
| def legacy_notify_issue(title, detail): | ||
| from notifications.renderers import render_runtime_error | ||
| # Raw broker detail belongs to controlled diagnostics, never the body. | ||
| return send_recorded_message(render_runtime_error(kind="real_error")) |
There was a problem hiding this comment.
When a quote lookup, purchase-quantity estimate, or order-status poll raises, its caller passes the exception details to this notifier without logging them separately (safe_quote_last_price, estimate_cash_buy_quantity_safe, and monitor_submitted_order_status). The previous build_issue_notifier printed the title and detail through its publisher's logging sink; this replacement discards both arguments and only sends a generic error body. Consequently, these failures now lose their exception message or traceback entirely, even when notification delivery succeeds. Keep the sanitized notification body, but preserve the diagnostic information in the restricted logging path before sending it.
Useful? React with 👍 / 👎.
| event = NotificationEvent(kind, stamp, platform_id="longbridge", | ||
| strategy_profile=report.get("strategy_profile"), | ||
| incident_reason=kind) | ||
| return render_asset_management_error(event, locale=locale or os.environ.get("NOTIFY_LANG") or "zh-CN") |
There was a problem hiding this comment.
When QSL_NOTIFY_LANG=en overrides NOTIFY_LANG=zh, runtime configuration correctly selects English (runtime_config_support.py:352), but the new runtime issue and order-error paths call this helper without an explicit locale. This fallback ignores QSL_NOTIFY_LANG and renders those operational alerts in Chinese instead. The same mismatch occurs when only QSL_NOTIFY_LANG=en is set, because this helper defaults to Chinese. Resolve the locale using the existing QSL-first precedence, or pass the configured translator's locale from the runtime adapter, so urgent alerts follow the recipient's configured language.
Useful? React with 👍 / 👎.
AI assistance
Codex; model selected by the host (not independently available to this worker).
What changed
Persist completed fills and verified partial observations as routine facts instead of immediate order notifications, preserve partial-to-timeout error reporting, and retain PAPER recovery/maintenance records as pending central ingestion. Use the shared runtime-error renderer on the retained unified/error routes.
Preserve legacy flag-off heartbeat bodies, runtime-guard localized operational alerts with no provider audit payload, and PAPER incident-bearing active alerts. These bounded compatibility fixes remove all 21 notification-body regressions identified in the retained patch, without editing their assertions.
How verified
Known-red Draft, not green. Owner t1234s7038 A explicitly authorizes publication with exactly the ten shared baseline failures. Base: c9cb033. QPK #774 source pin: aa93e12bfa64431bf71cf59b9b24c0709ca1b76a. Both source archives were verified byte-for-byte; remote main and QPK pin were read back unchanged.
Same Python 3.12 virtualenv, immutable QPK source via PYTHONPATH,
pytest -q -p no:cacheprovider tests:Ten inherited failures: one PAPER receipt assertion expects broker permission despite policy denial; three tests lack installed-source provenance in the source-override environment; two risk-attention body assertions and one strategy display-name assertion differ from shared rendering; three invalid safety-boolean cases expect permissive legacy parsing. No safety/authentication/provenance gate was weakened to change these outcomes. Native red CI statuses remain binding.
Compatibility limitation: ordinary frozen dependencies still fail both new-renderer checks with ImportError. The dependency lock is unchanged. These results establish only the explicitly pinned source path, not ordinary-lock compatibility or deployment readiness. The exception is limited to this exact candidate/base/pin/environment and invalidates on drift; it grants no merge permission.
Why
The retained notification-unification patch changed established heartbeat/runtime/PAPER bodies and introduced 21 additional failures. Preserve those body contracts while retaining the bounded routing and persistence work.
Risk
Notification and routine-record behavior only. PAPER records remain pending_central; consumer adoption, authentic producer/run links, delivery receipts and runtime reliability are NOT_PROVEN. No workflows, risk gates, deployment settings, Secret/IAM, live, Kelly or stop-loss configuration changed. No real notifications or deployment performed. Draft publication does not authorize merge or enablement.
Central publication scan
publication-check/2.0-advisory, build 83d9d1130377, advisory exit 0: 1464 findings / 1453 matching lines in the complete candidate tree plus routing test. First 20 matches (masked by the shared checker):
.env.example:21:TELEGRAM_TOKEN=<<HOST>>.<PROJECT_ID>/CODEOWNERS:7:/.env.example @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:8:/.github/CODEOWNERS @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:9:/.github/workflows/ @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:10:/application/account_new_risk_gate_support.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:11:/application/durable_execution_commands.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:12:/application/execution_receipt_adapter.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:13:/application/execution_service.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:14:/application/execution_state.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:15:/application/longbridge_execution.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:16:/application/paper_execution_command_consumer.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:17:/application/paper_strategy_risk_state.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:18:/runtime_execution_policy.py @<PROJECT_ID>.<PROJECT_ID>/CODEOWNERS:19:/scripts/execution_report_heartbeat.py @<PROJECT_ID>.<PROJECT_ID>/dot-review/review.cjs:106:const token = <REDACTED> core.getIDToken(audienceText); // Exact bytes; URL normalization may change an audience..<PROJECT_ID>/dot-review/review.cjs:190:const <PROJECT_ID> = {...pr,head:{...pr.head,sha:pair.target},base:{...pr.base,sha:pair.base}};.<PROJECT_ID>/dot-review/review.cjs:191:const binding = {...b.binding({repository:trust.repo.full_name,pr:<PROJECT_ID>,policyCommit:trust.commit,files},trust.policySha),.<PROJECT_ID>/dot-review/test-review.cjs:81:const content='<PROJECT_ID> source\n', sourceSHA='d'.repeat(40);.<PROJECT_ID>/dot-review/test-review.cjs:82:const pr={number:7,changed_files:1,commits:1,title:'<PROJECT_ID> change',body:'<PROJECT_ID> body',.<PROJECT_ID>/dot-review/test-review.cjs:100:assert.equal(audience,'https://audience.example.test');return '<PROJECT_ID>-only';},summary};Exact known-red test identities
tests/test_durable_execution_commands.py::test_paper_producer_persists_observation_gate_receipt_without_authorizing_a_consumertests/test_notification_archive_finalizer.py::test_finalizer_and_receiver_use_exact_recorded_sourcestests/test_notification_consolidation.py::test_risk_attention_persists_transitions_without_raw_strategy_keys[en]tests/test_notification_consolidation.py::test_risk_attention_persists_transitions_without_raw_strategy_keys[zh]tests/test_runtime_composer.py::test_plugin_error_is_structured_attention_even_without_display_linestests/test_runtime_composer.py::test_runtime_cycle_success_is_quiet_even_in_authorized_live_sessiontests/test_runtime_config_precedence.py::test_cash_only_platform_pair_precedes_generic_pair[values7-False]tests/test_runtime_config_precedence.py::test_dry_run_alias_truth_table[maybe-true-False-False]tests/test_runtime_config_precedence.py::test_dry_run_alias_truth_table[maybe-true-False-True]tests/test_runtime_strategy_adapters.py::test_runtime_strategy_adapters_builds_escalated_plugin_alert_message