Add external event hooks for quota and provider state changes#2001
Conversation
|
Codex review: needs changes before merge. Reviewed July 17, 2026, 1:36 AM ET / 05:36 UTC. Summary Reproducibility: yes. for the patch defects: the current-head source establishes deterministic paths for a synthetic quota sample, a repeated outage after recovery, and selected-account refresh failures. Live account access is unnecessary to demonstrate these control-flow errors. Review metrics: 3 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Fix the three event-delivery gaps without expanding the API, preserve the opt-in direct-exec safeguards, validate both fresh and existing shared configurations, and then obtain explicit maintainer acceptance of the automation contract. Do we have a high-confidence way to reproduce the issue? Yes for the patch defects: the current-head source establishes deterministic paths for a synthetic quota sample, a repeated outage after recovery, and selected-account refresh failures. Live account access is unnecessary to demonstrate these control-flow errors. Is this the best way to solve the issue? No, not yet: the generic direct-exec hook architecture is plausible, but three event paths violate its promised transition contract and the permanent product boundary remains unapproved. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b7920f1aead5. Label changesLabel justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (40 earlier review cycles; latest 8 shown)
|
![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: b1a5c65eaa
ℹ️ 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".
clawsweeper
Bot
added
rating: 🧂 unranked krab
Run user-configured external commands when quota/provider events occur, without CodexBar owning any switching logic. Hooks are opt-in and disabled by default. Core (CodexBarCore/Hooks): HookEvent (env vars + stdin JSON payload), HookRule/HooksConfig (matching, provider scoping, quota_low threshold gate, absolute-path requirement), HookRunner (delegates process work to the existing SubprocessRunner: shell-free exec, env injection, stdin, timeout, path validation, redacted logging), and an in-memory HookRateLimiter as a storm backstop. Adds an optional top-level `hooks` section to CodexBarConfig. App: emitHook dispatches fire-and-forget from the existing detection choke points so nothing blocks menu refresh: quota_low, quota_reached, quota_reset, provider_unavailable, provider_recovered, refresh_failed. Account labels respect hidePersonalInfo. Adds a Hooks preferences pane (enable toggle, trust warning, per-rule editing) wired through SettingsStore config mutators. CLI: `codexbar hooks list | enable | disable | test <event> --provider <p>`, sharing HookRunner with the app. Tests: unit coverage for matching, threshold gating, payload encoding, rate limiting, and the runner; localization catalogs updated for the new UI keys. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b1a5c65 to
2882485
Compare
…logs Address automated review feedback on steipete#2001: - Forward only a narrow allowlist of environment variables to hook commands (PATH/HOME/USER/SHELL/locale/TMPDIR) plus the event's CODEXBAR_* values, so a provider API key living in CodexBar's environment can never leak to a hook. - Rate-limit only the storm-prone events (provider_unavailable, refresh_failed). Quota events dedupe upstream; throttling them dropped a lower remaining-quota warning that crossed within the 10-minute window (Codex P2). - Redact hook failure logs: log the event and a coarse reason (exit code, timeout, not-found) instead of the subprocess stderr, which could echo the payload. Adds tests for the rate-limit classification and the environment allowlist. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@clawsweeper re-review Pushed d7130ae addressing the review:
|
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: sufficient
Address the remaining P2 review points on steipete#2001: - Quota hooks no longer depend on notification preferences. Quota-warning and session-depletion detection now runs when either notifications OR a matching hook rule is enabled, and only the notification post is gated on the notification settings. Gating is keyed on hasQuotaHookRule(event:provider:), so behavior is byte-identical for users without a configured quota hook. - Editing hook rules no longer triggers a provider refresh. Hook config writes go through a dedicated path that persists and bumps a separate hooksRevision for the settings pane, without bumping configRevision (which drives the background refresh introduced by steipete#1996). Bundles the per-refresh quota-warning constants into QuotaWarningTransitionContext to keep the transition helper within the parameter-count limit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@clawsweeper re-review Pushed fa11d2d addressing the remaining P2 items:
Quota/notification transition suites and hooks tests are green. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Address ClawSweeper's remaining P2 blocker (UsageStore+QuotaWarnings.swift): quota_low hooks were emitted only on notification-threshold crossings, so a rule at threshold 0.90 could never fire under the default 50/20 remaining notification thresholds. quota_low hooks now run on a dedicated path, fully separate from the notification machinery: - Per-lane usage is tracked and each matching rule fires when usage crosses its own threshold upward. A rule without a threshold falls back to the provider's notification thresholds so a plain low-quota hook still fires at the app's warning points. - The notification path reverts to its original notification-only form; hooks no longer piggyback on it. - The crossing selection is extracted to QuotaLowHookThreshold.crossedRules in Core and unit-tested (own-threshold crossing, thresholdless fallback, and selecting only the newly-crossed rule among several). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@clawsweeper re-review Pushed 247bcb7 fixing the remaining P2 (quota_low threshold contract):
All hooks + quota/notification transition suites green; make lint clean. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
clawsweeper
Bot
added
status: ⏳ waiting on author
Address ClawSweeper's remaining P2 (UsageStore+Hooks.swift): quotaLowHookUsage was keyed by provider/window/windowID only, so when several accounts share one provider, one account's usage observations could suppress, clear, or re-arm another account's quota_low threshold crossing. Add a stable account discriminator (the unredacted account email, used only as an in-memory key, never logged or forwarded) to a dedicated QuotaLowHookUsageKey. Each account/lane now tracks its crossings independently. Adds a regression test covering two accounts on one provider plus lane independence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@clawsweeper re-review Pushed b14629b addressing the remaining P2 (account-scoped quota_low baselines):
make lint clean; hooks + session-quota + Codex baseline suites green. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…keys Refine the account discriminator for quota_low hook state (ClawSweeper P2): keying on the account email alone let email-less or same-email accounts share a baseline, so one account could suppress/clear/re-arm another's threshold crossing. quotaHookAccountKey now composes the full provider identity (email, organization, login method). Accounts without an email, or sharing one field, key independently whenever any other identity field differs; only truly identical identities (the same account) share a baseline. The pure composition is factored out and covered by tests for email-less, same-email, and identical-identity cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t-hooks # Conflicts: # Sources/CodexBar/UsageStore+QuotaWarnings.swift
|
@clawsweeper re-review Merged current main (which landed #2130's account-scoping). quota_low hook crossing history now reuses main's stable account discriminator via QuotaWarningStateKey (dropped the parallel composite key/helper). Quota hooks remain decoupled from notification preferences. make lint clean; hooks + quota/session/account-scoping suites green. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff6fb83dd7
ℹ️ 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".
| self.pruneQuotaLowHookUsage( | ||
| provider: provider, | ||
| accountDiscriminator: accountContext.discriminator, | ||
| keepingExtraWindowIDs: Set(extraWindows.map(\.id))) |
There was a problem hiding this comment.
When a Claude snapshot temporarily omits extraRateWindows, extraWindows is empty and this prune call removes every tracked extra-window baseline. The notification path above explicitly treats an empty extras payload as non-authoritative, but the hook path loses the baseline; if the same Claude model window reappears already past a quota_low threshold, dispatchQuotaLowHooks sees it as the first sample and never emits the crossing. Only prune when an authoritative extras list is present.
Useful? React with 👍 / 👎.
| let revision = self.settings.configRevision | ||
| guard self.quotaLowHookConfigRevision != revision else { return } | ||
| self.quotaLowHookUsage.removeAll() | ||
| self.quotaLowHookConfigRevision = revision |
There was a problem hiding this comment.
Because settings.configRevision increments for provider/order/token-account changes as well as hook edits, this clears quota_low baselines after unrelated config changes. If usage was 40%, the user changes a provider setting, and the next refresh is 95%, the previous sample is discarded here and no quota_low hook runs despite crossing the rule threshold. Track a hook-specific revision or compare the hooks config instead of the global config revision.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🐚 platinum hermit
…t-hooks # Conflicts: # Sources/CodexBar/Resources/de.lproj/Localizable.strings # Sources/CodexBar/Resources/es.lproj/Localizable.strings # Sources/CodexBar/Resources/fr.lproj/Localizable.strings # Sources/CodexBar/Resources/ja.lproj/Localizable.strings # Sources/CodexBar/Resources/ko.lproj/Localizable.strings # Sources/CodexBar/Resources/nl.lproj/Localizable.strings # Sources/CodexBar/Resources/pt-BR.lproj/Localizable.strings # Sources/CodexBar/Resources/ru.lproj/Localizable.strings # Sources/CodexBar/Resources/sv.lproj/Localizable.strings # Sources/CodexBar/Resources/uk.lproj/Localizable.strings # Sources/CodexBar/Resources/vi.lproj/Localizable.strings # Sources/CodexBar/Resources/zh-Hans.lproj/Localizable.strings # Sources/CodexBar/Resources/zh-Hant.lproj/Localizable.strings
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14fc1f32e0
ℹ️ 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".
| let current = rateWindow.usedPercent / 100 | ||
| let previous = self.quotaLowHookUsage[key] | ||
| self.quotaLowHookUsage[key] = current |
There was a problem hiding this comment.
When Claude web reports no live session, the primary window is a synthetic 0%-used placeholder (isSyntheticPlaceholder), which the notification path already skips. This hook path records that placeholder as the previous usage, so if the next real session sample appears already above a quota_low threshold it looks like a 0%→high crossing and runs the hook even though no real quota lane crossed the threshold.
Useful? React with 👍 / 👎.
| case .providerUnavailable, .refreshFailed: | ||
| true |
There was a problem hiding this comment.
When a provider goes outage → normal → outage again within ten minutes, emitProviderStatusHooks correctly creates a second provider_unavailable transition, but marking this event as rate-limited causes the second transition to share the same limiter key and be dropped. Since provider_unavailable is already transition-driven, this hides real repeated outages after a recovery rather than just coalescing refresh storms.
Useful? React with 👍 / 👎.
| self.emitHook( | ||
| .refreshFailed, | ||
| provider: provider, | ||
| status: Self.refreshFailureHookStatus(error)) |
There was a problem hiding this comment.
This refresh_failed emission only runs in the unscoped provider failure path. Selected token-account and Codex visible-account failures are handled in UsageStore+TokenAccounts.applySelectedOutcome / applySelectedCodexVisibleAccountOutcome, which update errors but never call emitHook, so users relying on configured accounts will not receive refresh_failed hooks when the selected account refresh fails.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
…t-hooks # Conflicts: # Sources/CodexBar/SettingsStore+ConfigPersistence.swift
|
Merged as Verification:
Manual local hook test used an isolated config under {"event":"quota_reached","provider":"codex","timestamp":"2026-07-17T01:11:08Z","usagePercent":1,"window":"session"}The missing-executable failure case also returned immediately without wedging the command or refresh path. |
2 participants
Closes #2000.
What
Optional, opt-in hooks that run a user-configured command when a quota or provider event fires. CodexBar detects the event and runs the command; it does not switch providers, restart apps, or store keys.
Events:
quota_low(with a per-rule usage threshold),quota_reached,quota_reset,provider_unavailable,provider_recovered,refresh_failed.How
config.jsonunder a top-levelhooksblock, so the app and CLI share it. Off by default.SubprocessRunner: direct exec (no shell), timeout with SIGTERM then SIGKILL, absolute-path validation. Dispatch is fire-and-forget so it never blocks the menu refresh.codexbar hooks list | enable | disable | test.Security boundary
CODEXBAR_*values. CodexBar's own environment is not inherited, so a provider key living in env cannot reach a hook.provider_unavailable,refresh_failed) are rate limited. Quota events dedupe upstream, so they are not throttled.Payload
Env vars (
CODEXBAR_EVENT,CODEXBAR_PROVIDER,CODEXBAR_USAGE_PERCENT,CODEXBAR_RESET_AT, ...) plus a JSON payload on stdin.Proof
Real CLI run against an isolated config (
CODEXBAR_CONFIG), hook script pointed at a logger:The env-allowlist test also confirms a secret in the base environment (
ANTHROPIC_API_KEY) does not reach the child, whilePATHdoes.Tests
Unit coverage for rule matching, threshold gating, payload encoding, rate-limit classification, the environment allowlist, and the runner (exec + missing executable). Localization catalogs updated for the new UI strings.
swift build,make lint, and the hooks and localization suites are green.Notes
mainso PR Fix visual settings refresh #1996 (visual settings refresh) is included; nothing from it is reverted.updateConfig(affectsBackgroundWork: false), so the config persists and the pane re-renders without scheduling a background refresh (reuses main's refresh-isolation mechanism).quota_lowhooks are driven by each rule's own usage threshold (crossed upward), independently of the notification thresholds and preferences. A rule without a threshold falls back to the provider's notification thresholds. Crossing selection lives inQuotaLowHookThreshold.crossedRules(Core) and is unit-tested.Non-goals
No provider switching, no editing other tools' config, no app restarts, no storing keys, no vendor fallback chains, no running commands from remote config.
🤖 Generated with Claude Code