fix(qa): #842 quota circuit-breaker + stale-evidence hygiene (6 remaining gaps) - #1042
Merged
Conversation
…s QUOTA-ABORT, never a junk RRI/score (#842) PR #844 landed the core (sequential personas, quota_tripped, QUOTA_ABORT sentinel, the rollup ABORTED status). This closes the SIX remaining gaps so a mid-sweep account session-limit 429 can never masquerade as a product measurement, and a quota'd run can never republish a previous run's evidence. Fix A (qa/vm/sweep_v2.sh) — sweep-start cleanup now also `rm -f "$RES/RRI.json"`, so a sweep that quota-aborts before writing a fresh rollup can't leave the PREVIOUS run's RRI.json in place (the rc3 stale-RRI bug). Fix B (qa/vm/sweep_v2.sh) — the canary QUOTA_ABORT path now writes the {"status":"ABORTED",…} RRI.json (it previously touched DONE + exited, leaving a stale RRI behind). Extracted a shared write_aborted_rri() helper and reused it at BOTH canary-abort sites and the post-batch QUOTA_ABORT short-circuit so the ABORTED JSON shape is identical at every quota exit. Fix C (qa/vm/sweep_v2.sh) — wipe this run's stale duo artifacts (duo-{tolkien,angrydm,latency}.json + qa/transcripts/vm2-duo.{tolkien,angrydm}.json) BEFORE the duo runs, so the `[ -f ] && cp` only copies CURRENT-run output (rc3 republished rc2's byte-identical "story 4.0/mech 3.0" verbatim). Fix D (qa/ui_playtest_app.sh) — the backend player-ready poll loop now greps backend.log for a 429/session-limit INSIDE the loop (after the kill -0 check), drops a QUOTA_EXHAUSTED sentinel + breaks early; the readiness-failure path checks the sentinel and emits a `quota_exhausted` bucket instead of mis-bucketing the corpse as backend_not_ready/no_actor. Added quota_exhausted to APP_FAILURE_BUCKETS_JSON. Fix E (qa/run_duo.sh + qa/vm/sweep_v2.sh) — run_duo detects a session-limit 429 in the DM cold-open ($COMBINED / $RUN.dm.err), logs "[duo] QUOTA ABORT", skips scoring, and exits rc=2 (before the empty-reply rc=1 abort). The sweep greps duo.log for that marker before copying duo scores → on a hit it writes QUOTA_ABORT + the ABORTED RRI and skips the rollup. Fix F (qa/score.sh + qa/run_duo.sh) — score.sh adds a 429 fast-fail arm (no 3 retries): on api_error_status==429 (or a session-limit body) it writes the sentinel {"quota_exhausted":true,"api_error_status":429} to $OUT and exits rc=2. run_duo's post-scoring check treats any lens carrying that sentinel as a quota abort (→ "[duo] QUOTA ABORT" + exit rc=2), never a valid scorecard, before the behavioral gate runs. Tests (qa/test_release_gate_static.py) — 6 static grep-the-shell-source contracts mirroring test_release_gate_static.py style (no live runs): RRI.json in the cleanup rm; canary-abort writes an ABORTED RRI; duo-artifact rm precedes the duo call; quota_exhausted in APP_FAILURE_BUCKETS_JSON; score.sh 429 fast-fail arm (sentinel + rc=2); run_duo checks for QUOTA ABORT before scoring. All 20 tests in the file pass (14 pre-existing + 6 new). bash -n clean on all four touched scripts.
📝 WalkthroughWalkthroughQuota/session-limit (HTTP 429) exhaustion is now treated as an infrastructure abort rather than a scoring or product miss across the QA pipeline. ChangesQuota/429 Circuit-Breaker Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![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
🤖 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 `@qa/test_release_gate_static.py`:
- Around line 229-231: The loop variable `l` (lowercase L) in the generator
expression within the next() function call is ambiguous and flagged by Ruff
(E741). Replace all occurrences of the loop variable `l` with a more descriptive
name like `line` in both the iterator part and the condition that checks
`startswith("APP_FAILURE_BUCKETS_JSON=")` to improve readability and satisfy
static checks.
In `@qa/ui_playtest_app.sh`:
- Around line 969-988: The quota-exhaustion detection logic using grep to check
for "session limit|HTTP 429|hit your (session|usage) limit" in backend.log must
be repositioned to execute before the backend process liveness check (the kill
-0 command). Currently, if the backend process terminates due to a quota error,
the kill -0 check exits the loop before the quota detection runs, causing
incorrect bucketing as backend_not_ready instead of quota_exhausted. Move the
entire quota-check block (the grep condition with its log, touch
QUOTA_EXHAUSTED, and break statements) to occur earlier in the loop, before any
process termination checks that might prematurely exit the loop.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ee74fb3-e967-427e-b2fc-a244ac9bef97
📒 Files selected for processing (5)
qa/run_duo.shqa/score.shqa/test_release_gate_static.pyqa/ui_playtest_app.shqa/vm/sweep_v2.sh
Comment on lines
+229
to
+231
| buckets_line = next( | ||
| l for l in source.splitlines() if l.startswith("APP_FAILURE_BUCKETS_JSON=") | ||
| ) |
There was a problem hiding this comment.
Rename ambiguous loop variable at Line 230.
l is flagged by Ruff (E741) and is hard to read in this context. Rename it to line (or similar) to keep static checks green and improve clarity.
Suggested patch
- buckets_line = next(
- l for l in source.splitlines() if l.startswith("APP_FAILURE_BUCKETS_JSON=")
- )
+ buckets_line = next(
+ line for line in source.splitlines() if line.startswith("APP_FAILURE_BUCKETS_JSON=")
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| buckets_line = next( | |
| l for l in source.splitlines() if l.startswith("APP_FAILURE_BUCKETS_JSON=") | |
| ) | |
| buckets_line = next( | |
| line for line in source.splitlines() if line.startswith("APP_FAILURE_BUCKETS_JSON=") | |
| ) |
🧰 Tools
🪛 Ruff (0.15.17)
[error] 230-230: Ambiguous variable name: l
(E741)
🤖 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 `@qa/test_release_gate_static.py` around lines 229 - 231, The loop variable `l`
(lowercase L) in the generator expression within the next() function call is
ambiguous and flagged by Ruff (E741). Replace all occurrences of the loop
variable `l` with a more descriptive name like `line` in both the iterator part
and the condition that checks `startswith("APP_FAILURE_BUCKETS_JSON=")` to
improve readability and satisfy static checks.
Source: Linters/SAST tools
Comment on lines
+969
to
+988
| # #842 Fix D (quota circuit-breaker): a DM cold-open that 429s on the account session limit writes | ||
| # "session limit" / "HTTP 429" into backend.log. WITHOUT this the poll just runs out its ~10-min cap | ||
| # and mis-buckets the corpse as no_actor/no_provider (the rc3 misattribution). Detect it INSIDE the | ||
| # loop, drop a QUOTA_EXHAUSTED sentinel, and break early so we abort honestly instead of waiting + | ||
| # mis-bucketing. This is an INFRA abort, NOT a product-readiness miss. | ||
| if grep -qiE "session limit|HTTP 429|hit your (session|usage) limit" "$RUNDIR/backend.log" 2>/dev/null; then | ||
| log "[B] QUOTA EXHAUSTED — backend DM cold-open hit the account session limit (HTTP 429); see $RUNDIR/backend.log. Aborting the ready-wait (INFRA abort, not a product miss)." | ||
| touch "$RUNDIR/QUOTA_EXHAUSTED" | ||
| break | ||
| fi | ||
| sleep 3 | ||
| done | ||
| if [ "$ready" != "1" ]; then | ||
| # #842 Fix D: a quota 429 short-circuit takes precedence over the generic backend_not_ready | ||
| # classification — bucket it as quota_exhausted so the rollup attributes an INFRA abort, never a | ||
| # no_actor/no_provider product failure (the rc3 mis-bucketing the quota circuit-breaker exists to kill). | ||
| if [ -f "$RUNDIR/QUOTA_EXHAUSTED" ]; then | ||
| log "[B] backend never became player-ready — QUOTA_EXHAUSTED (account session limit) — see $RUNDIR/backend.log" | ||
| PART_B_RESULT="quota_exhausted"; set_bucket_pair B "$(bucket_pair quota_exhausted 'DM cold-open hit the account session limit (HTTP 429) — INFRA abort, not a product readiness miss')"; return 1 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 'kill -0 "\$B_BACKEND"|QUOTA_EXHAUSTED|session limit\|HTTP 429' qa/ui_playtest_app.shRepository: electricsheephq/WorldOS
Length of output: 2039
Move the quota-log check before the backend liveness check.
At line 968, the kill -0 check exits the loop if the backend process dies, bypassing the quota detection logic at lines 974–978. If a backend exits due to a 429 error, the quota check never runs, causing the failure to be mis-bucketed as backend_not_ready instead of quota_exhausted.
Suggested fix
- kill -0 "$B_BACKEND" 2>/dev/null || { log "[B] backend exited early — see $RUNDIR/backend.log"; break; }
# `#842` Fix D (quota circuit-breaker): a DM cold-open that 429s on the account session limit writes
# "session limit" / "HTTP 429" into backend.log. WITHOUT this the poll just runs out its ~10-min cap
# and mis-buckets the corpse as no_actor/no_provider (the rc3 misattribution). Detect it INSIDE the
# loop, drop a QUOTA_EXHAUSTED sentinel, and break early so we abort honestly instead of waiting +
# mis-bucketing. This is an INFRA abort, NOT a product-readiness miss.
if grep -qiE "session limit|HTTP 429|hit your (session|usage) limit" "$RUNDIR/backend.log" 2>/dev/null; then
log "[B] QUOTA EXHAUSTED — backend DM cold-open hit the account session limit (HTTP 429); see $RUNDIR/backend.log. Aborting the ready-wait (INFRA abort, not a product miss)."
touch "$RUNDIR/QUOTA_EXHAUSTED"
break
fi
+ kill -0 "$B_BACKEND" 2>/dev/null || { log "[B] backend exited early — see $RUNDIR/backend.log"; break; }🤖 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 `@qa/ui_playtest_app.sh` around lines 969 - 988, The quota-exhaustion detection
logic using grep to check for "session limit|HTTP 429|hit your (session|usage)
limit" in backend.log must be repositioned to execute before the backend process
liveness check (the kill -0 command). Currently, if the backend process
terminates due to a quota error, the kill -0 check exits the loop before the
quota detection runs, causing incorrect bucketing as backend_not_ready instead
of quota_exhausted. Move the entire quota-check block (the grep condition with
its log, touch QUOTA_EXHAUSTED, and break statements) to occur earlier in the
loop, before any process termination checks that might prematurely exit the
loop.
…tail (was masking as RELEASE_READY)
Adversarial review caught the load-bearing defect: the ABORTED RRI used {status:ABORTED, detail:...}
but evidence_audit.py keys on aborted:true + abort_detail — so a quota-aborted sweep read as
RELEASE_READY, the exact masking #842 prevents. Mirror release_readiness.py's shape. Lock it with a
static key-check + a functional test that runs the shape through evidence_audit.py (21 pass).
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 `@qa/test_release_gate_static.py`:
- Around line 219-229: The subprocess.run call in the test function is using the
hardcoded string "python3" as the interpreter in the command list, which relies
on PATH resolution and triggers a security linting concern. Replace the
hardcoded "python3" string with sys.executable to use the active Python
interpreter instead, which is safer and more deterministic for the test
environment. Make sure to import sys at the top of the file if it is not already
imported.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bf804c12-3f25-4d5c-be47-29cbdad74696
📒 Files selected for processing (2)
qa/test_release_gate_static.pyqa/vm/sweep_v2.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- qa/vm/sweep_v2.sh
Comment on lines
+219
to
+229
| import json, subprocess, tempfile, os | ||
| rri = {"status": "ABORTED", "aborted": True, "abort_reason": "quota_session_limit", | ||
| "abort_detail": "newbie — quota resets ~3h", "build_sha": "deadbeef", | ||
| "release_ready": False, "note": "infra abort, not a product RRI"} | ||
| fd, path = tempfile.mkstemp(suffix=".json") | ||
| try: | ||
| with os.fdopen(fd, "w") as f: | ||
| json.dump(rri, f) | ||
| out = subprocess.run( | ||
| ["python3", str(ROOT / "qa" / "evidence_audit.py"), "--rri", path], | ||
| capture_output=True, text=True, timeout=30) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there are no remaining partial-path python invocations in subprocess calls in this test file.
rg -nP 'subprocess\.run\(\s*\[\s*"python3"' qa/test_release_gate_static.pyRepository: electricsheephq/WorldOS
Length of output: 49
🏁 Script executed:
sed -n '219,229p' qa/test_release_gate_static.pyRepository: electricsheephq/WorldOS
Length of output: 690
Use sys.executable for the subprocess interpreter (line 228).
Calling "python3" by partial path relies on PATH and is exactly what Ruff S607 flags. Using the active interpreter is safer and more deterministic for this test.
Suggested patch
- import json, subprocess, tempfile, os
+ import json, subprocess, tempfile, os, sys
@@
- ["python3", str(ROOT / "qa" / "evidence_audit.py"), "--rri", path],
+ [sys.executable, str(ROOT / "qa" / "evidence_audit.py"), "--rri", path],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import json, subprocess, tempfile, os | |
| rri = {"status": "ABORTED", "aborted": True, "abort_reason": "quota_session_limit", | |
| "abort_detail": "newbie — quota resets ~3h", "build_sha": "deadbeef", | |
| "release_ready": False, "note": "infra abort, not a product RRI"} | |
| fd, path = tempfile.mkstemp(suffix=".json") | |
| try: | |
| with os.fdopen(fd, "w") as f: | |
| json.dump(rri, f) | |
| out = subprocess.run( | |
| ["python3", str(ROOT / "qa" / "evidence_audit.py"), "--rri", path], | |
| capture_output=True, text=True, timeout=30) | |
| import json, subprocess, tempfile, os, sys | |
| rri = {"status": "ABORTED", "aborted": True, "abort_reason": "quota_session_limit", | |
| "abort_detail": "newbie — quota resets ~3h", "build_sha": "deadbeef", | |
| "release_ready": False, "note": "infra abort, not a product RRI"} | |
| fd, path = tempfile.mkstemp(suffix=".json") | |
| try: | |
| with os.fdopen(fd, "w") as f: | |
| json.dump(rri, f) | |
| out = subprocess.run( | |
| [sys.executable, str(ROOT / "qa" / "evidence_audit.py"), "--rri", path], | |
| capture_output=True, text=True, timeout=30) |
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 226-228: Command coming from incoming request
Context: subprocess.run(
["python3", str(ROOT / "qa" / "evidence_audit.py"), "--rri", path],
capture_output=True, text=True, timeout=30)
Note: [CWE-20].
(subprocess-from-request)
🪛 Ruff (0.15.17)
[error] 227-227: subprocess call: check for execution of untrusted input
(S603)
[error] 228-228: Starting a process with a partial executable path
(S607)
🤖 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 `@qa/test_release_gate_static.py` around lines 219 - 229, The subprocess.run
call in the test function is using the hardcoded string "python3" as the
interpreter in the command list, which relies on PATH resolution and triggers a
security linting concern. Replace the hardcoded "python3" string with
sys.executable to use the active Python interpreter instead, which is safer and
more deterministic for the test environment. Make sure to import sys at the top
of the file if it is not already imported.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the 6 gaps #844 left open. Critical: stale-evidence — a quota-aborted sweep was reusing the prior run's RRI/duo scores byte-identical (the rc3 'story 4.0/mech 3.0 were rc2's files' bug). Fixes: (A) clear stale RRI.json at sweep start; (B) canary-abort writes an ABORTED RRI (shared
write_aborted_rri()); (C) wipe stale duo artifacts before the duo; (D) ui_playtest early-429-exit →quota_exhaustedbucket; (E) run_duo QUOTA ABORT (rc=2) + sweep skip; (F) score.sh 429 fast-fail sentinel. 6 new static contract tests (20 pass),bash -nclean.Summary by CodeRabbit
Tests
Bug Fixes
quota_exhaustedfailure bucket during Part B readiness, taking precedence over generic backend-not-ready results.Chores