fix(polymarket): admit bounded legacy trade rate limits - #503
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![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: bc5319ae5c
ℹ️ 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".
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deployment/aliyun/polymarket-shadow-gate-policy.jq (1)
41-56: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRate-limit exemption leaks into the completion snapshot, beyond the stated "startup health" scope.
legacy_health_snapshotis shared by both.baseline_health_snapshot(Line 137) and.baseline_health_completion_snapshot(Line 152). Addingbounded_legacy_trade_rate_limitshere therefore also relaxes the completion health check, even though the PR objectives scope this admission to "legacy Python startup health" / the "initial legacy baseline admission" only. As written, a legacy collector that is still hitting the trades rate limit right up to cutover (at the completion snapshot, just beforeproduction_eligible/passed) can pass Gate, which undermines exactly the stability guarantee the completion check exists to enforce.No test exercises
baseline_health_completion_snapshot.api_errorswith a rate-limit error to catch this (all coverage targetsbaseline_health_snapshot).🐛 Proposed fix: parameterize the exemption by call site
-def legacy_health_snapshot: +def legacy_health_snapshot($allow_bounded_rate_limits): (.updated_at | utc_iso8601_unix | type == "number") and (.last_success_at | type == "string" and length > 0) and (.target_markets | positive_integer) - and (.api_errors | bounded_legacy_trade_rate_limits) + and (.api_errors + | if $allow_bounded_rate_limits then bounded_legacy_trade_rate_limits + else . == [] end) and .malformed_trade_rows == 0 and .truncated_trade_markets == [] and .stale_trade_markets == [] and .stale_settlement_markets == [] and (.overdue_unresolved_markets | type == "array" and all(.[]; type == "string" and length > 0));Then update the two call sites:
- and (.baseline_health_snapshot | legacy_health_snapshot) + and (.baseline_health_snapshot | legacy_health_snapshot(true))- (.baseline_health_completion_snapshot | legacy_health_snapshot) + (.baseline_health_completion_snapshot | legacy_health_snapshot(false))As per coding guidelines, "Make surgical changes: every changed line must trace to the request" and "retain validation, security, data-loss prevention, and trust-boundary checks" — the completion check is a trust boundary that this shared def unintentionally weakens beyond the requested startup-only scope.
🤖 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 `@deployment/aliyun/polymarket-shadow-gate-policy.jq` around lines 41 - 56, Restrict the rate-limit exemption to the startup baseline only: parameterize legacy_health_snapshot (and its api_errors validation) with an allow-rate-limit flag, enable it for baseline_health_snapshot, and disable it for baseline_health_completion_snapshot. Preserve strict completion validation so any rate-limit error fails the completion health check.Source: Coding guidelines
🧹 Nitpick comments (1)
deployment/aliyun/polymarket-raw-ops-shadow-gate.sh (1)
478-496: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBounded rate-limit check duplicated between jq policy and shell script.
The
length <= 3/ regex logic inlegacy_start_health_policy_cleanexactly mirrorsbounded_legacy_trade_rate_limitsinpolymarket-shadow-gate-policy.jq. Two independent copies of a security-relevant boundary (the "3" threshold and exact regex) risk silent drift if one is updated without the other.Minimal mitigation: cross-reference both definitions with a comment, or better, extract the check into a shared jq snippet sourced by both call sites.
🤖 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 `@deployment/aliyun/polymarket-raw-ops-shadow-gate.sh` around lines 478 - 496, Cross-reference the duplicated bounded rate-limit validation in legacy_start_health_policy_clean and bounded_legacy_trade_rate_limits so their shared threshold and exact error-pattern boundary are clearly linked. Prefer reusing a shared jq snippet from both call sites; otherwise add explicit comments identifying the corresponding definition and requiring both implementations to remain synchronized.
🤖 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 `@deployment/aliyun/test-polymarket-raw-ops-control-plane.sh`:
- Around line 2570-2603: Extend the legacy rate-limit tests after the existing
baseline_health_snapshot cases by creating a fixture that mutates
.baseline_health_completion_snapshot.api_errors with the bounded legacy trades
rate-limit error, then run POLICY against it and assert rejection with a
failing-test message. Keep the existing baseline_health_snapshot acceptance and
excess/mixed rejection cases unchanged.
---
Outside diff comments:
In `@deployment/aliyun/polymarket-shadow-gate-policy.jq`:
- Around line 41-56: Restrict the rate-limit exemption to the startup baseline
only: parameterize legacy_health_snapshot (and its api_errors validation) with
an allow-rate-limit flag, enable it for baseline_health_snapshot, and disable it
for baseline_health_completion_snapshot. Preserve strict completion validation
so any rate-limit error fails the completion health check.
---
Nitpick comments:
In `@deployment/aliyun/polymarket-raw-ops-shadow-gate.sh`:
- Around line 478-496: Cross-reference the duplicated bounded rate-limit
validation in legacy_start_health_policy_clean and
bounded_legacy_trade_rate_limits so their shared threshold and exact
error-pattern boundary are clearly linked. Prefer reusing a shared jq snippet
from both call sites; otherwise add explicit comments identifying the
corresponding definition and requiring both implementations to remain
synchronized.
🪄 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 Plus
Run ID: 3d795f83-07b4-4f0b-827b-efb2c90b97e8
📒 Files selected for processing (3)
deployment/aliyun/polymarket-raw-ops-shadow-gate.shdeployment/aliyun/polymarket-shadow-gate-policy.jqdeployment/aliyun/test-polymarket-raw-ops-control-plane.sh
1 participant
Change contract
Allow only the Gate's legacy-Python startup admission to accept one to three exact trades-endpoint HTTP 429 errors while preserving the original errors in immutable evidence and keeping shared cutover/Rust health policies strict.
Out of scope
Legacy Python retry/backoff or redeployment, Rust collector behavior, CI restructuring, research, snapshot/235, evaluator/MCTS, and production cutover.
Dependency or merge order
None. Merge directly to
main; the resulting exact-main artifact feeds the existing Polymarket runtime rollout.Focused validation
gate policy rejected a bounded legacy trades rate limit.bash deployment/aliyun/test-polymarket-raw-ops-control-plane.shbash -n deployment/aliyun/polymarket-raw-ops-shadow-gate.sh deployment/aliyun/test-polymarket-raw-ops-control-plane.shshellcheck deployment/aliyun/polymarket-raw-ops-shadow-gate.sh deployment/aliyun/test-polymarket-raw-ops-control-plane.shCounterexamples cover four rate limits, wrong endpoint, HTTP 500, timeout, mixed lists, strict shared legacy policy, strict Rust policy, and verbatim Gate evidence preservation.
Rollout and rollback impact
After merge, build a new exact-main immutable release, rerun the existing real closed-segment preflight, then run the strict 900-second Gate. Any other legacy error, identity/freshness failure, Rust/data/OSS/parity failure, or Gate failure remains fail-closed and preserves Python. Rollback uses the previous immutable control bundle.
Issue relationship
Closes #502