Repository navigation
Conversation
roboomp
added
auth
![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: f80c52cf20
ℹ️ 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".
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
Thanks for the thorough, well-structured provider @jroth1111 — the sand-vs-cursor-vs-xai distinction, catalog wiring, and proto round-trip tests are clearly done with care.
Ranked review:p3 — not a quality verdict but because this needs a maintainer policy decision first: the provider mints credentials against Cursor's private api2.cursor.sh sand endpoint and forges the Cursor client identity (x-cursor-client-type, reproduced x-cursor-checksum, stamped client version, x-ghost-mode). No linked issue or confirmed maintainer intent for a 5930-line, 22-file addition. @can1357 should accept/reject that before code review carries weight.
If accepted, two things to fix: (1) proto.ts is under a file-wide // @ts-nocheck, disabling types across the whole 754-line codec — at least give the exported encode/decode surface real signatures; (2) the module-level cachedToken in auth.ts isn't keyed by renewal credential or backend, so different baseUrl/credential in one process can get a bleed of the wrong JWT. Plus a note that seed cost is hardcoded $0. Conventions otherwise look good: CHANGELOG entries in all three packages, generator-driven models.json, lazy-loaded provider module matching the existing pattern.
| } | ||
|
|
||
| /** Cursor sand checksum: obfuscated floor(now/1e6) bytes + machine id. */ | ||
| export function createGrokbotChecksum(machineId: string, nowMs = Date.now()): string { |
There was a problem hiding this comment.
blocking (policy — maintainer call): this provider mints credentials against Cursor's private api2.cursor.sh sand endpoint and forges the Cursor desktop client identity — x-cursor-client-type: sand, a reproduced x-cursor-checksum (enhancedObfuscate + machine id), a stamped x-cursor-client-version (0.30.0-pre.16), and x-ghost-mode (see grokbot.ts L495-505). That is impersonating another vendor's proprietary client to access their infra, which is a repo-policy decision, not a code-review nit. Flagging for @can1357 to accept/reject before the rest of the review has any weight.
There was a problem hiding this comment.
Acknowledged — this is a maintainer policy call, not something we can resolve in code review. Leaving open for @can1357 to accept or reject Cursor sand client-identity / api2.cursor.sh access before the rest of the provider lands.
|
Follow-up: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a53fc5a6f0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ebdd137d0
ℹ️ 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".
jroth1111
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4256dd5149
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb6862f5c3
ℹ️ 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".
💡 Codex Review Replace the remaining inline discovery import
The earlier provider-loader import was fixed, but this separate catalog loader still introduces AGENTS.md reference: AGENTS.md:L37-L37 https://github.com/can1357/oh-my-pi/blob/88b940088a708029dd4d67673554341292c3233c/packages/catalog/src/models.json#L67527-L67531 Fresh evidence after the prior effort-ladder thread is that the final bundled AGENTS.md reference: AGENTS.md:L185-L185 oh-my-pi/packages/coding-agent/src/config/model-resolver.ts Lines 435 to 437 in 88b9400  Preserve exact model IDs when aliases collide
When one model's alias equals another model's canonical ID, this branch replaces the entry with the ambiguous sentinel, so even an exact oh-my-pi/packages/ai/src/providers/grokbot.ts Lines 714 to 718 in 88b9400 Treat input-token-limit frames as errors
When the backend reports a oh-my-pi/packages/ai/src/providers/grokbot.ts Line 775 in 88b9400 Do not record a model ID as the upstream provider
Whenever ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
💡 Codex Review Replace the remaining dynamic discovery import
Although the provider loader was corrected, this newly added catalog thunk still uses a dynamic AGENTS.md reference: AGENTS.md:L37-L37 https://github.com/can1357/oh-my-pi/blob/f687c51f8045134b4548bc846dbdf4f272cb4b00/packages/catalog/src/models.json#L67527-L67531 Fresh evidence after the earlier fix claim is that the committed AGENTS.md reference: AGENTS.md:L183-L185 oh-my-pi/packages/ai/src/providers/grokbot.ts Line 784 in f687c51 Keep routed model IDs out of upstreamProvider
When Sand returns ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e1a1daf3a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a67df40fad
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 414722ccab
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 586c8d5e8d
ℹ️ 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".
55c2d2d to
f22209d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4af672e19
ℹ️ 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".
💡 Codex Reviewhttps://github.com/can1357/oh-my-pi/blob/ccc50dce5b058769c446e155db4194f535dc4416/packages/catalog/src/models.json#L67610-L67612 Fresh evidence after the earlier effort fixes is that the committed bundle still gives AGENTS.md reference: AGENTS.md:L183-L185 Preserve configured path prefixes in auth URLs
When ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Related
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ddc2b73f8
ℹ️ 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".
| } | ||
|
|
||
| closeOpen(); | ||
| for (const state of toolStates.values()) finishTool(state); |
There was a problem hiding this comment.
When the backend sends a ToolCallPart with isComplete: false and then a valid Connect end-stream trailer, this loop calls finishTool anyway, parses potentially partial JSON as {}, and emits a successful toolUse response. That can execute or persist a tool call the provider explicitly marked incomplete; only states whose completion flag was received should be finalized, while remaining states should make the stream incomplete.
Useful? React with 👍 / 👎.
| export { shortenPath as shortenGrokbotDisplayPath } from "@oh-my-pi/pi-utils"; | ||
|
|
||
| /** Max display cells for `/grokbot` status field values (matches TUI title width). */ | ||
| const STATUS_VALUE_MAX_WIDTH = 60; |
There was a problem hiding this comment.
The fresh status-sanitization fix introduces a provider-local width of 60, which only coincidentally matches TRUNCATE_LENGTHS.TITLE and will drift when the shared TUI limits change. Move or expose the shared limit from a package pi-ai can consume instead of adding another ad-hoc display constant.
AGENTS.md reference: AGENTS.md:L207-L210
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 591872c4f7
ℹ️ 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".
| } catch { | ||
| return {}; |
There was a problem hiding this comment.
When a ToolCallPart has isComplete: true but contains malformed JSON, this catch silently converts its arguments to {}; the new incomplete-tool guard therefore does not run, and the response completes with stopReason: "toolUse". A zero-argument tool can then execute unintentionally instead of the provider response being rejected. Fresh evidence beyond the prior incomplete-tool finding is that a completed malformed part still bypasses the new guard; propagate the parse failure.
Useful? React with 👍 / 👎.
| let key = id; | ||
| if (!key && typeof indexHint === "number") key = `idx:${indexHint}`; |
There was a problem hiding this comment.
When successive ToolCallPart frames for one call do not repeat both identifiers—for example, the first frame supplies toolCallId plus toolIndex and a later frame supplies only the index—lookup changes from the id key to idx:<n>, but the original state was never registered under that index. The later chunk consequently creates a second tool call, and the otherwise valid stream ends as incomplete or exposes duplicate calls; resolve and register both identifiers to the same state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e92b4e22e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const headers: Record<string, string> = { | ||
| ...options?.headers, | ||
| ...grokbotClientHeaders(authCfg), |
There was a problem hiding this comment.
When a custom Grokbot model or reverse-proxy override supplies required headers through model.headers, this request includes only per-call options.headers, so headers such as a proxy API key are silently omitted and the inference call fails. Other provider transports merge model-level and call-level headers; include model.headers here as well while retaining the provider-owned auth/header precedence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32b08d0b35
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const identity = resolveGrokbotDiscoveryIdentity({ | ||
| namespace: config.namespace, | ||
| clientVersion: config.clientVersion, | ||
| }); |
There was a problem hiding this comment.
When an authenticated Grok Bot catalog is refreshed, this factory calls resolveGrokbotDiscoveryIdentity(), which synchronously reads secrets/grokbot.env; constructing cacheProviderId then invokes the same resolver and reads the file synchronously again. On a slow or network-mounted agent directory, background model refresh therefore blocks the TUI event loop. Fresh evidence after the earlier async-loading fix is that cache-scope construction has reintroduced the synchronous reader outside the registry callback; resolve the identity asynchronously once and pass it through without rereading the file.
AGENTS.md reference: AGENTS.md:L133-L137
Useful? React with 👍 / 👎.
fix(gc): address post-merge Codex findings on omp gc --compress
… add pricing Antigravity discovery serves Gemini 4 Argon as gemini-4-argon-low/-medium/-high SKUs, which showed as three models. A google-level variant family folds them into one gemini-4-argon row with low, medium, and high thinking levels, each routed to its SKU. The existing Antigravity pricing peer prices it at Google's first-party rate; the regenerated bundle carries it. Cursor's GetUsableModels carries no pricing, so Argon gets Cursor's published $2/$10, $0.10 cache-read card.
Neither discovery mapper read its gateway's pricing, so every row fell back to stencil.so or the bundle: Gemini 4 Argon showed $4/$20 on both against a live $2/$10, and many Kilo and NanoGPT rows showed $0. Kilo now maps its OpenRouter-shaped per-token pricing; NanoGPT requests its detailed roster and maps its per-million card (cache legs per thousand tokens). Names and limits keep their existing sources; legs a gateway leaves unpriced still fall back.
Google ends Argon's introductory $2/$10 ($0.10 cached) price on 2026-11-30 and bills $4/$20 ($0.20 cached) from 2026-12-01, midnight US Pacific when Cloud Billing's day starts. cost-patch now accepts the dated effective-rates cards cost-fallback already had, so a rule can move a price on a date without a catalog update. Google, Vertex, and Antigravity Argon rows take the dated card over whatever stencil.so or the bundled pricing peer reports; Cursor, which bills Other Models at the model's API price, schedules it on its own card. Cursor's rich-discovery card reader honors the same cards.
Dated cost-patch/cost-fallback cards were applied only when a row was built, so a row reused past the change (the committed Antigravity Argon bundle row, a model cache, a long session) kept billing the old price. buildModel now compiles every price period into cost.timeBased.effectiveRates, with a leading card for the pre-change price, under a schedule with no peak windows and a 1x multiplier; calculateUsageCost picks the card by request timestamp. hasRecurringTariff separates these dated-only schedules from peak/off-peak tariffs: no status-line arrow, and stats reprices a recorded $0 on them at the card for the request's date instead of freezing it. The Antigravity bundle is regenerated so its Argon row carries the schedule.
…ice in force Google's Argon rule carried only the 2026-12-01 card, so once upstream reported $4/$20 the pre-switch period derived from it vanished and a row built afterwards priced November requests at list. The rule now pins the $2/$0.10/$10 card beside it. Price displays read the row's scalar legs, which keep the card current when the row was built, so a bundle, cache or long session showed a superseded price; getEffectiveTokenCost returns the card in force and the model browser and omp models --json use it. This also corrects DeepSeek V4 Pro, listed at $1.32/$3.96 though billed at its $0.30/$1.20 card since 2026-09-14.
…dules on rebuilt rows and Cursor sibling lanes
…Argon thinking tiers and add pricing (@eggpeat)
…rom their live model lists (@eggpeat)
…pt card; Ctrl+C clears an approval draft
…proved input; queue Enter behind a landing paste
…s; a steer denies queued approvals too
Spawn acknowledgments no longer repeat the background-job contract; the Task description carries it once, and same-name extension replacements inherit it through AgentTool.delegationGuidance.
…l descriptions (@iliaal)
…rl+O to expand; drop transcript-card matching
…rows clipped to width
…ding details restarts the approval timeout
…uring tool approval, drop duplicated call details (@H4vC)
12 participants
What
Contributor's own words:
Adds distinct native Sand inference and host-managed GrokBotService chat connectors, account-scoped discovery, Sand allowance reporting, and opt-in Claude text-tool transport. Ported onto current upstream
581eaeecfbin logical commits; frontend demos, private API experiments, UI-created-bot adoption and gate/research artifacts are excluded.Why
Preserves explicit requested models and account-advertised packed Opus 5.5 effort routes, native signed content, product tool ownership and completion boundaries. Host-managed chat is an explicit text-only route—not a silent fallback or a claim of Opus serving. It uses its own temporary conversation, user-nonce correlation, independent transcript readback and ownership-checked cleanup.
Optional issuer-provided workload authorization is forwarded only to native inference. The connector does not forge or mint attestations, inspect another workload, grant entitlement, redeem resets or change billing limits. Newer upstream Connect decoding and unrelated catalog corrections are preserved; common framing lives in utilities rather than duplicated provider implementations.
Maintainer acceptance of the private vendor endpoints remains unresolved. This PR stays draft; neither technical fixes nor live chat success resolve the policy objection.
Testing
bun run check:tspasses: lint, formatting and all package TypeScript checks. Full rootbun check/ Rust suite is not claimed; no Rust implementation changes.responseInfo.model, signed Claude content, Opus 5.5 generation or live Claude text-tool round trip is claimed.Follow-up verification (10 October, Melbourne)
/grokbotstatus capability, using canonical credential/terminal helpers with host URL and control-character redaction.All three branches incorporate upstream 581eaee. No review comments were posted or threads resolved; policy decisions and comment approval remain separate from code fixes.
bun checkpasses