Add Venice web quota strategy - #3474
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
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. |
clawsweeper
Bot
added
P2
|
Codex review: needs real behavior proof before merge. Reviewed September 7, 2026, 4:05 PM ET / 20:05 UTC. ClawSweeper reviewWhat this changesAdds an explicit Venice Web source that reads monthly subscription credits using browser sessions or a stored manual cookie, with source selection and monthly-credit labels. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 11 items remain This remains a useful, distinct contribution, but the latest settings integration introduces an app-wide crash and ignores disabled cookie access. Earlier review fixes are present; the new live manual-cookie evidence establishes progress but does not cover the remaining integration gaps. Priority: P1 Review scores
Verification
How this fits togetherCodexBar converts provider credentials and settings into usage snapshots for its menu-bar app and CLI. Venice’s new Web strategy retrieves subscription quota alongside the existing API balance strategy. flowchart TD
A[Provider settings and selected account] --> B{Usage source}
B -->|Auto or API account| C[Existing API balance script]
B -->|Web| D[Manual cookie or browser session]
D --> E[Venice session endpoint]
E --> F[Monthly quota snapshot]
C --> G[App and CLI presentation]
F --> G
Before merge
Findings
Agent review detailsSecurityNeeds attention: The new cookie settings path bypasses an explicit access opt-out; no dependency, workflow, or package-resolution changes were introduced. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Complete Venice’s app settings contribution and enforce cookie policy before credential access, while preserving Auto/API behavior and demonstrating fresh-install and upgrade compatibility. Do we have a high-confidence way to reproduce the issue? Yes, from source: building the app registry settings snapshot reaches a nil Venice contribution, and Web plus cookieSource Off reaches browser import. These introduced failures were not executed during the read-only review. Is this the best way to solve the issue? The explicit Web strategy is a reasonable fit, but the current implementation is incomplete: app settings must supply the new typed section, and Off must stop credential access. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against c526fe71721e. LabelsLabel justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
There was a problem hiding this comment.
🟡 Changes recommended
The intended “Monthly credits” label helper is not currently wired into UI label selection, and several new user-facing error messages hard-code “Chrome” despite configurable cookie import sources.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new explicit-web-only Venice quota fetch path that reads the signed-in venice.ai session (via imported browser cookies) and derives a monthly refill window from the session JWT, without changing existing auto/API behavior.
Changes:
- Introduces
VeniceWebUsageFetcherto fetch/parse subscription quota from the Venice web session endpoint. - Adds a
VeniceWebFetchStrategyand wires it intoVeniceProviderDescriptorfor.websource mode only (no fallback to API key in web mode). - Adds cookie import + cookie-header normalization helpers and a new Venice-specific
VeniceUsageErrormodel, with comprehensive tests.
File summaries
| File | Description |
|---|---|
| Tests/CodexBarTests/VeniceWebUsageFetcherTests.swift | New test suite covering JWT parsing, quota mapping, cookie header normalization, and explicit-web gating behavior. |
| Sources/CodexBarCore/Providers/Venice/VeniceWebUsageFetcher.swift | Implements the web session request and JWT-claim parsing into UsageSnapshot + monthly window semantics. |
| Sources/CodexBarCore/Providers/Venice/VeniceWebFetchStrategy.swift | Adds explicit-web-only fetch strategy that imports sessions and calls the web usage fetcher. |
| Sources/CodexBarCore/Providers/Venice/VeniceUsageError.swift | Adds Venice-specific error types and user-facing messages for web/cookie flow failures. |
| Sources/CodexBarCore/Providers/Venice/VeniceProviderDescriptor.swift | Adds .web source mode and selects cookie strategy only for explicit web; adds a helper for a monthly label. |
| Sources/CodexBarCore/Providers/Venice/VeniceCookieImporter.swift | Imports and merges browser cookies (macOS-only) and resolves Venice sessions per profile/source. |
| Sources/CodexBarCore/Providers/Venice/VeniceCookieHeader.swift | Extracts/normalizes the Venice auth cookie (including chunked variants) into a safe Cookie: header. |
Review details
Suppressed comments (2)
Sources/CodexBarCore/Providers/Venice/VeniceUsageError.swift:22
- These error strings also hard-code “Chrome”, but the web strategy/importer can pull cookies from non-Chrome sources; make the messaging browser-agnostic so it stays accurate with configured cookie import order.
case .anonymousSession:
"Venice Chrome session is anonymous and has no subscription quota."
case .expiredSession:
"Venice Chrome session token is expired. Sign in to venice.ai again."
Sources/CodexBarCore/Providers/Venice/VeniceUsageError.swift:24
missingQuotaalso mentions “Chrome”, but quota parsing is tied to the Venice web session JWT rather than a specific browser; this can mislead users when the cookie came from another browser/profile.
case .missingQuota:
"Venice Chrome session did not include subscription quota."
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// Window label for the cookie-based monthly quota view. | ||
| public static func primaryLabel(window: RateWindow?) -> String? { | ||
| guard window?.windowMinutes == ProviderPaceCapability.monthlyWindowSentinelMinutes else { | ||
| return nil | ||
| } | ||
| return "Monthly credits" | ||
| } |
| case .missingCredentials: | ||
| "Venice Chrome session cookie not found. Sign in to venice.ai in Chrome." | ||
| case .invalidCredentials: | ||
| "Venice Chrome session is invalid or expired. Sign in to venice.ai again." |
![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: aa37649330
ℹ️ 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".
| guard context.sourceMode == .web else { return [script] } | ||
| return [VeniceWebFetchStrategy()] |
There was a problem hiding this comment.
Although the descriptor now advertises .web, the app's ProviderRegistry still receives .auto for Venice because VeniceProviderImplementation has neither a sourceMode override nor a source picker, and its availability check still requires an API key/token account. Consequently the menu-bar app can never select this branch—even a cookie-only user who enables Venice remains unavailable—and the new strategy is effectively limited to CLI --source web. Add the corresponding app setting/source-mode and cookie-based availability wiring.
Useful? React with 👍 / 👎.
| guard let session = try self.sessionLoader().first else { | ||
| throw VeniceUsageError.missingCredentials | ||
| } | ||
| let usage = try await self.usageLoader(session.cookieHeader) |
There was a problem hiding this comment.
When Chrome has multiple signed-in profiles, importSessions() returns every candidate sorted by profile label, but this selects only the first one. If that profile's cookie is expired while a later profile is valid, the request fails with invalidCredentials and the valid session is never tried. Iterate candidates on authentication failures, as the other multi-profile web strategies do, rather than making alphabetical profile order determine whether Venice works.
Useful? React with 👍 / 👎.
| usageLoader: @escaping UsageLoader = { try await VeniceWebUsageFetcher.fetchUsage(cookieHeader: $0) }, | ||
| sessionLoader: @escaping SessionLoader = { try Self.defaultSessions() }) |
There was a problem hiding this comment.
The Venice strategy discards ProviderFetchContext.webTimeout and calls a fetcher whose request timeout is hard-coded to 15 seconds. This contradicts the CLI's advertised --web-timeout behavior in CLIOptions.swift:71 and also shortens the app's 60-second budget, so slow Venice responses can fail even when callers requested a longer timeout. Thread context.webTimeout through the loader and apply it to the request.
Useful? React with 👍 / 👎.
| public static func primaryLabel(window: RateWindow?) -> String? { | ||
| guard window?.windowMinutes == ProviderPaceCapability.monthlyWindowSentinelMinutes else { | ||
| return nil | ||
| } | ||
| return "Monthly credits" |
There was a problem hiding this comment.
This helper is never called outside its unit test; the repository-wide menu label switch in MenuDescriptor.rateWindowLabels has no Venice branch. Therefore a web snapshot carrying the monthly sentinel still renders the existing metadata label Balance, not the newly intended Monthly credits. Add Venice to that rendering seam so the quota window is identified correctly.
Useful? React with 👍 / 👎.
899dc82 to
fdfbfec
Compare
clawsweeper
Bot
removed
the
merge-risk: 🚨 compatibility
|
@clawsweeper re-review Addressed all four findings: token-account rejection before import, multi-session fallback, Linux auto exemption with CLI preflight coverage, and full app exposure (usage-source setting, picker, sourceMode routing) plus presentation label wiring and timeout threading. Details in the updated PR body. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review Addressed the two new findings: selected token accounts now resolve to the API script at the credential-adapter seam (no ambient fetch, no mislabeling; covered), and the Preferences detail shows the live source label. Details in the updated PR body. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
d19880d to
f3de3df
Compare
|
Live proof (head f3de3df): ran the real
Scratch live test passed ( Caveat for QA: the session came from the user's relayed browser, not the local Chrome profile, so |
Users signed in only in Brave got missing-credentials on --source web. Chrome stays first per cookie-import policy; Brave is second, matching the Alibaba/Augment precedent.
|
Follow-up (8b46ff6): Venice web import now tries Chrome, then Brave. Rationale: the live session used for the proof above lives in the user's Brave, and local-Chrome-only import would report missing-credentials for Brave-only users. Chrome stays first per repo cookie policy; Brave-second matches the Alibaba/Augment precedent. Adds |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
A configured manual cookie header now feeds the web fetcher directly (Abacus pattern) instead of forcing browser import. Falls back to Chrome/Brave import when the source is auto. Adds VeniceProviderSettings snapshot section plus strategy tests.
|
Follow-up (eed1555): the web strategy now honors a stored manual cookie header (Abacus pattern) instead of forcing browser import. Motivation: a valid session cookie can exist in config while the browser profile is unreadable (sandboxed relay browsers, TCC-blocked profiles). Adds a |
clawsweeper
Bot
added
P1
Registering a Venice settings section without an app-side contribution tripped the startup snapshot precondition and trapped on launch. Mirror the Abacus wiring: SettingsStore snapshot helper plus implementation override.
|
Follow-up (4f8100d): fix launch trap. Registering the Venice settings section without an app-side snapshot contribution tripped the startup assembly precondition ( |
Cookie-based Venice subscription quota via the signed-in venice.ai session (monthly refill windows from the session JWT), as an explicit-web-only venice.web strategy.
Never runs in automatic mode, so no surprise browser-cookie prompts. Auto/API behavior unchanged.
Review follow-ups addressed (round 2):
Verified: all Venice suites plus gatekeeper/registry/CLI/preflight green, full lint-macos clean.