feat(providers): add Nous Portal (Nous Research) OAuth provider — device grant + free/paid live catalog - #1397
Conversation
github-actions
Bot
added
the
intake: hygiene-blocked
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds Nous Portal device-grant OAuth, rotating refresh tokens, identity extraction, provider registration, live model discovery, validation tests, and documentation updates. ChangesNous Portal integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant OAuthController
participant NousPortal
participant CredentialStore
participant NousInference
User->>OAuthController: ocx login nous
OAuthController->>NousPortal: request device authorization
NousPortal-->>OAuthController: return verification URL and user code
OAuthController->>NousPortal: poll for authorization
NousPortal-->>OAuthController: return access and rotated refresh tokens
OAuthController->>CredentialStore: persist account credentials
User->>NousInference: request models or chat completion
NousInference-->>User: return provider response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/ru/guides/providers.md`:
- Line 63: Update the Russian providers documentation sections around the preset
count, login commands, and provider table to match the English source: change
the OAuth total to eight, add the ocx login nous device-grant command, and add
the Nous provider row covering the openai-chat adapter, inference endpoint, live
paid/free model discovery, and rotated refresh tokens.
In `@src/oauth/nous.ts`:
- Around line 67-69: Update resolvePortalBaseUrl in src/oauth/nous.ts (lines
67-69) to parse the configured URL and reject non-HTTPS schemes, embedded
credentials, query strings, and fragments before returning the normalized base
URL. Update tests/nous-oauth.test.ts (lines 8-9) to use an HTTPS TEST_PORTAL and
add coverage proving an HTTP override fails before fetch is invoked.
- Around line 149-167: Update parseTokenPayload to remove refreshFallback and
require a non-empty refresh_token in every response; reject a returned token
equal to the refreshToken supplied to the refresh flow, and adjust that caller
to pass no fallback while preserving initial token parsing. Add a regression
test covering an omitted replacement refresh token and the consumed-token reuse
case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bd2bf16e-886e-40d2-8c88-40912a1f2872
📒 Files selected for processing (10)
docs-site/src/content/docs/guides/providers.mddocs-site/src/content/docs/ja/guides/providers.mddocs-site/src/content/docs/ko/guides/providers.mddocs-site/src/content/docs/ru/guides/providers.mddocs-site/src/content/docs/zh-cn/guides/providers.mdsrc/oauth/index.tssrc/oauth/nous.tssrc/providers/registry.tstests/nous-oauth.test.tstests/provider-registry-parity.test.ts
Code review (manual pass, 2026-08-10)Reviewed Non-blocking findings (no code change required before merge)
Windows / platform questionNo platform-specific code here: the flow is a pure RFC 8628 device grant — OpenCodex displays the verification URL + code ( CI note
|
|
Gaps de tests identifies en review : couverts dans le commit 6989a2e.
Verification : |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/nous-oauth.test.ts`:
- Around line 157-171: The tests around loginNous must assert the NousTokenError
contract, not only matching messages. Update the access_denied and expired_token
cases to verify rejection with NousTokenError and confirm the error’s oauthError
value preserves the corresponding OAuth code, while retaining the existing
message assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17ba1740-a7de-466c-81bd-baf695a97538
📒 Files selected for processing (2)
src/providers/registry.tstests/nous-oauth.test.ts
flyingsquirrel0419
left a comment
flyingsquirrel0419
left a comment
There was a problem hiding this comment.
The provider direction may be useful, but the current exact head 6989a2eae8fe1333d35cbf950e1b8389f05400a7 is not safe to merge yet.
Two code blockers are confirmed:
resolvePortalBaseUrl()acceptshttp:(and does not reject credentials/query/fragment) before either device or refresh requests are built.refreshNousToken()then sends the bearer-equivalent single-use refresh token inx-nous-refresh-tokento that destination. Validate the complete OAuth base URL and require HTTPS before any credential acquisition or dispatch; add a negative test provingfetchis never reached for an HTTP override.- The file documents Nous refresh tokens as single-use and rotated on every successful refresh, but
parseTokenPayload(..., refreshToken)falls back to the already-consumed token when the response omitsrefresh_token. Reject a missing replacement and a replacement equal to the submitted token, with focused tests, so the next refresh cannot reuse a consumed credential or trigger session revocation.
The existing unresolved translated-doc and terminal-error contract comments also need resolution. After those fixes, rebase onto current dev (this head is 43 commits behind), obtain exact-head green CI, and provide a dated real-account login/refresh/logout plus live-catalog/chat test and primary-source client-registration/endpoint evidence. Until then this should remain draft and should not receive maintainer-sponsored.
6989a2e to
e9d9d9e
Compare
…on; 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.
|
@flyingsquirrel0419 — addressed. Both blockers fixed, plus the unresolved doc/contract comments. All changes verified locally; no provider key is shared (privacy:scan passes, no live creds touched, live test is CI-skipped and prints no token). Blocker 1 —
|
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes. I found two merge-blocking OAuth correctness issues plus several medium/low issues that should be fixed before merge.
Merge blockers:
tests/nous-oauth-live.test.tsis destructive: it refreshes a saved single-use refresh token but intentionally does not persist the rotated token. That can leave the developer's stored Nous session holding an already-consumed refresh token and force re-authentication.- Single-use refresh is not failure-atomic. The per-account lock protects concurrent writers, but it does not protect an uncertain outcome where the server consumes RT-A and returns RT-B, then the client loses the response/crashes/parsing fails before RT-B is persisted. Retrying RT-A can then trigger reuse detection. This needs durable refresh-intent/uncertain-outcome handling, or an equivalent recovery contract that never blindly replays a possibly-consumed refresh token.
Other required fixes:
3. Credential-bearing refresh requests should reject redirects (redirect: "error") so custom auth headers cannot follow a cross-origin redirect.
4. Treat invalid_token as a terminal Nous refresh error and move the account to re-authentication.
5. Validate the returned access-token scope, including the required inference permission, before treating the credential as usable. Make sure this does not discard an already-rotated refresh token.
6. The live /models test assumes an array response, while the runtime/server contract uses an OpenAI-style { data: [...] } object. Reuse the production parser if possible.
7. freeTier: true is misleading for a mixed free/paid provider. Use model-level cost classification or avoid labelling the entire provider as free.
8. pollForToken() consumes the response JSON, then the fallback error path tries to read the body again, which loses useful unknown OAuth error details. Pass the parsed payload through instead.
9. Polling sleep accumulates abort listeners on normal timer completion. Remove the listener when the timer resolves.
10. Russian Nous documentation is incomplete/inconsistent with the added provider/login flow.
I also checked several adjacent concerns that are already handled: HTTPS enforcement for custom Nous URLs, rejection of credentials/query/fragment in the base URL, per-account cross-process locking, reread-after-lock, credential generation checks, successful rotated-token persistence, production { data: [...] } model parsing, and obvious refresh-token logging paths.
CI for reviewed head e9d9d9ebe109629d0f5770d83265d789ae1a4133 was not fully green when checked: Linux shard 3/4 failed while the other inspected shards/React Doctor passed. I could not establish from the available log data whether that failure is caused by this PR, so it should be resolved or shown to be unrelated before merge.
…, redirect guard Addresses the 10 review points from Wibias on PR lidge-jun#1397: - lidge-jun#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. - lidge-jun#3 Credential-bearing OAuth requests (device + token) now pass redirect: 'error' so custom auth headers cannot follow a cross-origin redirect. - lidge-jun#4 invalid_token (and invalid_grant/revoked/revoked_token) are now terminal NousTokenError values that drive re-authentication. - lidge-jun#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. - lidge-jun#6 Live /models test accepts both the OpenAI-style { data: [...] } body and a bare array (production contract). - lidge-jun#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. - lidge-jun#8 pollForToken parses the response body once and passes the payload through to the error path instead of re-reading a consumed body. - lidge-jun#9 sleep() now removes its abort listener on both resolve and abort, so polling iterations do not accumulate listeners. - lidge-jun#1 The live test is now non-destructive: it persists the rotated token back through mergeAccountCredential (prod path), so the local session stays valid. - lidge-jun#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.
|
@Wibias — thank you, this is a thorough review. All ten points are now addressed on #1 — live test was destructive. Fixed and made non-destructive: #2 — single-use refresh not failure-atomic. Now handled with a durable refresh-intent file. Keyed by #3 — redirects on credential-bearing requests. #4 — #5 — validate returned scope. #6 — #7 — #8 — body read twice. #9 — abort listeners accumulate. #10 — Russian docs. On the CI status you flagged
Verification (this head)
Kept draft, no |
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/providers.md`:
- Line 116: Synchronize the OAuth provider lists: in
docs-site/src/content/docs/guides/providers.md:116, add Nous Portal to the
authMode: oauth “Used by” list; in
docs-site/src/content/docs/ja/guides/providers.md:55,
docs-site/src/content/docs/ko/guides/providers.md:54, and
docs-site/src/content/docs/zh-cn/guides/providers.md:51, add GitHub Copilot.
Keep all English and translated OAuth provider lists consistent.
In `@src/oauth/index.ts`:
- Line 451: Update the NousTokenError handling in the OAuth error classification
to honor error.terminal, returning the terminal status directly instead of
limiting terminal failures to the five listed oauthError values. Preserve the
existing OAuth error matching for non-terminal NousTokenError instances and the
surrounding classification behavior.
In `@src/oauth/nous.ts`:
- Line 1: Unify Nous OAuth terminality by updating the dispatcher’s classifier
to honor error.terminal alongside its existing oauthError allowlist, while
retaining tokenErrorFromPayload as the sole classifier. Mark both expired_token
and access_denied NousTokenError constructions at the device-flow throw sites
with terminal: true, and add regression coverage in the Nous OAuth tests
verifying invalid_token and access_denied are classified as terminal.
- Around line 167-178: The embedded-credentials validation branch in the Nous
Portal base URL parser must not include raw in its NousTokenError message.
Replace the interpolated URL with a credential-safe value or generic message
while preserving the existing rejection behavior; leave the HTTPS, query-string,
and fragment validation messages unchanged.
- Around line 378-385: Update the successful device-authorization response
parsing in the surrounding request flow to catch JSON parse failures and use an
empty object fallback, matching the guarded error path and pollForToken
behavior. Preserve the existing required-field validation and its “Nous Portal
device authorization response missing required fields” error for empty or
non-JSON success bodies.
- Around line 111-120: Update writeRefreshIntent to create the refresh-intent
directory with owner-only permissions and write each intent file with
restrictive owner-only permissions, matching the protection used by the auth
store. Preserve the existing best-effort error handling and refresh behavior.
- Around line 281-288: Update the shared classifier in `classifyRefreshError`
(the `NousTokenError` branch in `src/oauth/index.ts`) to honor the
provider-computed `terminal` flag, while preserving existing OAuth-error
allowlist behavior where needed. Add a regression test verifying that an
`invalid_token` `NousTokenError` is classified as terminal by the shared
dispatcher.
- Around line 199-229: Reconcile the opaque-token behavior in
identityFromNousTokens and the downstream scope validation: either remove the
docstring’s claim that opaque tokens still work, preserving the hard JWT scope
gate, or update the validation around jwtGrantsInference/parseTokenPayload so
scope is enforced only when decodeJwtPayload returns a payload. Keep the
implementation and documentation consistent.
- Around line 231-248: Update NousTokenError so the credentials field is defined
as non-enumerable while remaining readable through err.credentials. Preserve the
existing optional OAuthCredentials value and constructor behavior, and leave
oauthError enumerable.
- Around line 436-437: Update the NousTokenError construction in the
expired_token and access_denied branches of the device authorization flow to
pass terminal: true. Also add access_denied to the terminal-error allowlist in
the relevant oauth index classification so both permanent outcomes are
consistently treated as terminal.
- Around line 502-526: Update the refresh-intent flow around the token request
and persistence boundary: keep the intent non-clearable after successful
parsing, export a nousCommitRefreshRotation function for the persistence path to
call only after durable credential storage succeeds, and remove the premature
clearRefreshIntent call from the response handler. Treat AbortSignal timeout or
aborted-request failures as uncertain by writing the intent state accordingly,
while preserving pending for failures known not to reach the server. Add a
regression test covering persistence failure after successful rotation and
asserting nousRefreshIntentIsUncertain(oldToken) remains true.
In `@tests/nous-oauth-live.test.ts`:
- Around line 45-57: Replace the direct refreshNousToken and
mergeAccountCredential sequence in the live test with the production
refresh-and-persist coordinator, preserving generation-aware single-use
coordination and persistence. If no coordinator is test-accessible, acquire the
same account lock, capture the credential generation before refreshing, and pass
it as expectedGeneration when calling mergeAccountCredential.
In `@tests/nous-oauth.test.ts`:
- Around line 501-512: Make the replay-guard test observe that the second
refresh attempt does not call fetch: replace the second fetch mock with a spy or
mock that would fail if invoked, then assert the request is rejected by the
uncertain-intent guard without relying on the server’s refresh_token_reused
response. Rename the test to describe the non-JSON token-payload parse-failure
scenario rather than a crash-after-rotation case, while preserving the
assertions that the intent becomes uncertain and the retry is refused.
- Around line 48-57: Isolate OPENCODEX_HOME in the describe block’s
beforeEach/afterEach around refreshNousToken, matching the setup and cleanup
used by the other refreshNousToken describe blocks. Set it to a temporary
test-specific directory before each test and restore or remove the previous
value afterward, alongside the existing NOUS_PORTAL_BASE_URL handling.
- Around line 288-309: Add a focused test beside the existing
NOUS_PORTAL_BASE_URL validation tests that sets an override containing a
non-root path, invokes refreshNousToken, and verifies the request uses only the
URL origin without that path. Preserve the existing rejection assertions for
credentials, query, and fragment overrides, and ensure the test confirms the
normalized endpoint behavior rather than merely successful execution.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 05c2cba8-29f5-42b0-b610-974445a2d71c
📒 Files selected for processing (11)
docs-site/src/content/docs/guides/providers.mddocs-site/src/content/docs/ja/guides/providers.mddocs-site/src/content/docs/ko/guides/providers.mddocs-site/src/content/docs/ru/guides/providers.mddocs-site/src/content/docs/zh-cn/guides/providers.mdsrc/oauth/index.tssrc/oauth/nous.tssrc/providers/registry.tstests/nous-oauth-live.test.tstests/nous-oauth.test.tstests/provider-registry-parity.test.ts
|
Follow-up on #2 (single-use refresh atomicity) — I dug deeper with a real execution probe (not just mocks) and found the first cut still had a hole: after a successful rotation it cleared the intent, so if the rotated token was obtained but lost before the store persisted it, the next replay of the old token was refused only by the server, not by the guard. That is exactly the "uncertain outcome" case you flagged. Hardened the contract on
Proven by a real run (not a mock): 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 added/updated in
Targeted suite: 195/195 pass; |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes again on the current head 97f89ac4ace9cb72d95b962bfb9d49648cce49d6.
Several earlier findings were fixed correctly, but not all required changes are resolved yet, and the remaining issues are still concentrated in the single-use refresh-token safety path.
Merge blockers:
- The new durable refresh-intent guard still fails open.
writeRefreshIntent()swallows persistence failures and refresh proceeds anyway;readRefreshIntent()also treats unreadable/corrupt state as absent. For a mechanism whose purpose is preventing replay of a possibly consumed single-use token, inability to durably create/read the intent must fail closed, not silently disable the guard. Reuse the repository's existing hardened OAuth refresh-intent machinery if possible. - Ambiguous fetch failures clear the intent and make the old token replayable. A timeout/abort/connection failure does not prove the Portal never received and rotated the token. Once dispatch may have occurred, the outcome must remain uncertain and the submitted token must not be replayed.
- The post-persist cleanup is wired into the wrong coordinator.
clearNousRefreshIntent(candidate.refresh)was added torefreshXaiAccountWithLock(), but Nous routes throughrefreshGenericAccountWithLock(). Successful Nous refreshes therefore persist the rotated credential without clearing their old-token intent, while xAI unnecessarily calls a Nous-specific cleanup hook. Clear the Nous intent only after durable Nous persistence succeeds on the actual production path.
Still-required correctness/security fixes:
- The shared terminal classifier still ignores
NousTokenError.terminal, so provider-classified terminal failures such asinvalid_token/insufficient_scopecan remain retryable instead of moving the account to re-authentication. - The opt-in live test still refreshes and persists outside the production generation-aware/account-lock coordinator, which is unsafe for a single-use token if another process refreshes concurrently.
- The first normal refresh-wiring test calls
refreshNousToken()without isolatingOPENCODEX_HOME, so it can leave durable intent state in the developer/runner config tree and become non-repeatable. - Embedded-credential URL validation currently echoes the raw credential-bearing URL in the thrown error.
NousTokenError.credentialsstores live access/refresh credentials as an enumerable Error property, which is unsafe for structured logging/serialization.- Device-flow
access_denied/expired_tokenterminality and the shared-classifier regression coverage are still incomplete. - The replay-guard test should prove
fetchis not called, rather than returning the samerefresh_token_reusederror shape from the mocked server. - Provider docs still have OAuth-list inconsistencies across English/translated pages.
Please also rebase/refresh onto current dev and obtain green exact-head CI after these fixes. This PR should remain draft until the single-use rotation/recovery contract is fail-closed end-to-end and all valid unresolved review findings are addressed.
…sifier 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.
|
@Wibias — you were right, and these were real defects in my code (not the PR scope or the unrelated Merge blockers
Still-required fixes
The remaining Verification
Ready for another look. |
de6e549 to
0d4b308
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-site/src/content/docs/guides/providers.md (1)
116-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSynchronize the translated Nous provider documentation
- Add the
ocx login nouscommand and the complete Nous row todocs-site/src/content/docs/ja/guides/providers.md,docs-site/src/content/docs/ko/guides/providers.md, anddocs-site/src/content/docs/zh-cn/guides/providers.md. These pages currently omit the provider while listing Nous as an OAuth provider.- Add the terminal refresh-failure instruction,
ocx login nous, to all four translated pages. The Russian page has the provider row but omits this instruction.- Include the endpoint,
openai-chat, device-grant login, per-request inference JWTs, live paid and:freediscovery, and single-use rotating refresh tokens. Preserve the existing account-pool and credential-privacy guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-site/src/content/docs/guides/providers.md` around lines 116 - 123, Synchronize the Nous documentation across the Japanese, Korean, Simplified Chinese, and Russian provider guides: add the complete Nous row where missing and ensure each page includes the terminal refresh-failure instruction `ocx login nous`. Match the source row’s endpoint, `openai-chat` type, device-grant login, per-request inference JWTs, live paid/`:free` discovery, and rotating single-use refresh-token details while preserving existing account-pool and credential-privacy guidance.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs-site/src/content/docs/guides/providers.md`:
- Around line 116-123: Synchronize the Nous documentation across the Japanese,
Korean, Simplified Chinese, and Russian provider guides: add the complete Nous
row where missing and ensure each page includes the terminal refresh-failure
instruction `ocx login nous`. Match the source row’s endpoint, `openai-chat`
type, device-grant login, per-request inference JWTs, live paid/`:free`
discovery, and rotating single-use refresh-token details while preserving
existing account-pool and credential-privacy guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c4660f78-c4c1-45a4-9af4-98f4d55096b0
📒 Files selected for processing (6)
docs-site/src/content/docs/guides/providers.mdsrc/oauth/index.tssrc/providers/registry.tstests/nous-oauth-live.test.tstests/nous-oauth.test.tstests/oauth-refresh.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-site/src/content/docs/guides/providers.md (1)
116-123: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the missing Nous details to the translated provider guides.
docs-site/src/content/docs/ja/guides/providers.md,docs-site/src/content/docs/ko/guides/providers.md, anddocs-site/src/content/docs/zh-cn/guides/providers.mdlist Nous Portal as OAuth but omitocx login nous, its provider row, endpoint, device-grant flow, live paid/:freediscovery, and rotating refresh-token behavior.docs-site/src/content/docs/ru/guides/providers.md:105-120contains these details but omits the terminal recovery instruction. Add the equivalent ofAfter a terminal Nous refresh failure, run ocx login nous to reauthenticateto all four translated pages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-site/src/content/docs/guides/providers.md` around lines 116 - 123, Update the Nous provider sections in the translated guides for Japanese, Korean, Chinese, and Russian to include the complete provider row details: the ocx login nous command, endpoint, device-grant authentication flow, live paid and :free model discovery, and rotating single-use refresh tokens. Also add the terminal refresh-failure recovery instruction to all four pages, reusing the existing translated wording conventions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/nous-oauth-live.test.ts`:
- Around line 94-98: Update the ID filtering in the models flatMap callback to
trim string IDs and accept them only when the trimmed value is non-empty. Return
the trimmed ID so the subsequent model-count assertion counts only usable model
identifiers.
---
Outside diff comments:
In `@docs-site/src/content/docs/guides/providers.md`:
- Around line 116-123: Update the Nous provider sections in the translated
guides for Japanese, Korean, Chinese, and Russian to include the complete
provider row details: the ocx login nous command, endpoint, device-grant
authentication flow, live paid and :free model discovery, and rotating
single-use refresh tokens. Also add the terminal refresh-failure recovery
instruction to all four pages, reusing the existing translated wording
conventions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f9733227-3576-43ba-b0d4-178b8f24b243
📒 Files selected for processing (6)
docs-site/src/content/docs/guides/providers.mdsrc/oauth/index.tssrc/providers/registry.tstests/nous-oauth-live.test.tstests/nous-oauth.test.tstests/oauth-refresh.test.ts
…ice grant + free/paid live catalog (Closes lidge-jun#1148)
…hy3, laguna-s/xs, step-3.7-flash)
…lback - 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)
…on; 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.
…, redirect guard Addresses the 10 review points from Wibias on PR lidge-jun#1397: - lidge-jun#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. - lidge-jun#3 Credential-bearing OAuth requests (device + token) now pass redirect: 'error' so custom auth headers cannot follow a cross-origin redirect. - lidge-jun#4 invalid_token (and invalid_grant/revoked/revoked_token) are now terminal NousTokenError values that drive re-authentication. - lidge-jun#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. - lidge-jun#6 Live /models test accepts both the OpenAI-style { data: [...] } body and a bare array (production contract). - lidge-jun#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. - lidge-jun#8 pollForToken parses the response body once and passes the payload through to the error path instead of re-reading a consumed body. - lidge-jun#9 sleep() now removes its abort listener on both resolve and abort, so polling iterations do not accumulate listeners. - lidge-jun#1 The live test is now non-destructive: it persists the rotated token back through mergeAccountCredential (prod path), so the local session stays valid. - lidge-jun#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.
…fresh 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.
…sifier 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.
…re, 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.
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.
… 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).
…e-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).
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.
4d490e2 to
2a77b66
Compare
…im 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.
|
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
Rate Limit Exceeded
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/ja/guides/providers.md`:
- Line 112: Make both Kiro installation pipeline commands table-safe by
replacing literal pipe characters with table-safe markup such as a raw code
element using &`#124`; in
docs-site/src/content/docs/ja/guides/providers.md:112-112,
docs-site/src/content/docs/ko/guides/providers.md:111-111,
docs-site/src/content/docs/ru/guides/providers.md:121-121, and
docs-site/src/content/docs/zh-cn/guides/providers.md:102-102.
In `@tests/nous-oauth-live.test.ts`:
- Line 24: Update the refreshed-account assertion in the test to call
getAccountCredential with the "nous" provider and stored!.accountId!, replacing
the getCredential call so it reads the refreshed account by ID and avoids the
excess-argument type error.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a01f0201-e5c4-4fe8-aad0-093951fb4b78
📒 Files selected for processing (7)
docs-site/src/content/docs/ja/guides/providers.mddocs-site/src/content/docs/ko/guides/providers.mddocs-site/src/content/docs/ru/guides/providers.mddocs-site/src/content/docs/zh-cn/guides/providers.mdsrc/oauth/index.tstests/nous-oauth-live.test.tstests/oauth-refresh.test.ts
| | `anthropic` | `anthropic` | `https://api.anthropic.com` | Claude モデル; ライブモデル一覧は `/v1/models` から取得。 | | ||
| | `kimi` | `openai-chat` | `https://api.kimi.com/coding/v1` | Kimi K2.7/K2.6/K2.5 コーディングモデル。 | | ||
| | `nous` | `openai-chat` | `https://inference-api.nousresearch.com/v1` | Nous Research サブスクリプションゲートウェイ（Hermes Agent と同じバックエンド）。`portal.nousresearch.com` へのデバイスグラントログイン; access トークンはリクエストごとの inference JWT。有料 + `:free` モデルの混在カタログ（`tencent/hy3:free`、`stepfun/step-3.7-flash:free` など）はサインイン中のアカウントからライブ探索されます。Refresh トークンは単回使用で、更新のたびにローテーションされます。 | | ||
| | `kiro` | `kiro` | `https://runtime.us-east-1.kiro.dev` | 初回ログインは、インストール済みでサインインした `kiro-cli` セッションを取り込みます（Unix では `curl -fsSL https://cli.kiro.dev/install | bash`、Windows PowerShell では `irm 'https://cli.kiro.dev/install.ps1' | iex` でインストールしてから `kiro-cli login` を実行）。**アカウントを追加**は `kiro-cli` をログアウトして新しいブラウザログインを開始し、`kiro-cli` 自体のアカウントを切り替えてアカウント別プロファイルメタデータを保存します。既存の OpenCodex アカウントは保持され、キャンセルまたは失敗時には以前の `kiro-cli` セッションが復元されます。 | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix raw pipeline characters in localized provider tables.
Each Kiro row contains raw | characters inside inline code. The table parser reports six cells instead of four, so it can truncate or misrender the row. Replace the inline pipeline commands with table-safe markup, such as a raw <code> element using |, or restructure the commands so no literal pipe appears in the table cell.
docs-site/src/content/docs/ja/guides/providers.md#L112-L112: Make both Kiro installation pipeline commands table-safe.docs-site/src/content/docs/ko/guides/providers.md#L111-L111: Make both Kiro installation pipeline commands table-safe.docs-site/src/content/docs/ru/guides/providers.md#L121-L121: Make both Kiro installation pipeline commands table-safe.docs-site/src/content/docs/zh-cn/guides/providers.md#L102-L102: Make both Kiro installation pipeline commands table-safe.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 112-112: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 112-112: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 112-112: Table column count
Expected: 4; Actual: 6; Too many cells, extra data will be missing
(MD056, table-column-count)
📍 Affects 4 files
docs-site/src/content/docs/ja/guides/providers.md#L112-L112(this comment)docs-site/src/content/docs/ko/guides/providers.md#L111-L111docs-site/src/content/docs/ru/guides/providers.md#L121-L121docs-site/src/content/docs/zh-cn/guides/providers.md#L102-L102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/ja/guides/providers.md` at line 112, Make both
Kiro installation pipeline commands table-safe by replacing literal pipe
characters with table-safe markup such as a raw code element using &`#124`; in
docs-site/src/content/docs/ja/guides/providers.md:112-112,
docs-site/src/content/docs/ko/guides/providers.md:111-111,
docs-site/src/content/docs/ru/guides/providers.md:121-121, and
docs-site/src/content/docs/zh-cn/guides/providers.md:102-102.
Source: Linters/SAST tools
| * accepting either an OpenAI-style `{ data: [...] }` body or a bare array. | ||
| */ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { getCredential } from "../src/oauth/store"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Store accessor declarations =="
rg -n -C 3 'export function getCredential|export function getAccountCredential' src/oauth/store.ts
echo "== Test TypeScript configuration =="
fd -t f '^(tsconfig.*\.json|bunfig\.toml)$' . -x sh -c 'echo "== $1 =="; cat "$1"' sh {}Repository: lidge-jun/opencodex
Length of output: 3705
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Test imports and refresh assertion =="
sed -n '1,95p' tests/nous-oauth-live.test.ts
echo "== Credential accessor usages =="
rg -n -C 2 'getCredential|getAccountCredential' tests src
echo "== Stored account type and accountId optionality =="
rg -n -C 5 'accountId|interface.*Account|type.*Account|StoredAccount|OAuthAccount' src/oauth/store.ts tests/nous-oauth-live.test.tsRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Credential type =="
rg -n -C 8 'interface OAuthCredentials|type OAuthCredentials' src/oauth/types.ts src/oauth/store.ts
echo "== Relevant live-test lines =="
sed -n '46,78p' tests/nous-oauth-live.test.ts
echo "== Test and type-check scripts =="
rg -n -C 3 '"(test|typecheck|check|lint)[^"]*":' package.jsonRepository: lidge-jun/opencodex
Length of output: 3704
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
store = Path("src/oauth/store.ts").read_text()
test = Path("tests/nous-oauth-live.test.ts").read_text()
sig = re.search(
r'export function getCredential\s*\(([^)]*)\)\s*:\s*([^ {]+)',
store,
)
call = re.search(r'getCredential\("nous",\s*stored!\.accountId\)', test)
print("getCredential signature:", sig.group(0) if sig else "not found")
print("two-argument call found:", bool(call))
if sig:
params = [p.strip() for p in sig.group(1).split(",") if p.strip()]
print("declared parameter count:", len(params))
PY
node - <<'JS'
function getCredential(provider) {
return { provider, accountId: "active-account" };
}
console.log("runtime result:", getCredential("nous", "requested-account"));
console.log("runtime extra argument ignored:", getCredential.length === 1);
JSRepository: lidge-jun/opencodex
Length of output: 409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Generic account refresh implementation =="
sed -n '330,430p' src/oauth/index.ts
echo "== Account persistence and active-account handling =="
sed -n '560,625p' src/oauth/index.ts
sed -n '430,475p' src/oauth/store.ts
sed -n '580,620p' src/oauth/store.ts
echo "== Test runner configuration =="
sed -n '1,180p' scripts/test.tsRepository: lidge-jun/opencodex
Length of output: 16730
Read the refreshed account by ID.
At tests/nous-oauth-live.test.ts:72, getCredential accepts only provider; the second argument is ignored. Use getAccountCredential("nous", stored!.accountId!) to assert the account passed to refreshGenericAccountWithLock. This also avoids an excess-argument TypeScript diagnostic when tests are type-checked.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/nous-oauth-live.test.ts` at line 24, Update the refreshed-account
assertion in the test to call getAccountCredential with the "nous" provider and
stored!.accountId!, replacing the getCredential call so it reads the refreshed
account by ID and avoids the excess-argument type error.
Blockers from this review were addressed in later commits (HTTPS base-URL validation, single-use refresh rotation without consumed-token fallback, docs/tests). Merging with maintainer sponsorship.
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Approving after the earlier change requests: fail-closed refresh-intent, HTTPS-only portal base URL, single-use rotation without consumed-token fallback, redirect rejection, terminal error classification, and live-test safety are in place. CI is green on the current head.
|
Thanks @Cheurteenyt — this was useful because it lands a full first-class Nous Portal OAuth path (device grant, single-use rotating refresh, live free/paid catalog) that matches how Hermes talks to the same backend, so users get Merging now. |
…ive-test account read (follow-up to #1397) (#1450) * docs(providers): table-safe Kiro install pipes in translations; fix live-test account read - ja/ko/ru/zh-cn kiro rows: replace literal pipe characters in the Kiro CLI install pipelines with table-safe | entities so the markdown tables render correctly. - nous-oauth-live.test.ts: read the refreshed account with getAccountCredential('nous', accountId) instead of passing an excess accountId argument to getCredential (fixes the type error). * fix(oauth/nous): address CodeRabbit + review findings on the follow-up - preserveNousRotatedRefresh returns the exact generation it wrote, removing the post-merge getAccountCredential re-read so a concurrent writer cannot be marked needsReauth (TOCTOU). Rethrow OAuthMutationBusyError unchanged so the caller can retry. - parseTokenPayload: a missing access_token is now a terminal NousTokenError (invalid_token) instead of a plain Error. - Live test: read the store row id via getAccountSet(...).activeAccountId and pass rowId! to both refreshGenericAccountWithLock and getAccountCredential (the row id is a SHA-256-derived hash, not the JWT sub). - Docs: escape the Kiro install pipes (|) in the English providers table too, and add command-code to the oauth 'Used by' list across all five locales. - Regression: persisted-branch TOCTOU test (concurrent writer not marked needsReauth). * fix(oauth/nous): code/robustness fixes from CodeRabbit outside-diff review - jwtExpiryMs: only accept an exp within a plausible now-relative window, falling back to expires_in for out-of-range claims; clamp skew-adjusted expiry to non-negative. Add a dedicated DEFAULT_ACCESS_TOKEN_TTL_MS so the device-flow window is not reused as the access-token fallback lifetime. - pollForToken: tolerate transient transport errors (timeout/DNS/connection) until the device deadline; only genuine cancellation aborts early. - writeRefreshIntent: re-apply owner-only 0o700 on an existing intent dir. - resolvePortalBaseUrl docstring: stop claiming Hermes host-allowlist parity. - parseTokenPayload: device-login missing refresh_token is invalid_token, not refresh_token_reused; missing access_token is now a terminal error. - index.ts: anchor nous defaultRefreshPolicy explicitly to lazy-only. - Live test: import NOUS_INFERENCE_BASE_URL instead of the hard-coded URL; use a structural refresh-only def type instead of the non-exported OAuthProviderDef. - Regressions: implausible exp falls back to expires_in; device-login missing refresh_token is invalid_token. * fix(oauth/nous): fail closed on refresh-intent hardening, honor device-flow deadline, classify missing access_token as terminal * docs(providers): render Kiro install pipes as visible | in all locale tables A bare | entity inside a code span is emitted literally by Astro's markdown processor, so the Kiro CLI install commands showed the escaped text instead of a pipe. Move the pipe out of the code span so it renders as a visible | between the two command fragments, keeping the markdown table intact across all five locales. * fix(oauth/nous): enforce device-flow deadline on every poll path and normalize null token bodies CodeRabbit follow-up on the deadline cap: authorization_pending and slow_down still slept the full interval and a delayed success response could return credentials after the deadline. Route every retry through a deadline-aware sleep helper and recheck the deadline after each fetch. Also normalize a valid-JSON null response body to an empty object so a successful-but-null payload raises the terminal invalid_token NousTokenError instead of a raw TypeError. Add a regression test for the null-body case.
3 participants
Closes #1148
What
Adds Nous Portal (Nous Research) as a first-class OAuth provider, matching the device-grant flow Hermes Agent uses against the same backend.
OAuth flow (RFC 8628 device authorization grant)
POST https://portal.nousresearch.com/api/oauth/device/code(client_id=hermes-cli,scope=inference:invoke) →user_code+ verification URL surfaced via the controller (onAuth), same UX as Kimi/Kiro.POST .../api/oauth/tokenwithgrant_type=urn:ietf:params:oauth:grant-type:device_code, handlingauthorization_pending,slow_down(backoff),access_denied,expired_token.inference:invoke) → used directly asAuthorization: Beareragainst the OpenAI-compatible endpointhttps://inference-api.nousresearch.com/v1(adapter: openai-chat).Refresh (single-use rotation)
x-nous-refresh-tokenheader (not the body) withgrant_type=refresh_token+client_id.refresh_token_reused). The refresh path persists the rotated token immediately and stays on the default lazy-only refresh policy — no proactive background refresh for this provider.Registry & catalog
nousregistry entry: featured,freeTier: false(free tier is per-model via the:freeslugs, not provider-wide),liveModels: truewith discovery on/v1/models(max 512 models). Catalog is a mix of paid models and:freeslugs; free-tier gating is decided live by the Portal per account, with a static fallback seed for the logged-out state (see below).NousTokenErrormapped to terminal refresh errors (invalid_grant,refresh_token_reused,revoked,revoked_token,expired_token).Free model seed (verified against the live Portal list, 2026-08-10)
The registry ships a static fallback seed with the 4
:freemodels currently advertised by the Portal — confirmed against the public endpoint Hermes Agent uses (https://portal.nousresearch.com/api/nous/recommended-models):tencent/hy3:freepoolside/laguna-s-2.1:freestepfun/step-3.7-flash:freepoolside/laguna-xs-2.1:freeNote:
inclusionai/ling-3.0-flash:freewas removed from the Portal's free list (404 on the inference API since 2026-08-07) and is therefore not seeded.Paid catalog value
Beyond the free tier, the Nous Portal paid catalog is significant. Nous Research's own announcements:
With
liveModels: truediscovery, all paid models (including the discounted DeepSeek V4 Flash 0731) show up automatically once a Portal account is connected.Multiauth
sub→ accountId, lowercasedemailwhen present); multiple Portal accounts are stored/upserted persublike other OAuth providers.Tests & docs
tests/nous-oauth.test.ts(41 tests): JWT identity, refresh header/rotation wiring, mocked device-grant login, multiauth (append/upsert), refresh-intent schema validation (fail-closed), HTTP failure-atomicity classification, and terminal-error contracts. All network calls mocked viaNOUS_PORTAL_BASE_URL— no real login performed.tests/provider-registry-parity.test.ts(featured set + freeTier list) and provider docs (en/ja/ko/ru/zh-cn).Verification
bun run typecheck✅bun run test(targeted OAuth/provider suites): 292 pass, 1 skip (opt-in live), 0 fail — includingnous-oauth.test.ts,oauth-refresh.test.ts,oauth-provider-reconcile.test.ts,catalog-oauth-observation.test.ts,provider-registry-parity.test.ts,oauth-public-surface.test.ts,oauth-store-multi.test.ts,oauth-status-privacy.test.ts,oauth-health.test.ts,repo-hygiene.test.tsand the other OAuth/catalog suites. ✅bun run privacy:scan✅Note: no live Nous login was executed during development (credentials/OAuth state untouched); the flow is verified against Hermes Agent's
hermes_cli/auth.pyimplementation and mocked responses.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
ocx login nouscommand.Bug Fixes
Documentation