Skip to content

fix(limit-warmup): repair typing for warmup sender#778

Merged
Soju06 merged 7 commits into
mainfrom
fix/pr-773-limit-warmup-typing
May 24, 2026
Merged

fix(limit-warmup): repair typing for warmup sender#778
Soju06 merged 7 commits into
mainfrom
fix/pr-773-limit-warmup-typing

Conversation

@Komzpa
Copy link
Copy Markdown
Collaborator

@Komzpa Komzpa commented May 22, 2026

Supersedes #773 because that contributor branch has maintainer edits disabled, while its current head fails ty.

What changed:

  • rebases the limit warm-up trigger onto current main
  • builds ResponsesRequest via model validation so max_output_tokens is accepted through the schema
  • narrows LimitWarmupService constructor dependencies to structural protocols so unit-test fakes type-check
  • keeps the request-log protocol aligned with the concrete repository signature
  • compares limit_warmup_min_available_percent against available quota, not used quota
  • defaults legacy frontend settings payloads for the new limit warm-up fields

Local verification:

  • uv run ruff check app/modules/limit_warmup/service.py tests/unit/test_limit_warmup.py
  • uv run ty check
  • uv run pytest tests/unit/test_limit_warmup.py -q
  • bun run typecheck
  • bun run test src/features/settings/schemas.test.ts --run

Fixes the CI failure from #773.

@Komzpa Komzpa force-pushed the fix/pr-773-limit-warmup-typing branch from 3fdef67 to f4151a6 Compare May 22, 2026 21:37
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3fdef673d5

ℹ️ 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".

Comment thread app/modules/limit_warmup/service.py Outdated
@Soju06 Soju06 added 🤖 codex: needs work [@codex review] raised an issue and removed 🤖 codex: needs work [@codex review] raised an issue labels May 22, 2026
@Komzpa Komzpa force-pushed the fix/pr-773-limit-warmup-typing branch from d90cdea to ada28b5 Compare May 22, 2026 22:15
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented May 22, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ada28b5f80

ℹ️ 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".

Comment thread app/core/usage/refresh_scheduler.py
@Soju06 Soju06 added the 🤖 codex: needs work [@codex review] raised an issue label May 22, 2026
@Soju06 Soju06 removed the 🤖 codex: needs work [@codex review] raised an issue label May 22, 2026
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented May 22, 2026

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

ℹ️ 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".

@Soju06 Soju06 added 🤖 codex: ok [@codex review] says no issues found. and removed 🤖 codex: ok [@codex review] says no issues found. labels May 22, 2026
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented May 24, 2026

Hermes review — blockers found

PR #778 is not merge-ready yet.

Blockers

  1. Warm-up opt-in reload is still stale within the same SQLAlchemy session.
    app/core/usage/refresh_scheduler.py:82 loads accounts, refresh_accounts(...) runs, then refresh_scheduler.py:94 calls accounts_repo.list_accounts() again before warm-up. But app/modules/accounts/repository.py:44-46 is a plain select(Account) using the same AsyncSession; already-loaded Account identities are returned from the session identity map without populate_existing, expire_all, or a fresh session. So if an operator disables limit_warmup_enabled while the refresh cycle is in flight, LimitWarmupService can still see the old value at app/modules/limit_warmup/service.py:229-233 and send a warm-up request.
    Suggested fix: force-refresh the account rows/flags before warm-up (populate_existing=True, session.expire_all(), or a fresh session/query dedicated to warm-up candidates) and add a regression test for the same-session stale identity-map case.

  2. OpenSpec change is structurally invalid.
    uv run openspec validate add-limit-warmup-trigger --strict fails because all three change spec files lack delta sections:

    • openspec/changes/add-limit-warmup-trigger/specs/database-migrations/spec.md
    • openspec/changes/add-limit-warmup-trigger/specs/frontend-architecture/spec.md
    • openspec/changes/add-limit-warmup-trigger/specs/usage-refresh-policy/spec.md
  3. Live GitHub mergeability is blocked.
    GitHub currently reports mergeable=false, mergeable_state=dirty, mergeable=CONFLICTING / mergeStateStatus=DIRTY.

Additional non-blocking cleanup worth considering

  • frontend/src/features/settings/components/routing-settings.tsx:143-147 adds a Switch with no accessible name. The visible text is not programmatically associated with the switch.
  • frontend/src/features/dashboard/components/account-card.tsx:125-139 exposes the per-account warm-up toggle as only “On”/“Off”, ambiguous for screen readers across multiple account cards.
  • Frontend warm-up model/prompt validation only checks non-empty (frontend/src/features/settings/schemas.ts:38-39), while backend rejects model >128 and prompt >512 (app/modules/settings/schemas.py:48-49).
  • Cooldown parsing uses Number.parseInt (routing-settings.tsx:38-47), so 60.5 silently becomes 60 before save.

Verification run

  • git diff --check origin/main...HEAD
  • uv run pytest tests/unit/test_limit_warmup.py tests/integration/test_settings_api.py tests/integration/test_accounts_api.py -q ✅ (15 passed)
  • uv run openspec validate --specs ✅ (21 passed)
  • uv run openspec validate add-limit-warmup-trigger --strict ❌ as above
  • Live checks are green, but mergeability is currently dirty/conflicting.

@Soju06 Soju06 added the hermes: needs-followup Hermes left a blocker/comment that needs follow-up observation label May 24, 2026
Soju06 added 2 commits May 24, 2026 18:42
…e-fix

# Conflicts:
#	app/modules/dashboard/service.py
- refresh account rows with populate_existing before warm-up evaluation
- cover same-session SQLAlchemy identity-map staleness
- make warm-up OpenSpec delta strict-valid
- align frontend warm-up validation with backend bounds
- improve warm-up toggle accessible names and cooldown parsing
@github-actions github-actions Bot added the db migration PR changes Alembic database migrations; maintainer must coordinate merge order label May 24, 2026
@Soju06
Copy link
Copy Markdown
Owner

Soju06 commented May 24, 2026

Addressed the Hermes blockers and related cleanup:

  • Merged current main into the PR branch to resolve the dirty/conflicting state.
  • Added AccountsRepository.list_accounts(refresh_existing=True) using SQLAlchemy populate_existing and switched the warm-up scheduler to use it immediately before candidate evaluation.
  • Added a same-session identity-map regression test so stale limit_warmup_enabled values cannot regress silently.
  • Converted add-limit-warmup-trigger OpenSpec files to strict-valid delta specs.
  • Tightened frontend warm-up validation to match backend max lengths, rejected decimal cooldown values without truncation, and added descriptive accessible names for warm-up controls.

Local verification:

  • uv run ruff check app/core/usage/refresh_scheduler.py app/modules/accounts/repository.py tests/integration/test_accounts_repository.py tests/unit/test_proxy_load_balancer_refresh.py
  • uv run ruff format --check app/core/usage/refresh_scheduler.py app/modules/accounts/repository.py tests/integration/test_accounts_repository.py tests/unit/test_proxy_load_balancer_refresh.py
  • uv run pytest tests/integration/test_accounts_repository.py tests/unit/test_limit_warmup.py tests/integration/test_settings_api.py tests/integration/test_accounts_api.py -q
  • uv run pytest -q (2750 passed, 7 skipped)
  • uv run ty check
  • uv run openspec validate add-limit-warmup-trigger --strict
  • uv run openspec validate --specs
  • npx --yes bun@1.3.14 install --frozen-lockfile
  • npx --yes bun@1.3.14 run test src/features/settings/schemas.test.ts src/features/settings/components/routing-settings.test.tsx src/features/dashboard/components/account-cards.test.tsx --run
  • npx --yes bun@1.3.14 run lint
  • npx --yes bun@1.3.14 run build

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

ℹ️ 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".

@Soju06 Soju06 added 🤖 codex: ok [@codex review] says no issues found. and removed hermes: needs-followup Hermes left a blocker/comment that needs follow-up observation labels May 24, 2026
@Soju06 Soju06 merged commit a96c487 into main May 24, 2026
23 checks passed
@Soju06 Soju06 deleted the fix/pr-773-limit-warmup-typing branch May 24, 2026 10:28
Soju06 added a commit that referenced this pull request May 25, 2026
## [1.19.0](v1.18.2...v1.19.0) (2026-05-25)


### Features

* **accounts:** add export action with audit and no-store safeguards ([#412](#412)) ([c03e310](c03e310))
* **accounts:** add operator-controlled account aliases ([#759](#759)) ([781e259](781e259))
* **api-ui:** add account cost distribution for API ([#734](#734)) ([d0a6737](d0a6737))
* **api-ui:** add account pool window usage remaining bar ([#785](#785)) ([8eee9e2](8eee9e2))
* **api:** add codex /model support for allowed models ([#607](#607)) ([15874aa](15874aa))
* **dashboard:** account burn projection card ([#752](#752)) ([c48a20a](c48a20a))
* **dashboard:** present hourly/weekly credits as raw remaining/total ([#612](#612)) ([b6b2f8b](b6b2f8b))
* **frontend:** add GitHub link to status bar ([#508](#508)) ([8577edc](8577edc))
* **release:** add PR-driven beta release channel ([#732](#732)) ([72b2962](72b2962))
* **request-log:** detail cost breakdown for each request ([#694](#694)) ([cb05d0e](cb05d0e))
* **ui:** multiple dashboard ui adjustments ([#776](#776)) ([c933b52](c933b52))


### Bug Fixes

* **accounts:** hide zero-capacity primary quota rows ([#770](#770)) ([8920274](8920274))
* **accounts:** own DB session in detached token-refresh task ([#774](#774)) ([3bdc9de](3bdc9de))
* **archive:** stream gzip writes asynchronously ([#725](#725)) ([67917ca](67917ca))
* **archive:** throttle backpressure warnings ([#718](#718)) ([feb0def](feb0def))
* **ci:** harden Codex label sync token writes ([#740](#740)) ([c40837d](c40837d))
* **ci:** restore main and enforce merge-head gates ([#715](#715)) ([b061ea5](b061ea5))
* **ci:** tolerate transient Codex label read failures ([#769](#769)) ([8fe58fe](8fe58fe))
* **codex desktop:** restore backend responses compatibility ([#756](#756)) ([fe591b7](fe591b7))
* Codex websocket pre-created keepalives ([#727](#727)) ([f52167d](f52167d))
* **codex_version:** fall back to npm registry when GitHub is rate-limited ([#744](#744)) ([7d790ba](7d790ba)), closes [#664](#664)
* **codex:** accept OpenAI-style backend responses requests ([#755](#755)) ([838386c](838386c))
* **limit-warmup:** refresh opt-in state before warm-up ([#778](#778)) ([a96c487](a96c487))
* **oauth:** isolate concurrent browser flows ([#753](#753)) ([409a83e](409a83e))
* **openai:** preserve json mode instruction messages ([#731](#731)) ([b48ed67](b48ed67)), closes [#730](#730)
* **proxy:** accept /backend-api/codex/v1/<rest> as alias for /backend-api/codex/<rest> ([#610](#610)) ([0aaaa80](0aaaa80))
* **proxy:** add HTTP bridge keepalive backstop and prewarm timeout ([#736](#736)) ([28c2430](28c2430))
* **proxy:** align Codex websocket error parsing ([#789](#789)) ([714315f](714315f))
* **proxy:** allow larger compressed responses bodies ([#772](#772)) ([67795a1](67795a1))
* **proxy:** bound HTTP bridge startup waits ([#723](#723)) ([48e7ccf](48e7ccf))
* **proxy:** fail over compact after invalidated token ([#777](#777)) ([afd23d2](afd23d2))
* **proxy:** fail over reset-prone upstream stalls ([#771](#771)) ([13dcf74](13dcf74))
* **proxy:** fail over websocket connect timeouts ([#726](#726)) ([a8b44f7](a8b44f7))
* **proxy:** inline external image URLs in HTTP bridge path ([#794](#794)) ([5ff6679](5ff6679))
* **proxy:** make Codex Spark quota gating plan-aware ([#751](#751)) ([a476ecd](a476ecd))
* **proxy:** mask codex chatgpt previous-response websocket errors ([#775](#775)) ([d39350f](d39350f))
* **proxy:** mask websocket prepare continuity errors ([#717](#717)) ([a0a290b](a0a290b))
* **proxy:** recover stale websocket previous response anchors ([#724](#724)) ([48f083e](48f083e))
* **proxy:** replay pre-visible websocket drops ([#729](#729)) ([4471b9a](4471b9a))
* **proxy:** report backend context window in v1 models ([#722](#722)) ([ebabd31](ebabd31))
* **proxy:** respect stream_idle_timeout_seconds in HTTP bridge keepalive backstop ([#793](#793)) ([ccdf99f](ccdf99f))
* **settings:** include all updatable fields in audit changed_fields ([#719](#719)) ([def95bb](def95bb))
* **status:** reconcile background account recovery after resets ([#754](#754)) ([4b9ada8](4b9ada8)), closes [#479](#479)
* **usage:** ignore stale usage after account reset ([#760](#760)) ([d739ebf](d739ebf))


### Performance Improvements

* **dashboard:** memoize per-account depletion EWMA state ([#749](#749)) ([2abe7a9](2abe7a9))


### Documentation

* add Lotfree618 as a contributor for code, test, and 2 more ([#739](#739)) ([66764f4](66764f4))
* add usage reset troubleshooting FAQ ([#710](#710)) ([b6c35f6](b6c35f6))
* backfill missing contributors ([#741](#741)) ([505a208](505a208))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖 codex: ok [@codex review] says no issues found. db migration PR changes Alembic database migrations; maintainer must coordinate merge order

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants