fix(ci): unblock deployment boot smoke and pace eval canary#437
Conversation
- Restore CI-safe RAG_QUERY_HASH_SECRET placeholder in deployment boot smoke so verify passes without a dedicated repo secret; real secret still used when set. - Pace golden retrieval in eval canary with inter-case and force-embedding delays plus longer provider backoff to avoid OpenAI 429/quota exhaustion on vector probes. - Document canary pacing in observability SLOs. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThis PR adds env-driven pacing for eval runs, pauses between retrieval cases, updates eval-canary workflow env vars, adjusts one UI navigation wait, adds tests, and documents the pacing settings. ChangesEval Pacing and Retry Configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant eval-retrieval.ts
participant pauseBetweenEvalCases
participant sleep
eval-retrieval.ts->>pauseBetweenEvalCases: { caseIndex, forceEmbedding }
pauseBetweenEvalCases->>sleep: await computed delay
🚥 Pre-merge checks | ✅ 4 | ❌ 7❌ Failed checks (1 warning, 6 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 22bf2ed61e
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
docs/observability-slos.md (1)
160-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the provider retry/backoff env vars too.
This section covers the case/embedding delay vars but omits
RAG_EVAL_PROVIDER_RETRY_ATTEMPTS,RAG_EVAL_PROVIDER_RETRY_INITIAL_MS, andRAG_EVAL_PROVIDER_RETRY_MAX_MSadded in the same workflow change, which are also part of the rate-limit mitigation described in the PR objectives.🤖 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 `@docs/observability-slos.md` around lines 160 - 163, Update the observability SLOs documentation to also mention the provider retry/backoff environment variables alongside the existing delay settings. In the section describing the nightly rate-limit mitigation, add references to RAG_EVAL_PROVIDER_RETRY_ATTEMPTS, RAG_EVAL_PROVIDER_RETRY_INITIAL_MS, and RAG_EVAL_PROVIDER_RETRY_MAX_MS so the docs match the workflow behavior; keep the wording consistent with the surrounding text in docs/observability-slos.md..github/workflows/eval-canary.yml (1)
41-47: 🧹 Nitpick | 🔵 TrivialVerify job timeout accommodates the new worst-case retry/backoff duration.
Baseline pacing (~2s/case × ~34 cases) plus a worst-case 6-attempt backoff (8s→16s→32s→64s→120s→120s ≈ 6 min per exhausted case) could push total runtime well past prior expectations. Confirm the job's
timeout-minutes(if set) leaves adequate headroom, otherwise this fix could trade a rate-limit failure for a timeout failure.#!/bin/bash rg -n "timeout-minutes" .github/workflows/eval-canary.yml🤖 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 @.github/workflows/eval-canary.yml around lines 41 - 47, The eval-canary workflow pacing changes can increase total runtime beyond the current job budget. Review the workflow’s timeout setting in eval-canary.yml, especially the job that uses RAG_EVAL_CASE_DELAY_MS and the RAG_EVAL_PROVIDER_RETRY_* settings, and raise timeout-minutes if needed so the worst-case retry/backoff plus baseline case delay still has enough headroom. Keep the existing retry/backoff configuration intact; only adjust the job timeout if it is too low.tests/eval-utils.test.ts (1)
102-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTight real-timer margins risk CI flakiness.
The test measures a real ~30ms sleep and asserts
<40mstotal — only a 10ms margin including test-runner overhead. Under CI load or Node timer jitter, this could intermittently flake. Consider usingvi.useFakeTimers()+vi.advanceTimersByTimeAsync()to make the assertion deterministic instead of relying on wall-clock timing.🤖 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 `@tests/eval-utils.test.ts` around lines 102 - 110, The pauseBetweenEvalCases timing test is relying on real wall-clock delays and has too little margin for CI jitter. Update the test to use vi.useFakeTimers() and drive the waits with vi.advanceTimersByTimeAsync() so the assertions around the pause behavior are deterministic. Keep the coverage in eval-utils.test.ts focused on pauseBetweenEvalCases and its delay env vars, but remove dependence on Date.now() for elapsed-time checks.
🤖 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 `@scripts/deployment-boot-smoke.mjs`:
- Around line 124-130: The RAG query hash secret fallback in the smoke test env
setup still uses nullish coalescing, so an empty string from GitHub Actions will
bypass the placeholder and leave the secret invalid. Update the
`RAG_QUERY_HASH_SECRET` assignment in `deployment-boot-smoke.mjs` to use a
falsy/trimmed check instead of `??`, matching the existing placeholder behavior
for the `SUPABASE_SERVICE_ROLE_KEY` and `OPENAI_API_KEY` entries.
In `@tests/eval-utils.test.ts`:
- Around line 103-104: The test that stubs RAG_EVAL_CASE_DELAY_MS and
RAG_EVAL_FORCE_EMBEDDING_DELAY_MS needs explicit cleanup so those env overrides
do not leak into other tests. Update the relevant test in eval-utils.test.ts to
restore stubbed env vars after it runs by calling vi.unstubAllEnvs() in the test
teardown path, or add equivalent beforeEach/afterEach cleanup around the
affected test block.
---
Nitpick comments:
In @.github/workflows/eval-canary.yml:
- Around line 41-47: The eval-canary workflow pacing changes can increase total
runtime beyond the current job budget. Review the workflow’s timeout setting in
eval-canary.yml, especially the job that uses RAG_EVAL_CASE_DELAY_MS and the
RAG_EVAL_PROVIDER_RETRY_* settings, and raise timeout-minutes if needed so the
worst-case retry/backoff plus baseline case delay still has enough headroom.
Keep the existing retry/backoff configuration intact; only adjust the job
timeout if it is too low.
In `@docs/observability-slos.md`:
- Around line 160-163: Update the observability SLOs documentation to also
mention the provider retry/backoff environment variables alongside the existing
delay settings. In the section describing the nightly rate-limit mitigation, add
references to RAG_EVAL_PROVIDER_RETRY_ATTEMPTS,
RAG_EVAL_PROVIDER_RETRY_INITIAL_MS, and RAG_EVAL_PROVIDER_RETRY_MAX_MS so the
docs match the workflow behavior; keep the wording consistent with the
surrounding text in docs/observability-slos.md.
In `@tests/eval-utils.test.ts`:
- Around line 102-110: The pauseBetweenEvalCases timing test is relying on real
wall-clock delays and has too little margin for CI jitter. Update the test to
use vi.useFakeTimers() and drive the waits with vi.advanceTimersByTimeAsync() so
the assertions around the pause behavior are deterministic. Keep the coverage in
eval-utils.test.ts focused on pauseBetweenEvalCases and its delay env vars, but
remove dependence on Date.now() for elapsed-time checks.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 795677bc-c6d7-47fb-b2d9-341d2cd7e06e
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/eval-canary.ymldocs/observability-slos.mdscripts/deployment-boot-smoke.mjsscripts/eval-retrieval.tsscripts/eval-utils.tstests/eval-utils.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f5343d7ec
ℹ️ 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".
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Restore RAG_QUERY_HASH_SECRET to deployment boot smoke required env so main/release CI validates the real repository secret instead of injecting a placeholder. Apply CLI --force-embedding pacing in eval-retrieval and fix Prettier formatting on the eval-utils import block. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Use vi.useFakeTimers for deterministic delay assertions and unstub env overrides in teardown so RAG_EVAL_* delay settings do not leak across tests. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
Head branch was pushed to by a user without write access
Summary
Fixes two standing CI/observability failures on
main:Deployment boot smoke —
verifyfails whenRAG_QUERY_HASH_SECRETis not configured as a GitHub secret (fix(ci): require RAG_QUERY_HASH_SECRET for deployment boot smoke #429 required it upfront). Boot smoke only needs to prove productionnext start+/api/local-project-id; restore a CI-safe placeholder (min 16 chars) while still using the real secret when provided.Eval Canary — nightly golden retrieval fails on the first
forceEmbeddingprobe (vector-ptsd) after ~22 text-fast-path cases due to OpenAI rate limits / quota exhaustion. Add configurable inter-case pacing and longer provider backoff in the canary workflow.Changes
scripts/deployment-boot-smoke.mjs— drop hash secret from required preflight env; pass placeholder to spawnednext start.github/workflows/ci.yml— document optional hash secret overridescripts/eval-utils.ts/scripts/eval-retrieval.ts—pauseBetweenEvalCases()withRAG_EVAL_CASE_DELAY_MS+RAG_EVAL_FORCE_EMBEDDING_DELAY_MS.github/workflows/eval-canary.yml— set pacing/backoff env for nightly runsdocs/observability-slos.md— note canary pacingVerification
npm run verify:cheapnpm run test -- tests/eval-utils.test.tsnpm run check:deployment-readiness(needs clean build without dev server; env preflight confirmed withoutRAG_QUERY_HASH_SECRET)workflow_dispatchafter merge (cannot trigger from agent)Clinical Governance Preflight
Not applicable — CI/observability plumbing only; no clinical output or retrieval logic changes.