feat(quota): detect usage-window resets and notify on them (rebase of #2783) - #3592
Conversation
Carries #3447 by @hualiny (Antigravity weekly windows via retrieveUserQuotaSummary, plus Ollama Cloud quota from /api/usage), rebased rename-aware onto tests/providers/. The provider-level probe added there sent the stored account bearer to an operator-configured baseUrl with default redirect following, while fetchAntigravityUsageQuota in the same file already pins the identical request. Route it through providerOutboundPost against ANTIGRAVITY_ACCOUNT_QUOTA_BASE with the providerRedirectError check so a configured baseUrl stays a routing choice for model requests, not a second destination for Google's accounting. The fetchAvailableModels fallback is unchanged from dev. Co-authored-by: hualiny <82697947+hualiny@users.noreply.github.com>
…ogle's host Co-authored-by: hualiny <82697947+hualiny@users.noreply.github.com>
…vider quota test After the summary probe moved off globalThis.fetch, this test made a real request to Google (sandbox DNS failure masked it as a fallthrough). Inject the seam with a 404 so the fetchAvailableModels fallback is what the test exercises, as before. Co-authored-by: hualiny <82697947+hualiny@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
intake: hygiene-blocked
|
리뷰 · 우선순위 61 / 80설명 이 PR은 지금 이전 리뷰에서 막았던 구멍을 여섯 개로 나눠 고쳤다고 본문에 적혀 있습니다. 웹훅은 보안 경계가 중요합니다. 웹훅 URL은 Slack/Discord처럼 그 자체가 비밀인 경우가 많고, 라인 - 문제 경로 hygiene / missing_coauthor_credit - 메인테이너의 판단이 필요한 지점
너의 추천
이 댓글은 grok-bot이 작성했습니다 |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
![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: adb0dc51d0
ℹ️ 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".
| webhookUrl: `http://127.0.0.1:${server.port}/hook`, | ||
| allowPrivateNetwork: true, |
There was a problem hiding this comment.
This end-to-end test writes an http:// webhook into config.json, but the newly added quotaResetNotifySchema accepts only https: URLs regardless of allowPrivateNetwork. loadConfig() therefore drops the entire optional section, syncQuotaResetActivation() returns false, and the assertion on line 515 fails before any rollover is exercised. Use a TLS-capable test receiver or inject a validated HTTPS endpoint so the focused test can pass.
AGENTS.md reference: AGENTS.md:L360-L362
Useful? React with 👍 / 👎.
| * `webhookUrl` is here because for Slack and Discord the URL itself is the authorization: | ||
| * anyone holding it can post to the channel. It looks like configuration rather than a secret, | ||
| * which is exactly why it needs to be named explicitly — none of the other patterns match it, | ||
| * so `ocx config show` printed it and `config export` wrote it to disk in the clear. | ||
| */ | ||
| const SECRET_KEYS = /^(apiKey|key|accessToken|refreshToken|idToken|token|password|clientSecret|webhookUrl)$/i; |
There was a problem hiding this comment.
Adding webhookUrl to SECRET_KEYS protects config show, get, and set, but ocx config export still serializes readConfigDiagnostics().config directly at line 198 without calling redact. Consequently, ocx config export - prints the bearer-equivalent Slack/Discord URL to stdout and file exports retain it, contrary to both this comment and the new documentation. Omit or redact this field on the export path as well.
AGENTS.md reference: AGENTS.md:L353-L359
Useful? React with 👍 / 👎.
| // Synchronous: a lost claim means a duplicate notification after restart, and claims are | ||
| // rare (one per real reset), so the write cost is irrelevant. | ||
| // A failed write is reported rather than swallowed: the caller reads true as "safe to | ||
| // dispatch", and an unpersisted claim re-notifies on the next start. | ||
| return persistNow(); |
There was a problem hiding this comment.
When persistNow() fails because the config directory is temporarily unwritable or the disk is full, this function returns false but leaves the key in claims. The observer correctly refrains from dispatching that event, yet every later observation sees claims.has(key) and skips it; once writes recover, another debounced state write can even persist the never-delivered claim permanently. Delete the newly inserted claim when persistence fails so the reset can be retried.
Useful? React with 👍 / 👎.
| // Opt-in: the tick itself is a no-op unless config.quotaResetNotify is enabled with a | ||
| // sink, and the interval is unref'd, so a default install pays one dormant timer. | ||
| startQuotaResetPoller(); |
There was a problem hiding this comment.
Every server now calls startQuotaResetPoller() before checking configuration, so an installation with no quotaResetNotify section still owns a timer and wakes every 15 minutes to import the resolver and activation modules. This contradicts the new documented contract that an absent section starts no timer and makes the supposedly opt-in subsystem part of every server lifecycle. Resolve activation first and create the interval only when notification polling is enabled.
Useful? React with 👍 / 👎.
| enabled: isQuotaResetNotificationEnabled(), | ||
| events: listRecentQuotaResetEvents(limit), |
There was a problem hiding this comment.
The route and CLI explicitly accept a non-negative limit, so ?limit=0 reaches this call with zero, but listRecentQuotaResetEvents clamps its argument with Math.max(1, ...) and returns one event. Either permit zero in the store or reject it as invalid and describe the option as positive; currently callers asking for zero receive data despite their limit.
Useful? React with 👍 / 👎.
| // Forced: an unforced call would be served from the 5-minute cache and produce no new | ||
| // observation at all. | ||
| await fetchProviderQuotaReports(loadConfig(), true); |
There was a problem hiding this comment.
The generation check runs before fetchProviderQuotaReports, even though that awaited function commits its cache and invokes the quota observer internally. If shutdown or a poller restart calls stopQuotaResetPoller() while the upstream probe is awaiting, the old generation can still publish a snapshot and dispatch a webhook after the poller was stopped. Pass cancellation/generation ownership into the probe or otherwise prevent its internal commit when the generation changes during this await.
Useful? React with 👍 / 👎.
Rebase of the maintainer's #2783 onto the Antigravity/Ollama quota layer, with the three review blockers closed as six bounded fixes: https-only webhook URLs (B1), manual redirect handling on webhook sends (B2), MIN_INTERVAL_MS and MIN_POLL_SECONDS raised together to 600s (B3), configured cadence reaching the poller through a dynamic import that keeps the core-boundary guard green (B4), an in-flight fence on poll ticks (B5), and durable seen-claims (B6). Seven new test basenames registered in the layout map and fixture under tests/usage/. Docs: server.md floor, https requirement, redirect refusal.
adb0dc5 to
8087316
Compare
github-actions
Bot
removed
the
intake: hygiene-blocked
|
Maintainer admin merge (ruleset bypass recorded per MAINTAINERS.md): head 8087316; typecheck 0 and the PR's focused tests green locally; per maintainer instruction the final dev-tip CI run is the acceptance gate for this batch. wp4 of the 260905 open-work closeout. |
2 participants
Summary
Usage-window reset detection for account quotas: OpenCodex notices when a provider's 5-hour / weekly / monthly window rolls over, records the observation, and can notify through a configured webhook. Rebase of #2783 (maintainer-authored, 23 commits) onto the merged Antigravity/Ollama quota layer, with the three blockers from the last review closed as six bounded fixes:
https://(http://is rejected before any send).redirect: "manual"; a 3xx is ablocked-destinationresult, never a followed hop (no SSRF via redirect).MIN_INTERVAL_MSraised to 600 000 (the docstring's "above the 10-minute per-account TTL" is now true) andMIN_POLL_SECONDSraised to 600 with it so the resolver cannot accept a cadence the poller then silently overrides.syncQuotaResetPollerCadence()resolves it through a dynamicimport()from the lifecycle sotests/usage/quota-reset-core-boundary.test.ts's zero-static-import guard on the poller stays green (the plan's direct call would have pulled the config barrel into every server start).Seven new test basenames are registered in
scripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.jsonundertests/usage/.docs-siteserver.mdcorrected (600-second floor, https requirement, redirect refusal). Thesrc/providers/quota.tsmerge against layer 1 keeps layer 1's pinned-transport summary probe intact (its 121/0 guard still passes here). Supersedes #2783.Security-boundary note (MAINTAINERS.md): B1/B2 touch outbound webhook delivery — a new egress path carrying no credentials (payload is quota observations only). https-only plus manual redirect handling is the fail-closed shape; nothing is logged beyond the delivery result. Recorded here for explicit review.
Stack (wp4 quota stack; merge bottom-up):
devDepends on #3587. Review this PR's diff only. Unit:
devlog/_plan/260905_open_work_closeout/(040, 041, 042).Verification
bun run typecheck— exit 0.bun run privacy:scan— passed.unresolvedNew7 → 0 (15/2 → 17/0); B1expect(result.ok)onhttp://false→ok (10/0); B2 302http-error→blocked-destination; B3 60 000 → 600 000; B4quotaResetPollerIntervalForTestsmissing → 1 800 000 passes / 60 000 clamps to 600 000; B5 overlap 2 → 1 (15/0); B6 pruned claim true → false (16/0).ls-checked): quota-reset detector 28/0, account-key 3/0, core-boundary 14/0, notify-config 10/0, seen-store 16/0, observation 15/0,tests/lab/core-lab-boundary.test.ts17/0,tests/providers/provider-quota.test.ts121/0,tests/ci-workflows/repo-hygiene.test.ts14/0, layout guard 17/0.quota-reset-notifyandserver-background-lifecyclebind ports and are hosted-CI-only locally (EADDRINUSE, identical before and after the change).Checklist
devafter feat(quota): support Google Antigravity weekly quota and Ollama Cloud quota (carry of #3447) #3587 lands)