Add freshness and data quality gates#35
Conversation
🤖 Codex PR Review🚫 Merge blocked: 2 serious issue(s) found in high-risk files ⚖️ Codex Review Arbitration🚫 block: 🚫 Blocking IssuesThese issues must be fixed before this PR can be merged: 1. 🟠 [HIGH] Logic in
|
9137b42 to
c18b468
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9137b4235c
ℹ️ 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".
| if expires_at < dt.date.today() and not args.allow_expired: | ||
| raise SystemExit(f"expired signal artifact: expires_at={expires_at.isoformat()}") |
There was a problem hiding this comment.
When data/output/latest_signal.json is expired, this default failure blocks the scheduled long-horizon refresh path: .github/workflows/dispatch_shadow_signal.yml invokes python scripts/validate_latest_signal.py --allow-missing before building the context bundle or opening the shadow-signal issue. The committed artifact expires on 2026-06-30, so any run after that date (including the next monthly run) exits here and cannot create the request that would replace the expired signal; the refresh workflow should pass --allow-expired or otherwise treat expiry as a warning in that preflight step.
Useful? React with 👍 / 👎.
| "as_of": snapshot_as_of, | ||
| "generated_at": (generated_at or dt.datetime.now(dt.UTC)).isoformat().replace("+00:00", "Z"), | ||
| "generated_at": timestamp.isoformat().replace("+00:00", "Z"), | ||
| "expires_at": (parse_price_date(snapshot_as_of) + dt.timedelta(days=THEME_MOMENTUM_EXPIRY_DAYS)).isoformat(), |
There was a problem hiding this comment.
When the theme snapshot workflow is manually dispatched with a future as_of value, the builder filters prices with that cutoff but still stamps snapshot_as_of from the requested date, so this new expiry is minted from a date beyond the latest available prices and validate_theme_momentum_snapshot.py accepts it. For example, running on 2026-07-13 with prices through 2026-07-10 and --as-of 2026-12-31 produces as_of=2026-12-31, expires_at=2027-03-25, and no stale warning, causing downstream consumers that rely on expiry to treat a stale/future-dated artifact as fresh.
Useful? React with 👍 / 👎.
| "status": "blocked" if quality_warnings else "pass", | ||
| "allow_downstream_recommendation": not quality_warnings, | ||
| "reasons": quality_warnings, |
There was a problem hiding this comment.
If the snapshot is built from a local prices file that omits the configured exposure symbols, or all matching symbols have too little history to produce momentum scores, quality_warnings remains empty and this new gate reports pass/allow_downstream_recommendation=true even though priced_symbol_count or usable ranks are zero. That makes downstream consumers that follow the new gate treat an empty or unscorable snapshot as healthy; the gate should also consider missing coverage / insufficient-history conditions before allowing recommendations.
Useful? React with 👍 / 👎.
f6453b7 to
d7ac1f6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7ac1f65a9
ℹ️ 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".
| if generated_date - parse_price_date(snapshot_as_of) > dt.timedelta(days=MAX_PRICE_AGE_DAYS): | ||
| quality_warnings.append( | ||
| f"stale price data: latest as_of {snapshot_as_of} is older than {MAX_PRICE_AGE_DAYS} days" | ||
| ) |
There was a problem hiding this comment.
When the input has mixed symbol recency, such as one exposure symbol ending weeks before another symbol's latest row, snapshot_as_of is the maximum date across all symbols, so this check can leave data_quality.gate as pass while the stale symbol is still included in priced_members and top_symbols. That defeats the new stale-price safety gate for partial downloads or local CSVs with lagging symbols; compare each symbol's own symbol_data["as_of"] to the generated date or exclude/flag lagging symbols.
Useful? React with 👍 / 👎.
| if check_freshness and dt.date.fromisoformat(payload["expires_at"]) < (reference_date or dt.date.today()): | ||
| raise SignalValidationError(f"signal artifact expired on {payload['expires_at']}") |
There was a problem hiding this comment.
Because the new default compares every validation to dt.date.today(), the unit tests that load the static example artifact and call validate_signal(payload) become time-dependent: after examples/latest_signal.example.json expires on 2026-12-31, those tests will fail before exercising their intended schema assertions. CI already passes --allow-expired for the script path, so the pytest callers should similarly pin reference_date or disable freshness when testing schema behavior.
Useful? React with 👍 / 👎.
| if schema_version == "2": | ||
| for key in ("model_version", "scoring_version"): | ||
| _require_string(payload.get(key), key) |
There was a problem hiding this comment.
With this branch, a fresh shadow artifact can set schema_version to "1" and omit both model_version and scoring_version, and validate_latest_signal.py still accepts it because the metadata check only runs for "2". The issue prompt still only asks the bridge to write data/output/latest_signal.json, so an old-format output can bypass the new versioned-metadata contract; require these fields for current/latest validation or reserve schema v1 only for explicit replay paths.
Useful? React with 👍 / 👎.
| if reference_date and expires_at < reference_date: | ||
| raise ValueError(f"theme momentum snapshot expired on {expires_at.isoformat()}") |
There was a problem hiding this comment.
A freshly built snapshot with no warnings gets allow_downstream_recommendation=true and an expiry 84 days after as_of, but after MAX_PRICE_AGE_DAYS elapses this validator still accepts it because it only checks the long expiry date. Downstream consumers that rely on validate_theme_momentum_snapshot.py and the stored gate can therefore treat 1- to 12-week-old prices as healthy; the validator should compare as_of to reference_date and fail or flip the gate once the stale-price window is exceeded.
Useful? React with 👍 / 👎.
d7ac1f6 to
ea0e692
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea0e692246
ℹ️ 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".
| if as_of_date and actual_latest_date and as_of_date > actual_latest_date: | ||
| raise ValueError( | ||
| f"as_of cannot be later than latest available price date {actual_latest_date.isoformat()}" | ||
| ) |
There was a problem hiding this comment.
When a manual run supplies --as-of for a weekend/holiday or a local price file whose last row is the previous trading day, this check raises even though the input is a valid inclusive cutoff. Because actual_latest_date is computed after filtering rows to <= as_of, any cutoff that does not exactly match a price row is treated as invalid; the workflow input describes this as a cutoff, so it should either stamp/use the latest available price date or only reject genuinely future/unavailable data.
Useful? React with 👍 / 👎.
| required = ("schema_version", "as_of", "generated_at") | ||
| missing = [key for key in required if not snapshot.get(key)] | ||
| if missing: | ||
| raise ValueError(f"theme momentum snapshot missing required keys: {', '.join(missing)}") |
There was a problem hiding this comment.
The new workflow validation step relies on this validator before publishing theme_momentum_snapshot.json, but the required set only covers dates and schema metadata, so a v2 file with a gate but no mode, artifact_type, theme_ranks, or research-only policy still passes as a valid theme momentum snapshot. Add value checks for the snapshot identity fields at minimum, otherwise a miswired or truncated builder output can be uploaded/committed despite not being a theme snapshot.
Useful? React with 👍 / 👎.
ea0e692 to
c03c504
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c03c5046d7
ℹ️ 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 .theme_momentum import validate_theme_momentum_snapshot | ||
|
|
||
| try: | ||
| validate_theme_momentum_snapshot(theme_payload, reference_date=reference_date) |
There was a problem hiding this comment.
When callers use the default CLI path without --reference-date, validate_signal(..., check_freshness=True) checks the latest signal against today's UTC date, but this call passes None into the referenced theme snapshot validator. Since validate_theme_momentum_snapshot only applies its expiry check when reference_date is truthy, a still-fresh latest signal can validate while pointing at a theme snapshot whose expires_at is already in the past; compute the effective reference date once and pass it to both validators.
Useful? React with 👍 / 👎.
| for symbol, symbol_data in symbol_scores.items(): | ||
| metrics = symbol_data["metrics"] |
There was a problem hiding this comment.
When a caller passes price rows for a broader universe than the configured exposures, this new gate scans every symbol in symbol_scores; any extreme return or split-like move in an unrelated extra symbol adds a warning and blocks allow_downstream_recommendation, even though the theme ranks below only use symbols present in the exposure map. The quality gate should iterate the priced exposure symbols so unrelated rows cannot falsely block an otherwise healthy snapshot.
Useful? React with 👍 / 👎.
| theme_payload = json.loads(artifact.read_text(encoding="utf-8")) | ||
| from .theme_momentum import validate_theme_momentum_snapshot | ||
|
|
||
| try: | ||
| validate_theme_momentum_snapshot(theme_payload, reference_date=reference_date) |
There was a problem hiding this comment.
The strict validator loads the referenced theme snapshot but never checks its point-in-time date against the signal. The committed latest_signal.json is as_of 2026-06-25 while its referenced theme_momentum_snapshot.json is as_of 2026-06-26, so a replay or audit can accept a signal that used future theme data relative to its own cutoff; compare the referenced snapshot as_of to the signal as_of before accepting it.
Useful? React with 👍 / 👎.
c03c504 to
718f834
Compare
Co-Authored-By: Codex <noreply@openai.com>
718f834 to
b43064d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b43064d733
ℹ️ 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".
| if gate.get("allow_downstream_recommendation") is not True: | ||
| raise SignalValidationError("referenced theme momentum artifact is blocked by data_quality.gate") |
There was a problem hiding this comment.
In the checked refresh workflow (.github/workflows/dispatch_shadow_signal.yml:53), the preflight runs validate_latest_signal.py --allow-missing before it can build the new context or open the replacement issue. This new gate check makes that preflight fail for the artifact committed in this PR because data/output/theme_momentum_snapshot.json has allow_downstream_recommendation=false; even adding --allow-expired would still exit here, so the scheduled/manual refresh path cannot replace the blocked artifact. Use schema-only validation or a preflight mode that treats the existing linked gate as a warning during refresh.
Useful? React with 👍 / 👎.
| ARGS+=(--strict-downloads) | ||
| fi | ||
| python scripts/build_theme_momentum_snapshot.py "${ARGS[@]}" | tee /tmp/theme_momentum_summary.json | ||
| python scripts/validate_theme_momentum_snapshot.py data/output/theme_momentum_snapshot.json |
There was a problem hiding this comment.
When workflow_dispatch supplies a historical as_of more than 84 days before the run date, the builder stamps expires_at from that cutoff, but this validation call provides neither --reference-date nor --allow-expired; scripts/validate_theme_momentum_snapshot.py therefore defaults freshness to today's UTC date and fails before upload/commit. For replaying a saved cutoff such as 2026-01-02 on 2026-07-13, pass the requested cutoff as the reference date or allow expired snapshots for historical dispatches.
Useful? React with 👍 / 👎.
| if check_freshness and dt.date.fromisoformat(payload["expires_at"]) < ( | ||
| reference_date or dt.datetime.now(dt.UTC).date() | ||
| ): | ||
| raise SignalValidationError(f"signal artifact expired on {payload['expires_at']}") |
There was a problem hiding this comment.
When strict/latest validation is run with a reference date, this freshness gate only checks whether expires_at is before that date and never rejects an as_of that is after it. A bridge output dated 2026-12-31 with a later expiry is accepted on 2026-07-13, so current consumers can ingest future-dated signal context; compare as_of to the effective reference date before accepting latest artifacts.
Useful? React with 👍 / 👎.
| "as_of": "2026-06-25", | ||
| "generated_at": "2026-06-27T08:22:28.245044Z", | ||
| "history_artifact": "data/output/signal_history/2026-06-25.json", |
There was a problem hiding this comment.
This manifest now points at the 2026-06-25 latest/history artifacts, but the unchanged validation block still records status: passed for commands that no longer pass with these artifacts: the default latest-signal validation rejects the committed expired signal, and the history command is also expired under the new freshness check. Consumers that trust the manifest provenance will treat an unvalidated artifact as validated; update the recorded commands/status to reflect the actual validation mode and outcome.
Useful? React with 👍 / 👎.
|
Closing this accumulated remediation branch after repeated semantic review findings and contract arbitration. The P0 work will be re-sliced from current main into smaller vertical PRs with isolated contracts and tests. No code from this branch is being merged as-is. |
Summary
Validation
python3 -m pytest -q(42 passed)python3 scripts/validate_latest_signal.py examples/latest_signal.example.json --allow-expiredpython3 scripts/validate_theme_momentum_snapshot.py data/output/theme_momentum_snapshot.jsonexpires_at=2026-06-30.Research-only scope; no execution, portfolio, account, or production dependency changes.