Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Comment |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
⏳ DRAFT
What to do
Its title has been prefixed with |
Owner
Author
|
Closing as already covered upstream. Current |
luvs01
pushed a commit
that referenced
this pull request
Aug 11, 2026
…ice grant + free/paid live catalog (lidge-jun#1397) * feat(providers): add Nous Portal (Nous Research) OAuth provider — device grant + free/paid live catalog (Closes lidge-jun#1148) * feat(providers): seed Nous Portal free models from live Portal list (hy3, laguna-s/xs, step-3.7-flash) * test(nous-oauth): cover device-flow error paths and refresh-token fallback - access_denied / expired_token surface as terminal NousTokenError - slow_down backs off (interval bump) then resumes polling to success - authorization_pending until deadline raises a timed-out error - refresh omitting a new refresh_token keeps the previous one (header sent) * fix(oauth/nous): enforce HTTPS base URL and single-use refresh rotation; docs + tests Addresses the two CHANGES_REQUESTED blockers on PR lidge-jun#1397: 1. resolvePortalBaseUrl() now hard-validates the full OAuth base URL via new URL() and throws BEFORE any fetch is dispatched: rejects non-HTTPS schemes, embedded credentials, query strings, and fragments; returns only url.origin. Aligns opencodex with Hermes hermes_cli/auth.py (_NOUS_PORTAL_ALLOWED_HOSTS, https-only) and prevents the single-use refresh token / inference JWT from ever traversing cleartext. 2. parseTokenPayload() no longer falls back to the submitted refresh token. A response that omits refresh_token, or returns a replacement equal to the submitted token, throws NousTokenError(oauthError: 'refresh_token_reused') so the next refresh cannot replay a consumed credential and trigger session revocation. Also: - tests/nous-oauth.test.ts: HTTPS/URL hardening (fetch never reached), missing/equal refresh rejection, and NousTokenError.oauthError contract on access_denied / expired_token. - tests/nous-oauth-live.test.ts: opt-in, CI-skipped live verification that reads the local refresh token without printing it (lengths only), asserts rotation + read-only /v1/models reachability. No provider key is shared. - docs ru/guides/providers.md: eight OAuth presets, ocx login nous, nous row. Verified: tsc --noEmit, bun test nous-oauth (17/17), privacy:scan passed, targeted suite 186/186. Full bun run test in progress. * fix(oauth/nous): failure-atomic refresh, terminal errors, scope check, redirect guard Addresses the 10 review points from Wibias on PR lidge-jun#1397: - #2 Single-use refresh is now failure-atomic. A durable refresh-intent file (keyed by a sha256 of the refresh token, never the token in cleartext) is written before the refresh request and cleared only after the rotated token is obtained. If the server responds but the rotation cannot be persisted, the intent is marked 'uncertain' and a later refresh REFUSES to replay the possibly-consumed token (NousTokenError refresh_token_reused, terminal) — forcing a clean re-auth instead of a session-revoking replay. - #3 Credential-bearing OAuth requests (device + token) now pass redirect: 'error' so custom auth headers cannot follow a cross-origin redirect. - #4 invalid_token (and invalid_grant/revoked/revoked_token) are now terminal NousTokenError values that drive re-authentication. - #5 The returned access-token JWT scope is validated for inference:invoke before the credential is treated as usable. An insufficient-scope token is a terminal error that STILL surfaces the already-rotated refresh token, so the caller can persist it and re-auth without discarding the rotation. - #6 Live /models test accepts both the OpenAI-style { data: [...] } body and a bare array (production contract). - #7 freeTier is no longer true for the mixed free/paid provider; free models are classified at model level (the :free slugs). Parity test updated. - #8 pollForToken parses the response body once and passes the payload through to the error path instead of re-reading a consumed body. - #9 sleep() now removes its abort listener on both resolve and abort, so polling iterations do not accumulate listeners. - #1 The live test is now non-destructive: it persists the rotated token back through mergeAccountCredential (prod path), so the local session stays valid. - #10 Russian docs already mirror the English source (8 presets, ocx login nous, nous table row with device grant + single-use rotation). No provider API key is shared; privacy:scan passes. Verified: tsc --noEmit, nous-oauth 21/21, provider-registry-parity + targeted suite 193/193. * fix(oauth/nous): close the uncertain-outcome window for single-use refresh Deep re-review (real execution proof) showed the first intent design still relied on the server to refuse a replay when the rotated token was obtained but lost before the store persisted it. Harden the contract: - The refresh-intent file now stays in the 'submitted' state after a successful rotation (it previously cleared it). It is only cleared by the account store via clearNousRefreshIntent() once mergeAccountCredential persists the rotated token. - Replaying a token whose intent is 'submitted' OR 'uncertain' is refused up front (NousTokenError refresh_token_reused, terminal) — never blindly replayed, and without depending on the server's reuse detection. - Network-level failure (server never saw the token) still clears the intent so a retry is safe. - clearNousRefreshIntent is wired into the shared refresh orchestrator (src/oauth/index.ts) right after mergeAccountCredential; it is a no-op for non-Nous providers (they never write an intent). Verified by a real execution probe (not just mocks): a rotation that obtains the rotated token but crashes before persistence now makes the next replay of the old token refused by the guard, with the intent present on disk. Tests: nous-oauth 23/23 (adds 'rotated token obtained but not persisted blocks replay', '200 unparseable body marks uncertain', 'network failure replayable'); targeted suite 195/195. tsc + privacy:scan clean. * fix(oauth/nous): fail-closed refresh-intent, hardened IO, shared-classifier terminal Addresses the remaining CHANGES_REQUESTED findings from Wibias on PR lidge-jun#1397 (head after this: fail-closed end-to-end single-use refresh recovery). 1. Refresh-intent is now FAIL-CLOSED and reuses the repo's hardened config IO: - writeRefreshIntent uses atomicWriteFile + hardenConfigDir (owner-only 0o700 dir) and THROWS on failure instead of swallowing it (refresh is refused rather than proceeding blind). readRefreshIntent treats any read/parse/permission error as 'uncertain' (replay refused), never as absent. clearNousRefreshIntent surfaces non-ENOENT failures. - Ambiguous fetch failures (timeout/abort/connection) now mark the intent 'uncertain' instead of clearing it: dispatch may have occurred, so the submitted token must never be replayed. 2. Post-persist cleanup is wired into the correct coordinator (refreshGenericAccountWithLock, the actual Nous path) after a successful mergeAccountCredential; removed the misplaced call from the xAI path. 3. Shared terminal classifier now honors NousTokenError.terminal (so provider-classified invalid_token / insufficient_scope move the account to re-authentication instead of staying retryable). 4. Opt-in live test refreshes through the production, generation-aware, account-locked coordinator (refreshGenericAccountWithLock) instead of calling refreshNousToken + mergeAccountCredential outside the lock. 5. First normal refresh-wiring test now isolates OPENCODEX_HOME so it cannot leave durable intent state in the config tree. 6. Embedded-credential URL validation no longer echoes the raw (credential- bearing) URL in the thrown error. 7. NousTokenError no longer stores live credentials as an enumerable property; only the rotated refresh token is retained, via a non-enumerable getter (getRotatedRefresh), so structured logging/serialization cannot leak it. 8. Replay-guard test now proves fetch is never called (not just the error shape). 9. Provider docs (ja/ko/zh-cn) updated to 'eight' OAuth presets to match the English/Russian sources. Verified by a real execution probe (not just mocks): rotation obtained but not persisted -> next replay refused by guard; network failure -> fail-closed uncertain (not replayable); insufficient_scope error does not leak credentials. Tests: nous-oauth 23/23 (adds fail-closed network-failure, replay-guard proves-no-fetch, non-enumerable credentials); targeted suite 195/195. tsc --noEmit and bun run privacy:scan clean. Kept draft, no maintainer-sponsored. * fix(oauth/nous): fail-closed refresh-intent schema, atomic HTTP failure, non-terminal local IO - Validate persisted refresh-intent schema; corrupt/unknown state is treated as uncertain (replay refused), never absent. Only ENOENT means no intent. - Classify HTTP refresh failures atomically: ambiguous 5xx/gateway responses leave the submitted token blocked (uncertain); only definitive 4xx client rejections clear the intent for a safe retry. - Surface local durable-write/read/cleanup failures as a non-terminal RefreshIntentIOError so the coordinator does not mark a valid credential needsReauth for broken local persistence. - Mark device-flow access_denied/expired_token as terminal consistently. - Handle non-JSON successful device-code bodies with the clear validation error instead of a raw JSON parse leak. - Redact raw values from malformed base-URL diagnostics. - Align the opaque-token docstring with the JWT scope gate. - Synchronize OAuth provider lists across en/ja/ko/ru/zh-cn docs. - Add regression coverage for all safety contracts. * test(oauth/nous): cover origin-only base URL normalization * test(oauth/nous): make intent-write failure tests platform-independent Planting a file at the intent-directory path made the guard read fail with ENOTDIR on Linux (treated as uncertain -> terminal) before any write could fail, so the test could not reach the non-terminal operational-error path. Force atomicWriteFile to fail via a spy instead, deterministically on every platform: the pre-dispatch write abort must surface RefreshIntentIOError, never call fetch, and leave the account valid. * fix(oauth/nous): fail closed on every ambiguous post-dispatch refresh outcome A non-2xx response does not prove the single-use refresh token was not consumed: 429 rate limits, unknown/custom 4xx, and gateway-generated client-class errors can be returned after the remote side already processed the token. Previously every 4xx cleared the durable refresh intent, which made a possibly-consumed RT-A locally replayable. Now every post-dispatch non-2xx response retains the intent as uncertain (previously only 5xx did), so the submitted token stays blocked and a later refresh is rejected before any fetch. The intent is cleared only after the rotated credential is durably persisted. Pre-dispatch local I/O failures remain distinct non-terminal operational errors. Replace the invented 'safe 4xx' test with regressions proving HTTP 429 and an unknown/custom 4xx both keep the old token blocked and reject a second attempt before fetch (exactly one token-endpoint call). * fix(oauth/nous): post-persist intent cleanup is best-effort; docs/live-test/modelDiscovery cleanups - refreshGenericAccountWithLock: a failure to unlink the old-token refresh- intent file after mergeAccountCredential commits the rotation no longer fails the refresh or marks the account needsReauth. The stale intent keys the old token (no longer stored), so retaining it is safe; the failure is logged non-fatally with no credential material. - Add coordinator-level regressions: the happy path persists RT-B and clears the RT-A intent (nousRefreshIntentBlocksReplay(RT-A) === false), and a forced cleanup failure still resolves with the fresh access token while the stored credential stays RT-B and the account is not marked needsReauth. - Add the provider-level clear-after-persist regression in nous-oauth.test.ts. - English providers doc: after a terminal Nous refresh failure, run 'ocx login nous' to reauthenticate. - Live test: correct the privacy wording (opt-in; credentials go only to the intended Nous endpoints; token values never printed) and parse the live catalog defensively so malformed bodies yield an empty list instead of a crash. - Nous registry modelDiscovery: use path 'models' (resolves against effectiveBaseUrl to the same canonical /v1/models endpoint). * docs(providers): sync Nous Portal details across ja/ko/ru/zh-cn Add the missing ocx login nous command, the full ous provider table row (openai-chat adapter, inference endpoint, device-grant login, per-request inference JWT, live paid/:free discovery, single-use rotated refresh tokens), and the terminal-refresh reauthentication instruction to each translated provider guide, matching the English source. * fix(oauth/nous): preserve rotated RT-B on terminal refresh errors; trim live-test model ids - refreshGenericAccountWithLock: when a terminal NousTokenError carries an already-issued rotated refresh token (e.g. access JWT lacks inference:invoke), persist RT-B generation-safely before forcing reauthentication. The unusable access token is never persisted as valid (empty placeholder, past expiry); RT-A's intent is cleared only after RT-B is durable (best-effort cleanup); persistence failure or a superseding concurrent generation never clears RT-A intent and never overwrites the newer credential; the account is marked needsReauth generation-safely and the caller receives OAuthLoginRequiredError. - Live catalog test: reject empty/whitespace-only model ids (trim before accept). - Coordinator regressions: RT-B preservation on insufficient_scope, RT-B persistence failure keeps RT-A intent blocking, superseded concurrent generation is not overwritten, cleanup failure after RT-B persistence keeps RT-B and marks needsReauth. --------- Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
github-actions
Bot
deleted the
codex/propose-fix-for-history-retagging-issue
branch
August 28, 2026 18:51
lidge-jun
added a commit
that referenced
this pull request
Sep 12, 2026
…he tool-catalog nudge (lidge-jun#4415) * refactor(devin): retire the ACP adapter and give the shared adapter the tool-catalog nudge Both Devin provider rows already stream Cognition's Connect-RPC api-server on the `devin` adapter. They differ only in where the credential came from: a browser sign-in through RegisterUser, or the `devin-session-token` the installed CLI already wrote to its own credentials.toml. A second adapter registered under the id `devin-cli` still spawned `devin acp` and drove the child over Agent Client Protocol on stdio. Nothing routed to it: `routedProviderConfig` pins the adapter from the registry for any registry id, so only a custom-named row such as `"devin-acp"` could select it. It is removed rather than kept, because the premise that justified it was false. The CLI's credential is the ordinary cloud token, so importing it does everything the child did without a placeholder `buildRequest`, a disabled `parseStream`, an identity-only `baseUrl` no request may connect to, and a subprocess running in the operator's own tree. `projectDevinCliAuthMode` used to warn and change nothing when a saved row still named that adapter, reasoning that routing already pinned the transport. That held only for the registry id. With the adapter gone a custom-named row has nothing pinning it and would throw `Unknown adapter: devin-cli` on every request, so the migration now rewrites every row naming the retired id whatever the row is called, and repoints a row still carrying the identity-only `cli.devin.ai` host at the api-server in the same pass. The nudge is the other half. Every non-OpenAI adapter that advertises a client tool catalog injects `buildNonOpenAIToolCatalogNudgeForTools` into its system prompt; Devin advertises a real catalog on proto field #10 and was the only one without the paragraph. It goes into `mapOcxMessagesToDevin`, which covers both provider rows at once. The wire-name callback is `tool => tool.name` rather than the default namespaced form, because `mapOcxToolsToDevin` writes the bare name — a nudge listing names the model is never offered is worse than none. The ACP wire could never have carried it: `session/prompt` takes prompt text only, with `capabilities: {}` and `mcpServers: []`. * fix(devin): share the api-server default and drop the out-of-scope GUI comment The migration wrote its own copy of the Cognition host, so a later change to the shared default would have left it writing the old address. It now imports DEVIN_DEFAULT_API_SERVER. The GUI comment rewrite went back to its dev state. It was unrelated cleanup by the PR checklist's own standard, and because the screenshot gate is path-based it was asking a comment-only diff to produce a screenshot of nothing. It can go in on its own.
luvs01
pushed a commit
that referenced
this pull request
Sep 12, 2026
…idge-jun#4426) The adapter asserted that Cognition has no reasoning-replay field and dropped every assistant thinking block, so a reasoning model restarted its chain on each turn of a tool loop and paid for it again. The field exists. Two independent clients of the same service write it on the assistant prompt: #11 thinking, #12 signature, #18 signature_type. The response side was half-missing too. delta_thinking (#9) was decoded but delta_signature (#10) was not, so even once the prompt could carry a signature there was never one to carry: the replay would always have been unsigned. #10 now surfaces as a thinking_signature event. Reasoning still stays out of the replayed assistant TEXT, which is what the original comment was right about — folding chain-of-thought into the visible content makes the model treat it as something it said to the user. It rides its own field instead. A turn that produced only reasoning is now replayed rather than skipped, since dropping it is what forces the next turn to re-derive the same chain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
restoreNativeCodexAsync()wrapper was scheduling a history restore job unconditionally after calling the synchronous restore path, which allowed the history worker to retag routed threads back to the native/OpenAI provider even when the synchronous restore preserved an external provider; this change prevents that unintended retagging.Description
restoreNativeCodexCore()that returns an additionalrestoreHistoryboolean indicating whether the async wrapper should run the history worker, and keep a thin publicrestoreNativeCodex()shim that preserves the original return shape.restoreNativeCodexAsync()to call the core and return early whenrestoreHistoryis false so the history worker is not scheduled for externally-preserved providers.restoreNativeCodexCore()symbol.Testing
bun run typecheck, which succeeded.bun run privacy:scan, which succeeded.git diff --checkand committed the patch asfix(codex): preserve external history on async restore.bun test tests/codex-history-job.test.ts tests/codex-inject-integration.test.ts, but the test run was blocked by the environment's Bun/runtime mismatch (installed Bun 1.2.14 fails due tonode:zlibnot exportingzstdDecompressSync), causing the integration subprocess-based tests to produce no usable output; the new regression test is present and should pass in CI or a local environment with a compatible Bun/runtime.