Skip to content

Fix: add cold-start candidate guardrails - #331

Merged
ohld merged 5 commits into
productionfrom
fix/ffm-1882-cold-start-candidate-filter
Jul 16, 2026
Merged

Fix: add cold-start candidate guardrails#331
ohld merged 5 commits into
productionfrom
fix/ffm-1882-cold-start-candidate-filter

Conversation

@ohld

@ohld ohld commented Jul 14, 2026

Copy link
Copy Markdown
Member

Fixes FFM-1882.

Summary

  • Add COLD_START_CANDIDATE_GUARDRAILS_ENABLED rollback flag for the cold-start candidate filter experiment.
  • Exclude weak source URLs from guarded cold_start_explore / cold_start_adapt candidate SQL for true-new positions 2-10 only, including mixed initial batches where position 1 remains control.
  • Persist treatment attribution via cold_start_explore_guarded / cold_start_adapt_guarded and include those engines in the first-10 readout.
  • Preserve existing text_light_lr_smoothed / best_uploaded_memes fallbacks when guarded cold-start pools are empty.

Rollback

Set COLD_START_CANDIDATE_GUARDRAILS_ENABLED=false in production env.

Verification

  • ruff check --fix src/ tests/ && ruff format src/ tests/
  • DATABASE_URL=postgresql+asyncpg://user:pass@localhost/test REDIS_URL=redis://localhost:6379/0 CORS_ORIGINS='[]' CORS_HEADERS='[]' pytest tests/recommendations/test_cold_start_candidate_guardrails.py tests/recommendations/test_pipeline.py tests/recommendations/test_meme_queue.py tests/stats/test_cold_start_quality.py::test_readout_includes_guarded_cold_start_treatment_engines -q

Notes for Analyst / QA

  • Analyst can compare treatment using user_meme_reaction.recommended_by IN ('cold_start_explore_guarded', 'cold_start_adapt_guarded') and fallback diagnostics via cold_start_candidate_guardrails_applied + fallback_used.
  • Full stats integration tests still require the project DB fixture.

@ohld

ohld commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

Found two correctness blockers in the cold-start guardrail segmentation:

  1. src/recommendations/pipeline.py computes guarded positions from request.nmemes_sent only. Normal queue refills run while Redis still has queued memes (check_queue refills at <= 8 and appends 15), so appended candidates can be actual positions 16+ while the new code treats them as positions 8-22 and applies guarded attribution/filtering outside the position 2-10 experiment window.

  2. Partial guarded segments can fall through into the trailing unguarded segment. If the guarded fetch returns fewer candidates than its segment limit, the code continues and can fill still-guarded positions with unguarded candidates instead of falling back/stopping the guarded segment.

Tests/verification run: targeted pytest for new guardrail tests passed with DATABASE_URL=postgresql+asyncpg://user:pass@localhost:5432/test; GitHub CI is green. CSO raw-SQL pass found no injection/secrets issue; the SQL fragments use bounded internal constants plus bind params. Codex review independently flagged the two issues above.

@ohld

ohld commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

STAFF ENGINEER REVIEW: CHANGES REQUESTED — Found two correctness blockers in the cold-start guardrail segmentation:

  1. src/recommendations/pipeline.py computes guarded positions from request.nmemes_sent only. Normal queue refills run while Redis still has queued memes (check_queue refills at <= 8 and appends 15), so appended candidates can be actual positions 16+ while the new code treats them as positions 8-22 and applies guarded attribution/filtering outside the position 2-10 experiment window.

  2. Partial guarded segments can fall through into the trailing unguarded segment. If the guarded fetch returns fewer candidates than its segment limit, the code continues and can fill still-guarded positions with unguarded candidates instead of falling back/stopping the guarded segment.

Tests/verification run: targeted pytest for new guardrail tests passed with DATABASE_URL=postgresql+asyncpg://user:pass@localhost:5432/test; GitHub CI is green. CSO raw-SQL pass found no injection/secrets issue; the SQL fragments use bounded internal constants plus bind params. Codex review independently flagged the two issues above.

@ohld

ohld commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

STAFF ENGINEER REVIEW: CHANGES REQUESTED — CI is red on the recommendation path changed by this PR. pytest tests/ -x -q fails at tests/recommendations/test_meme_queue.py::test_cold_start_phase1_uses_explore with ValueError: engine text_light_lr_smoothed is not supported after _fetch_primary_engine falls back from a partial guarded cold-start result. I reproduced the same failure locally in a clean PR worktree with the targeted recommendation test command.

Please fix the guarded-segment underfill/fallback behavior or update the affected test harness intentionally, then push a new commit. Raw SQL security check: the new source filter is parameterized (ANY(:cold_start_guardrail_source_urls)), and I did not find a SQL injection or secret leak in this diff.

@ohld

ohld commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

STAFF ENGINEER REVIEW: CHANGES REQUESTED — Latest head a74ea47 still has red CI. pytest tests/ -x -q now fails at tests/recommendations/test_queue_correctness.py::test_cold_start_below_30: generate_recommendations(... nmemes_sent=10 ...) returns [] instead of the expected lr-smoothed candidates.

The new _retriever_supports_engine guard appears to overfit to engine_map: the queue-correctness stub sets engine_map = {} but implements get_candidates dynamically, including aliasing text_light_lr_smoothed to lr_smoothed. With the new guard, _run_fallbacks skips every fallback for that retriever, changing the tested below-30 fallback behavior.

Please fix the fallback support detection or update the test contract intentionally, then push another commit. Auto-merge remains disabled.

@ohld

ohld commented Jul 14, 2026

Copy link
Copy Markdown
Member Author

STAFF ENGINEER REVIEW: CHANGES REQUESTED — rollback is not immediate for already queued guarded recommendations.

Blocking issue:

  • [P2] COLD_START_CANDIDATE_GUARDRAILS_ENABLED=false stops new guarded generation, but existing Redis queue entries with recommended_by = cold_start_explore_guarded / cold_start_adapt_guarded still pass _queued_meme_is_sendable and can be delivered until queues drain. This violates the PR/experiment rollback promise for a production cold-start experiment.

Please discard or reject guarded queued payloads when the guardrail flag is off, or otherwise make the rollback path stop treatment delivery immediately.

Verification I ran:

  • Targeted pytest set from the PR: 50 passed
  • uv run ruff check on changed source/test files
  • git diff --check
  • Diff-only secret scan
  • /codex review: found the rollback queue issue above

@ohld

ohld commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

STAFF ENGINEER REVIEW: APPROVED — Clean review on latest head ae2dbb0. Verified prior blockers are fixed: guarded queue rollback now discards existing guarded payloads when disabled, queued items are included in position-window math, guarded underfill routes to fallback instead of leaking unguarded cold-start candidates, and fallback support detection preserves dynamic test retrievers. Local targeted pytest passed (51 tests), ruff passed, git diff --check passed, diff secret scan found only public source URLs/test env text, CSO SQL/config pass found no injection or secret issue, and Codex review found no blockers.

@ohld
ohld merged commit f18ae38 into production Jul 16, 2026
3 checks passed
@ohld

ohld commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

✅ Approved + merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant