fix(prediction): preserve token-side factor identity - #149
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe change adds optional UP/DOWN review-side identity to factor previews and registry rows, validates side-bound exports, separates database uniqueness, filters legacy runtime consumers to pooled rows, and updates Alpha Zoo exports, tests, CI, and documentation. ChangesSide-aware research artifact identity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant persist_research_trace
participant factor_registry
participant AlphaZooSnapshot
CLI->>persist_research_trace: provide optional UP/DOWN export side
persist_research_trace->>factor_registry: persist review_side with factor identity
persist_research_trace->>factor_registry: fetch rows including review_side
factor_registry-->>persist_research_trace: return pooled and side-bound rows
persist_research_trace->>AlphaZooSnapshot: group rows by target and review_side
AlphaZooSnapshot-->>persist_research_trace: emit versioned snapshot
Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@rust_hft/prediction-markets/migrations/051_factor_registry_review_side.sql`:
- Line 19: Update the DROP INDEX statement in migration
051_factor_registry_review_side.sql to use IF EXISTS, matching the existing
idempotent ADD COLUMN behavior and allowing safe partial or repeated migration
runs.
🪄 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
Run ID: ef819d7c-df53-4716-91e4-aa15211aad7f
📒 Files selected for processing (7)
.github/workflows/ploy-ci.ymlrust_hft/prediction-markets/crates/ploy-research/examples/persist_research_trace.rsrust_hft/prediction-markets/crates/ploy-research/examples/research_trace_plan.rsrust_hft/prediction-markets/docs/ALPHA_FACTOR_SEARCH_CICD.mdrust_hft/prediction-markets/docs/runbooks/strategy-research-cicd.mdrust_hft/prediction-markets/migrations/051_factor_registry_review_side.sqlrust_hft/prediction-markets/tasks/todo.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abbd28f2cc
ℹ️ 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.
🧹 Nitpick comments (1)
rust_hft/prediction-markets/crates/ploy-research/examples/research_trace_plan.rs (1)
808-960: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest exercises only the top-level
sidepredicate; nestedsource_factor.sideis uncovered. Each filtered query added two exclusion clauses (output_json->>'side'andoutput_json#>>'{source_factor,side}'), but the inserted traces only set a top-levelside, so a regression in the nested-path predicate would pass unnoticed. Consider adding one side-bound row whose side lives only undersource_factor.side.Minor fidelity nit: the registry
review_sideuses"up"(matching the migrationup|downcheck) while the traceoutput_json.sideuses"Up". Aligning casing with the productionReviewSideserialization keeps the fixture representative.🤖 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 `@rust_hft/prediction-markets/crates/ploy-research/examples/research_trace_plan.rs` around lines 808 - 960, Extend postgres_legacy_manager_excludes_side_bound_trace_and_evaluation to insert a side-bound trace whose output_json contains source_factor.side without a top-level side, so nested-path filtering is exercised; update assertions to ensure its marker is excluded from latest runs and rejected patterns. Also align the existing trace side value casing with the production ReviewSide serialization while preserving the registry review_side value.
🤖 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.
Nitpick comments:
In
`@rust_hft/prediction-markets/crates/ploy-research/examples/research_trace_plan.rs`:
- Around line 808-960: Extend
postgres_legacy_manager_excludes_side_bound_trace_and_evaluation to insert a
side-bound trace whose output_json contains source_factor.side without a
top-level side, so nested-path filtering is exercised; update assertions to
ensure its marker is excluded from latest runs and rejected patterns. Also align
the existing trace side value casing with the production ReviewSide
serialization while preserving the registry review_side value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69079beb-5dc3-43fe-ab0b-0c0a653aa630
📒 Files selected for processing (3)
.github/workflows/ploy-ci.ymlrust_hft/prediction-markets/crates/ploy-research/examples/research_trace_plan.rsrust_hft/prediction-markets/migrations/051_factor_registry_review_side.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- rust_hft/prediction-markets/migrations/051_factor_registry_review_side.sql
Change contract
Persist AutoFactor registry identity as
(dsl_hash, target, horizon, optional review_side)and export Alpha Zoo snapshots for one exacttarget × optional side, while quarantining side-bound rows from every legacy Research Manager input.Out of scope
Dependency / merge order
Depends on merged PR #147. Merge this registry/Zoo layer before the candidate-replay identity layer. Legacy replay/runtime queries remain restricted to pooled artifacts (
review_side IS NULLor no side-bearing artifact root) until that later layer is merged.Focused validation
git diff --checkrustfmt --edition 2021 --checkfor both modified Rust examplesCounterexamples covered:
Rollout / rollback impact
Migration 051 is a forward-only identity widening for the manual Research OS writer; these examples are not a rolling service deployment. It adds nullable
review_side, validatesup|down, creates the four-column unique index withNULLS NOT DISTINCT, then removes the old three-column conflict arbiter. Historical rows are not backfilled and remain pooled (NULL).After migration 051, an older
persist_research_tracebinary deliberately fails closed because its three-columnON CONFLICTarbiter no longer exists. Do not perform a binary-only rollback. Restoring an older writer requires a coordinated database rollback that first archives side-bound factor rows and dependent evaluations/traces, then restores the old three-column index. No rollback step may silently discard or pool Up/Down identity.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests