docs(providers): table-safe Kiro install pipes in translations; fix live-test account read (follow-up to #1397) - #1450
Conversation
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
documentation
|
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:
📝 WalkthroughWalkthroughHardened Nous Portal OAuth token validation, device polling, refresh-token rotation recovery, and account-scoped verification. Escaped Kiro command pipe characters in five localized provider guides. ChangesNous Portal OAuth
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthLogin
participant NousPortal
participant OAuthRefresh
participant CredentialStore
OAuthLogin->>NousPortal: poll for device token
NousPortal-->>OAuthLogin: return credentials
OAuthRefresh->>CredentialStore: persist refresh intent
OAuthRefresh->>NousPortal: submit refresh token
NousPortal-->>OAuthRefresh: return rotated credentials
OAuthRefresh->>CredentialStore: commit exact credential generation
OAuthRefresh->>CredentialStore: clean up refresh intent
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
…ive-test account read - ja/ko/ru/zh-cn kiro rows: replace literal pipe characters in the Kiro CLI install pipelines with table-safe &lidge-jun#124; 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).
9d78f2d to
25d462f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 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 117: Update the Kiro table row to replace its raw pipe characters inside
inline commands or URLs with the table-safe entity used by the localized
provider rows, while preserving the displayed installation instructions and
table column structure.
- Line 60: Update the OAuth provider inventory to include command-code in
docs-site/src/content/docs/guides/providers.md:60,
docs-site/src/content/docs/ja/guides/providers.md:55,
docs-site/src/content/docs/ko/guides/providers.md:54,
docs-site/src/content/docs/ru/guides/providers.md:63, and
docs-site/src/content/docs/zh-cn/guides/providers.md:51. Do not add chatgpt,
since its configuration uses forward authentication.
In `@src/oauth/index.ts`:
- Around line 631-641: Remove the inline provider === "nous" branches from
refreshGenericAccountWithLock and move both behaviors onto OAuthProviderDef as
optional onRotationPersisted and rotatedRefreshFromError hooks. Configure the
nous provider definition with clearNousRefreshIntent and NousTokenError
rotated-refresh extraction, then have the generic coordinator invoke these hooks
while preserving existing behavior for providers without them.
- Around line 494-510: Update the recovery write catch in
refreshGenericAccountWithLock around mergeAccountCredential to rethrow
OAuthMutationBusyError unchanged so callers can retry. For all other errors,
record the failure with logOAuthEvent including the error as cause, then return
"failed" as before; preserve the existing persisted/superseded outcomes.
- Around line 198-208: Update the OAuth provider configuration used by Token
Guardian for the `nous` provider so its refresh policy is always lazy-only and
cannot be overridden by `config.providers.nous.refreshPolicy`. Anchor the change
in the `nous` entry and the policy resolution logic near
`providerConfig`/`refreshPolicy`, while preserving proactive refresh behavior
for other providers.
- Around line 662-677: Update preserveNousRotatedRefresh to return
credentialGeneration(recovery) after mergeAccountCredential succeeds, and use
that returned generation in the outcome === "persisted" path when calling
markAccountNeedsReauthIfGeneration. Remove the subsequent getAccountCredential
re-read and its persistedGeneration calculation so concurrent logins cannot
supply a different credential generation.
In `@src/oauth/nous.ts`:
- Around line 4-7: Update the module docstring near the Nous gateway description
in src/oauth/nous.ts:4-7 to remove inclusionai/ling-3.0-flash:free and either
use a currently seeded slug such as poolside/laguna-s-2.1:free or point readers
to src/providers/registry.ts. In src/providers/registry.ts:1062-1076, retain the
removal note and explicitly state that this seed list is the sole enumeration of
fallback slugs.
- Around line 399-410: Update parseTokenPayload so the missing access_token
branch throws a terminal NousTokenError, matching the existing missing
refresh_token and other unusable-response branches. Preserve the current error
message/context while ensuring the error is classified as terminal by the
coordinator.
- Around line 546-554: Add an inline comment at the onAuth payload in loginNous
clarifying that deviceCode intentionally contains device.userCode for display,
while the RFC 8628 device code is secret and must not be exposed. Preserve the
existing assignment and polling behavior.
- Around line 513-517: Update parseTokenPayload to distinguish device-login
responses from refresh-token rotation using the available submittedRefreshToken
value. When no refresh token is submitted, report the missing refresh_token with
an initial-login-appropriate message and OAuth error instead of
refresh_token_reused; preserve the existing rotation error behavior when
submittedRefreshToken is present, and update the caller to pass that value
through.
- Around line 226-255: Update the comment above resolvePortalBaseUrl to remove
the inaccurate claim that it mirrors Hermes’s host allowlist, and describe only
the validations this function actually performs: HTTPS, no embedded credentials,
query strings, or fragments, while returning the URL origin.
- Around line 104-199: Update writeRefreshIntent to prune stale intent files
after ensuring refreshIntentDir exists, using readdirSync and statSync to remove
files whose modification time exceeds the chosen maximum plausible refresh-token
lifetime; skip entries that cannot be inspected or removed. Keep
readRefreshIntent fail-closed and do not use age to allow replay or bypass the
existing guard.
In `@src/providers/registry.ts`:
- Around line 1092-1099: Update the user-facing note in the Nous Research
registry configuration to remove the dated fallback model list, since the
authoritative fallback entries remain in the models array and live discovery
supplies the catalog when available. Keep the note focused on the subscription
gateway and OAuth device login details without duplicating model slugs or dates.
- Around line 1077-1091: Update providerMatchesRegistryTransport() to reject
OAuth registry entries when the matching provider’s authMode is not "oauth",
preventing pre-existing key-auth providers such as "nous" from being retargeted.
Preserve the existing OAuth matching behavior for providers configured with
authMode "oauth" and keep them pinned to the registry endpoint.
In `@tests/nous-oauth-live.test.ts`:
- Around line 61-75: Update the Nous refresh test to use the store row ID rather
than the JWT account ID: import and call getAccountSet("nous"), assign its
activeAccountId to rowId, and assert that rowId exists. Pass rowId! to both
refreshGenericAccountWithLock and getAccountCredential, while preserving the
existing token-rotation assertions.
- Around line 42-51: Update the provider typing used by NOUS_DEF and
refreshGenericAccountWithLock so it references an exported type from
src/oauth/types.ts. Add or reuse a refresh-only provider type matching the
minimal id and refresh members, rather than importing the private full provider
definition from src/oauth/index.ts; preserve the existing two-argument expect
usage.
In `@tests/nous-oauth.test.ts`:
- Around line 10-13: Move previousOpencodexHome and previousPortalBase from
module scope into each describe block whose beforeEach/afterEach uses them,
following the existing previousHome pattern in the later blocks. Keep each
block’s environment-save state independent, and avoid sharing TEST_DIR across
concurrent or interleaved blocks by giving each describe its own temporary
directory state and path.
- Around line 271-303: Update the “Nous Portal base URL hardening” describe
block to save the ambient NOUS_PORTAL_BASE_URL value in beforeEach and restore
it in afterEach, preserving both defined and undefined states. Remove the
redundant per-test deletion of NOUS_PORTAL_BASE_URL from the affected finally
blocks, while retaining fetch cleanup and existing test behavior.
In `@tests/oauth-refresh.test.ts`:
- Around line 911-951: Add a focused regression test alongside “Nous RT-B
preservation never overwrites a newer concurrent generation” that has the
mergeAccountCredential spy commit a newer credential through the real store
write but return { superseded: false, stored: ... }, exercising the persisted
branch of getValidAccessTokenForAccount. Assert the concurrent refresh remains
stored and the newer generation is not marked needsReauth, while preserving the
existing replay-intent expectations; restore the spy in a finally block.
🪄 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: 9f2f05fe-56d2-408c-8067-2d37234f53ce
📒 Files selected for processing (12)
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/oauth-refresh.test.tstests/provider-registry-parity.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (19)
docs-site/src/content/docs/guides/providers.md (2)
60-60: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail rg -n -C 3 \ 'command-code|github-copilot|authMode.*oauth|ocx login nous' \ src/providers/registry.ts src/oauth/index.ts \ docs-site/src/content/docs/guides/providers.md \ docs-site/src/content/docs/ja/guides/providers.md \ docs-site/src/content/docs/ko/guides/providers.md \ docs-site/src/content/docs/ru/guides/providers.md \ docs-site/src/content/docs/zh-cn/guides/providers.mdRepository: lidge-jun/opencodex
Length of output: 37465
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- auth-table context ---' for f in \ docs-site/src/content/docs/guides/providers.md \ docs-site/src/content/docs/ja/guides/providers.md \ docs-site/src/content/docs/ko/guides/providers.md \ docs-site/src/content/docs/ru/guides/providers.md \ docs-site/src/content/docs/zh-cn/guides/providers.md do printf '\n### %s\n' "$f" sed -n '48,66p' "$f" done printf '%s\n' '--- parsed inventory comparison ---' python3 - <<'PY' from pathlib import Path import re registry = Path("src/providers/registry.ts").read_text() oauth = Path("src/oauth/index.ts").read_text() # Provider registry entries are object literals with an id and authKind. registry_oauth = set(re.findall( r'(?ms)^\s*{\s*\n\s*id:\s*"([^"]+)",.*?^\s*authKind:\s*"oauth",', registry, )) # OAUTH_PROVIDERS keys are the authoritative OAuth login definitions. oauth_block = oauth.split("export const OAUTH_PROVIDERS", 1)[1].split("};", 1)[0] oauth_keys = set(re.findall(r'(?m)^\s*"([^"]+)":\s*{', oauth_block)) oauth_keys |= set(re.findall(r'(?m)^\s*([a-z][a-z0-9-]*):\s*{', oauth_block)) files = [ Path("docs-site/src/content/docs/guides/providers.md"), Path("docs-site/src/content/docs/ja/guides/providers.md"), Path("docs-site/src/content/docs/ko/guides/providers.md"), Path("docs-site/src/content/docs/ru/guides/providers.md"), Path("docs-site/src/content/docs/zh-cn/guides/providers.md"), ] print("registry authKind=oauth:", sorted(registry_oauth)) print("OAUTH_PROVIDERS keys:", sorted(oauth_keys)) print("registry minus OAUTH_PROVIDERS:", sorted(registry_oauth - oauth_keys)) print("OAUTH_PROVIDERS minus registry:", sorted(oauth_keys - registry_oauth)) for path in files: rows = [] in_auth_table = False for number, line in enumerate(path.read_text().splitlines(), 1): if "Authentication" in line or "認証" in line or "인증" in line or "Аутентификация" in line or "认证" in line: in_auth_table = True if in_auth_table and re.match(r"^\| `[^`]+` \|", line): provider = re.match(r"^\| `([^`]+)` \|", line).group(1) auth = line.split("|")[2].strip() rows.append((provider, auth, number)) if in_auth_table and rows and line.strip() == "": break print(path, "table rows:", rows) PYRepository: lidge-jun/opencodex
Length of output: 6992
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- auth-table context ---' for f in \ docs-site/src/content/docs/guides/providers.md \ docs-site/src/content/docs/ja/guides/providers.md \ docs-site/src/content/docs/ko/guides/providers.md \ docs-site/src/content/docs/ru/guides/providers.md \ docs-site/src/content/docs/zh-cn/guides/providers.md do printf '\n### %s\n' "$f" sed -n '48,66p' "$f" done printf '%s\n' '--- parsed inventory comparison ---' python3 - <<'PY' from pathlib import Path import re registry = Path("src/providers/registry.ts").read_text() oauth = Path("src/oauth/index.ts").read_text() registry_oauth = set(re.findall( r'(?ms)^\s*{\s*\n\s*id:\s*"([^"]+)",.*?^\s*authKind:\s*"oauth",', registry, )) oauth_block = oauth.split("export const OAUTH_PROVIDERS", 1)[1].split("};", 1)[0] oauth_keys = set(re.findall(r'(?m)^\s*"([^"]+)":\s*{', oauth_block)) oauth_keys |= set(re.findall(r'(?m)^\s*([a-z][a-z0-9-]*):\s*{', oauth_block)) files = [ Path("docs-site/src/content/docs/guides/providers.md"), Path("docs-site/src/content/docs/ja/guides/providers.md"), Path("docs-site/src/content/docs/ko/guides/providers.md"), Path("docs-site/src/content/docs/ru/guides/providers.md"), Path("docs-site/src/content/docs/zh-cn/guides/providers.md"), ] print("registry authKind=oauth:", sorted(registry_oauth)) print("OAUTH_PROVIDERS keys:", sorted(oauth_keys)) print("registry minus OAUTH_PROVIDERS:", sorted(registry_oauth - oauth_keys)) print("OAUTH_PROVIDERS minus registry:", sorted(oauth_keys - registry_oauth)) for path in files: rows = [] in_auth_table = False for number, line in enumerate(path.read_text().splitlines(), 1): if any(term in line for term in ( "Authentication", "認証", "인증", "Аутентификация", "认证" )): in_auth_table = True if in_auth_table and re.match(r"^\| `[^`]+` \|", line): provider = re.match(r"^\| `([^`]+)` \|", line).group(1) auth = line.split("|")[2].strip() rows.append((provider, auth, number)) if in_auth_table and rows and line.strip() == "": break print(path, "table rows:", rows) PYRepository: lidge-jun/opencodex
Length of output: 6992
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- registry auth declarations ---' rg -n -A 7 -B 3 'id: "(command-code|xai|anthropic|kimi|kiro|google-antigravity|cursor|github-copilot|nous|chatgpt)"|authKind: "oauth"|authMode: "oauth"' \ src/providers/registry.ts src/providers/derive.ts src/oauth/index.ts printf '%s\n' '--- OAuth config and derivation references ---' rg -n -A 18 -B 8 'function oauthConfig|const oauthConfig|oauthConfig\(|deriveOAuthProviderConfig|OAUTH_PROVIDERS' \ src/providers/derive.ts src/oauth/index.ts src/providers/registry.ts printf '%s\n' '--- bounded registry-entry check and table comparison ---' python3 - <<'PY' from pathlib import Path import re registry = Path("src/providers/registry.ts").read_text() oauth = Path("src/oauth/index.ts").read_text() def object_after_id(source, provider_id): marker = f'id: "{provider_id}"' start = source.find(marker) if start < 0: return None begin = source.rfind("{", 0, start) depth = 0 string = None escape = False for i in range(begin, len(source)): c = source[i] if string: if escape: escape = False elif c == "\\": escape = True elif c == string: string = None elif c in ('"', "'", "`"): string = c elif c == "{": depth += 1 elif c == "}": depth -= 1 if depth == 0: return source[begin:i+1] raise RuntimeError(provider_id) ids = [ "command-code", "xai", "anthropic", "kimi", "kiro", "google-antigravity", "cursor", "github-copilot", "nous", "chatgpt", ] for provider_id in ids: entry = object_after_id(registry, provider_id) if entry is None: print(f"registry {provider_id}: MISSING") else: auth = re.search(r'\bauthKind:\s*"([^"]+)"', entry) adapter = re.search(r'\badapter:\s*"([^"]+)"', entry) print(f"registry {provider_id}: authKind={auth.group(1) if auth else '<default>'}, adapter={adapter.group(1) if adapter else '<default>'}") block = oauth.split("export const OAUTH_PROVIDERS", 1)[1] block = block.split("};", 1)[0] keys = re.findall(r'(?m)^\s*(?:"([^"]+)"|([a-z][a-z0-9-]*)):\s*{', block) oauth_keys = {a or b for a, b in keys} print("OAUTH_PROVIDERS:", sorted(oauth_keys)) for path in [ Path("docs-site/src/content/docs/guides/providers.md"), Path("docs-site/src/content/docs/ja/guides/providers.md"), Path("docs-site/src/content/docs/ko/guides/providers.md"), Path("docs-site/src/content/docs/ru/guides/providers.md"), Path("docs-site/src/content/docs/zh-cn/guides/providers.md"), ]: text = path.read_text() line = next((x for x in text.splitlines() if re.match(r'^\| `oauth` \|', x)), None) names = re.search(r'\| ([^|]+)\.\s*$', line) if line else None print(f"{path}: oauth row present={line is not None}; contains command-code={'command-code' in line if line else False}; contains chatgpt={'ChatGPT' in line or 'ChatGPT' in line if line else False}") PYRepository: lidge-jun/opencodex
Length of output: 39111
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- registry auth declarations ---' rg -n -A 7 -B 3 'id: "(command-code|xai|anthropic|kimi|kiro|google-antigravity|cursor|github-copilot|nous|chatgpt)"|authKind: "oauth"|authMode: "oauth"' \ src/providers/registry.ts src/providers/derive.ts src/oauth/index.ts printf '%s\n' '--- OAuth config and derivation references ---' rg -n -A 18 -B 8 'function oauthConfig|const oauthConfig|oauthConfig\(|deriveOAuthProviderConfig|OAUTH_PROVIDERS' \ src/providers/derive.ts src/oauth/index.ts src/providers/registry.ts printf '%s\n' '--- bounded registry-entry check and table comparison ---' python3 - <<'PY' from pathlib import Path import re registry = Path("src/providers/registry.ts").read_text() oauth = Path("src/oauth/index.ts").read_text() def object_after_id(source, provider_id): marker = f'id: "{provider_id}"' start = source.find(marker) if start < 0: return None begin = source.rfind("{", 0, start) depth = 0 string = None escape = False for i in range(begin, len(source)): c = source[i] if string: if escape: escape = False elif c == "\\": escape = True elif c == string: string = None elif c in ('"', "'", "`"): string = c elif c == "{": depth += 1 elif c == "}": depth -= 1 if depth == 0: return source[begin:i+1] raise RuntimeError(provider_id) ids = [ "command-code", "xai", "anthropic", "kimi", "kiro", "google-antigravity", "cursor", "github-copilot", "nous", "chatgpt", ] for provider_id in ids: entry = object_after_id(registry, provider_id) if entry is None: print(f"registry {provider_id}: MISSING") else: auth = re.search(r'\bauthKind:\s*"([^"]+)"', entry) adapter = re.search(r'\badapter:\s*"([^"]+)"', entry) print(f"registry {provider_id}: authKind={auth.group(1) if auth else '<default>'}, adapter={adapter.group(1) if adapter else '<default>'}") block = oauth.split("export const OAUTH_PROVIDERS", 1)[1] block = block.split("};", 1)[0] keys = re.findall(r'(?m)^\s*(?:"([^"]+)"|([a-z][a-z0-9-]*)):\s*{', block) oauth_keys = {a or b for a, b in keys} print("OAUTH_PROVIDERS:", sorted(oauth_keys)) for path in [ Path("docs-site/src/content/docs/guides/providers.md"), Path("docs-site/src/content/docs/ja/guides/providers.md"), Path("docs-site/src/content/docs/ko/guides/providers.md"), Path("docs-site/src/content/docs/ru/guides/providers.md"), Path("docs-site/src/content/docs/zh-cn/guides/providers.md"), ]: text = path.read_text() line = next((x for x in text.splitlines() if re.match(r'^\| `oauth` \|', x)), None) print(f"{path}: oauth row present={line is not None}; contains command-code={'command-code' in line if line else False}; contains chatgpt={'ChatGPT' in line if line else False}") PYRepository: lidge-jun/opencodex
Length of output: 39111
Add
command-codeto all OAuth inventories
src/providers/registry.ts:957-962definescommand-codewithauthKind: "oauth", andsrc/oauth/index.ts:168-174registers its OAuth login. Add it to the OAuth provider list atdocs-site/src/content/docs/guides/providers.md:60and the corresponding Japanese, Korean, Russian, and Simplified Chinese rows. Do not addchatgpt; its provider configuration usesauthMode: "forward"atsrc/oauth/index.ts:235-240.📍 Affects 5 files
docs-site/src/content/docs/guides/providers.md#L60-L60(this comment)docs-site/src/content/docs/ja/guides/providers.md#L55-L55docs-site/src/content/docs/ko/guides/providers.md#L54-L54docs-site/src/content/docs/ru/guides/providers.md#L63-L63docs-site/src/content/docs/zh-cn/guides/providers.md#L51-L51🤖 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` at line 60, Update the OAuth provider inventory to include command-code in docs-site/src/content/docs/guides/providers.md:60, docs-site/src/content/docs/ja/guides/providers.md:55, docs-site/src/content/docs/ko/guides/providers.md:54, docs-site/src/content/docs/ru/guides/providers.md:63, and docs-site/src/content/docs/zh-cn/guides/providers.md:51. Do not add chatgpt, since its configuration uses forward authentication.Source: Path instructions
117-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape the Kiro pipes in this table row.
Raw
|characters inside this Markdown table row are parsed as column separators. The rendered Kiro row can split into extra cells and lose part of the installation instructions. The supplied markdownlint output reports MD038 and MD056 for this line.Use the table-safe entity already used by the localized rows.
Proposed fix
-`curl -fsSL https://cli.kiro.dev/install | bash` +`curl -fsSL https://cli.kiro.dev/install &`#124`; bash` -`irm 'https://cli.kiro.dev/install.ps1' | iex` +`irm 'https://cli.kiro.dev/install.ps1' &`#124`; iex`🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 117-117: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 117-117: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 117-117: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 117-117: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 117-117: Table column count
Expected: 4; Actual: 6; Too many cells, extra data will be missing(MD056, table-column-count)
🤖 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` at line 117, Update the Kiro table row to replace its raw pipe characters inside inline commands or URLs with the table-safe entity used by the localized provider rows, while preserving the displayed installation instructions and table column structure.Sources: Path instructions, Linters/SAST tools
src/oauth/index.ts (4)
198-208: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Find any proactive/background/eager OAuth refresh scheduling that could select nous. set -uo pipefail echo "=== OAuthProviderDef shape (look for a background/proactive opt-in flag) ===" ast-grep run --pattern 'export interface OAuthProviderDef { $$$ }' --lang typescript src/oauth/types.ts echo "=== Scheduler-ish refresh triggers ===" rg -nP --type=ts -C4 '\b(setInterval|setTimeout|prewarm|preemptive|proactive|background|eager)\w*\b' src/oauth src/server* 2>/dev/null | rg -n -C4 -i 'refresh|token|oauth' || echo "no scheduler-shaped refresh triggers found" echo "=== Call sites that refresh across all providers rather than one ===" rg -nP --type=ts -C5 '(Object\.keys|Object\.entries|for\s*\(\s*const\s+\[?\w+)\s*.{0,40}(OAUTH_PROVIDERS|providers)\b' src/oauth echo "=== Every caller of the generic refresh coordinator ===" rg -nP --type=ts -C3 '\brefreshGenericAccountWithLock\s*\(' src tests echo "=== Anything that calls def.refresh directly ===" ast-grep run --pattern '$DEF.refresh($$$)' --lang typescript srcRepository: lidge-jun/opencodex
Length of output: 35187
🏁 Script executed:
#!/bin/bash set -uo pipefail echo "=== Provider policy definition and resolution ===" sed -n '130,170p;245,270p' src/oauth/index.ts echo "=== Nous registration ===" sed -n '194,212p' src/oauth/index.ts echo "=== Token guardian sweep and lifecycle ===" sed -n '1,190p;270,330p' src/oauth/token-guardian.ts echo "=== Guardian configuration and startup ===" rg -n -C5 --type=ts 'tokenGuardian|startTokenGuardian|refreshPolicy' src | head -240 echo "=== Config provider schema/default handling ===" rg -n -C6 --type=ts 'refreshPolicy|interface OcxProviderConfig|type OcxProviderConfig' src/config src/types.ts src/oauthRepository: lidge-jun/opencodex
Length of output: 27505
Prevent Token Guardian from proactively refreshing
nous
src/oauth/token-guardian.ts:143-150enumerates all OAuth providers.src/oauth/index.ts:260-264letsconfig.providers.nous.refreshPolicy: "proactive"override the lazy-only fallback. Enforce a non-overridablenousopt-out so the Token Guardian cannot refresh its single-use tokens in the background and triggerrefresh_token_reusedsession revocation.🤖 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 `@src/oauth/index.ts` around lines 198 - 208, Update the OAuth provider configuration used by Token Guardian for the `nous` provider so its refresh policy is always lazy-only and cannot be overridden by `config.providers.nous.refreshPolicy`. Anchor the change in the `nous` entry and the policy resolution logic near `providerConfig`/`refreshPolicy`, while preserving proactive refresh behavior for other providers.
494-510: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The blanket
catchmaps transient store failures to a forced re-login and logs nothing.Lines 507-509 convert every throw from
mergeAccountCredentialinto"failed". The caller at Lines 678-683 then marks the accountneedsReauthand throwsOAuthLoginRequiredError. Two problems follow.First,
OAuthMutationBusyErroris transient. The outer handler at Line 644 deliberately rethrows it so the caller can retry rather than lose the credential. This innercatchswallows it, so a merely busy mutation queue during the recovery write forces the user through a full device-grant re-login. The rotated refresh token RT-B is discarded even though the store was simply contended.Second, nothing is logged. Every other failure branch in
refreshGenericAccountWithLockemits alogOAuthEvent. When RT-B preservation fails, the operator sees onlyOAuthLoginRequiredErrorand has no signal about the underlying store error.Propagate the busy error and record the rest.
🐛 Proposed fix
const outcome = await mergeAccountCredential(provider, accountId, recovery, { expectedGeneration }); return outcome.superseded ? "superseded" : "persisted"; - } catch { + } catch (error) { + // A contended mutation queue is transient. Let the caller retry instead of + // discarding RT-B and forcing a full re-login. + if (error instanceof OAuthMutationBusyError) throw error; + logOAuthEvent("OAuth rotated refresh preservation failed", { + provider, + accountId, + cause: error instanceof Error ? error.message : String(error), + }); return "failed"; } }
OAuthMutationBusyErrorthen propagates out of theif (provider === "nous" ...)block at Lines 652-686 to the caller, matching the contract already established at Line 644. Confirm thelogOAuthEventfield filter drops nothing needed here:src/oauth/log.tsLines 35-48 skips keys rejected byisForbiddenFieldKey, andcauseis already used for this purpose at Lines 638 and 674.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try { const recovery: OAuthCredentials = { refresh: rotatedRefresh, // Never persist the unusable access token: an empty placeholder with a // past expiry can never be observed as a valid credential. access: "", expires: 0, ...(previous.accountId ? { accountId: previous.accountId } : {}), ...(previous.email ? { email: previous.email } : {}), ...(previous.source ? { source: previous.source } : {}), }; const outcome = await mergeAccountCredential(provider, accountId, recovery, { expectedGeneration }); return outcome.superseded ? "superseded" : "persisted"; } catch (error) { // A contended mutation queue is transient. Let the caller retry instead of // discarding RT-B and forcing a full re-login. if (error instanceof OAuthMutationBusyError) throw error; logOAuthEvent("OAuth rotated refresh preservation failed", { provider, accountId, cause: error instanceof Error ? error.message : String(error), }); return "failed"; }🤖 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 `@src/oauth/index.ts` around lines 494 - 510, Update the recovery write catch in refreshGenericAccountWithLock around mergeAccountCredential to rethrow OAuthMutationBusyError unchanged so callers can retry. For all other errors, record the failure with logOAuthEvent including the error as cause, then return "failed" as before; preserve the existing persisted/superseded outcomes.
631-641: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
refreshGenericAccountWithLocknow hardcodes twoprovider === "nous"branches; move the behavior onto the provider definition.Lines 631 and 652 add provider-specific logic to the shared, provider-agnostic refresh coordinator. The function already carries
xai,anthropic, andkirospecial cases through the dedicated wrappers inrefreshAndPersistAccessToken(Lines 704-706) and throughterminal(). Adding inline string comparisons inside the generic path means every future provider with rotation bookkeeping appends another branch to the same twotryblocks, and the Nous-specific recovery becomes untestable in isolation from the generic coordinator.The two behaviors are both expressible as optional hooks on
OAuthProviderDef: one that runs after a rotation is durably persisted, and one that recovers credential material from a terminal error.noussupplies both; every other provider supplies neither and the coordinator stays generic.♻️ Sketch of the hook shape
export interface OAuthProviderDef { id: string; refresh(rt: string, signal?: AbortSignal, cred?: OAuthCredentials): Promise<OAuthCredentials>; $$$ + /** Runs after a rotation is durably persisted. Failures are logged, never fatal. */ + onRotationPersisted?(consumedRefresh: string): void; + /** Extracts already-rotated refresh material from a terminal refresh error. */ + rotatedRefreshFromError?(error: unknown): string | undefined; }The coordinator then reads:
- if (provider === "nous") { - try { - clearNousRefreshIntent(stored.refresh); - } catch (cleanupErr) { + if (def.onRotationPersisted) { + try { + def.onRotationPersisted(stored.refresh); + } catch (cleanupErr) {and
- if (provider === "nous" && error instanceof NousTokenError) { - const rotated = error.getRotatedRefresh(); + const rotated = def.rotatedRefreshFromError?.(error); + {with the
nousentry at Lines 198-208 supplyingonRotationPersisted: clearNousRefreshIntentandrotatedRefreshFromError: e => e instanceof NousTokenError ? e.getRotatedRefresh() : undefined.As per path instructions for
src/**, flag "changes that bypass the shared routing/config layers" — provider-specific branching inside the shared coordinator is the same category of drift.Also applies to: 652-686
🤖 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 `@src/oauth/index.ts` around lines 631 - 641, Remove the inline provider === "nous" branches from refreshGenericAccountWithLock and move both behaviors onto OAuthProviderDef as optional onRotationPersisted and rotatedRefreshFromError hooks. Configure the nous provider definition with clearNousRefreshIntent and NousTokenError rotated-refresh extraction, then have the generic coordinator invoke these hooks while preserving existing behavior for providers without them.Source: Path instructions
662-677: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Confirm normalizeCredential preserves refresh/access/expires so the # locally computed generation matches what mergeAccountCredential stores. set -uo pipefail echo "=== normalizeCredential definition ===" ast-grep run --pattern 'function normalizeCredential($$$) { $$$ }' --lang typescript src/oauth/store.ts echo "=== credentialGeneration definition ===" ast-grep run --pattern 'export function credentialGeneration($$$) { $$$ }' --lang typescript src/oauth/store.ts echo "=== Does saveCredential take the refresh intent lock? ===" rg -nP --type=ts -C6 '\bcreateOAuthRefreshIntentLock\s*\(' src/oauth echo "=== All writers that can change a nous credential concurrently ===" rg -nP --type=ts -C3 '\b(saveCredential|mergeAccountCredential|mutateStore)\s*\(' src/oauthRepository: lidge-jun/opencodex
Length of output: 19356
🏁 Script executed:
#!/bin/bash set -uo pipefail echo "=== store.ts credential normalization and generation ===" rg -n -C12 'normalizeCredential|credentialGeneration|function mutateStore|export async function mergeAccountCredential' src/oauth/store.ts echo "=== index.ts preserve helper and recovery call ===" sed -n '480,680p' src/oauth/index.ts echo "=== credential types and serialization helpers ===" rg -n -C8 'interface OAuthCredentials|type OAuthCredentials|function credentialGeneration|const credentialGeneration|normalizeCredential' src/oauth src -g '*.ts'Repository: lidge-jun/opencodex
Length of output: 48856
🏁 Script executed:
#!/bin/bash set -uo pipefail echo "=== Nous provider and single-slot rules ===" rg -n -C8 'SINGLE_SLOT_PROVIDERS|nous|saveCredential\(' src/oauth -g '*.ts' echo "=== Login call sites that persist credentials ===" rg -n -C10 '\bsaveCredential\s*\(' src -g '*.ts' echo "=== Relevant credential identity behavior ===" sed -n '238,292p' src/oauth/store.ts sed -n '475,526p' src/oauth/store.tsRepository: lidge-jun/opencodex
Length of output: 27548
Carry the committed generation out of
preserveNousRotatedRefreshDo not re-read the credential after
mergeAccountCredentialsucceeds.saveCredentialdoes not acquire the refresh lock, so a concurrent Nous login can replace the credential beforegetAccountCredentialruns. The new credential can then be markedneedsReauth.Return
credentialGeneration(recovery)frompreserveNousRotatedRefreshand pass it directly tomarkAccountNeedsReauthIfGeneration.normalizeCredentialpreservesrefresh,access, andexpires, so the generation matches the persisted recovery credential.🤖 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 `@src/oauth/index.ts` around lines 662 - 677, Update preserveNousRotatedRefresh to return credentialGeneration(recovery) after mergeAccountCredential succeeds, and use that returned generation in the outcome === "persisted" path when calling markAccountNeedsReauthIfGeneration. Remove the subsequent getAccountCredential re-read and its persistedGeneration calculation so concurrent logins cannot supply a different credential generation.src/oauth/nous.ts (6)
4-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The free-model list is stated in two files and they now contradict each other on
inclusionai/ling-3.0-flash:free. The registry records that this slug was removed from the Portal free list and returns 404 on the inference API since 2026-08-07, while the OAuth module docstring still presents it as a current example of a:freeslug. A reader ofsrc/oauth/nous.tsgets a model id that no longer resolves.
src/oauth/nous.ts#L4-L7: removeinclusionai/ling-3.0-flash:freefrom the example list in the module docstring and cite a slug that is still in the registry seed, for examplepoolside/laguna-s-2.1:free, or drop the enumeration and point tosrc/providers/registry.tsas the single source.src/providers/registry.ts#L1062-L1076: keep the removal note, which is the authoritative statement, and make it explicit that the seed list here is the only place the fallback slugs are enumerated.📍 Affects 2 files
src/oauth/nous.ts#L4-L7(this comment)src/providers/registry.ts#L1062-L1076🤖 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 `@src/oauth/nous.ts` around lines 4 - 7, Update the module docstring near the Nous gateway description in src/oauth/nous.ts:4-7 to remove inclusionai/ling-3.0-flash:free and either use a currently seeded slug such as poolside/laguna-s-2.1:free or point readers to src/providers/registry.ts. In src/providers/registry.ts:1062-1076, retain the removal note and explicitly state that this seed list is the sole enumeration of fallback slugs.
104-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
updatedAtis recorded and validated but never used; intent files accumulate without bound.
RefreshIntent.updatedAtis written at Line 184, strictly validated at Lines 151-153, and then never read by any consumer.clearRefreshIntentruns only on the success path throughclearNousRefreshIntent. Everyuncertainoutcome (network failure at Line 628, non-2xx at Line 651, parse failure at Line 668) leaves a permanent file in.nous-refresh-intent/, keyed by a token that is already dead. Those files are never removed, so the directory grows once per failed refresh for the lifetime of the config dir.The guard must stay fail-closed, so do not add a TTL that unblocks replay. Instead, prune files that are older than any plausible refresh-token lifetime during
writeRefreshIntent, which is the one point that already touches the directory.♻️ Suggested pruning on write (keeps the guard fail-closed)
+// Intent files outlive the tokens they key. A refresh token that has not been +// submitted for this long can no longer be replayed by this client, so its +// intent record is dead bookkeeping rather than an active guard. +const REFRESH_INTENT_RETENTION_MS = 90 * 24 * 60 * 60 * 1000; + +function pruneExpiredRefreshIntents(dir: string): void { + try { + for (const entry of readdirSync(dir)) { + if (!entry.endsWith(".json")) continue; + const full = join(dir, entry); + try { + const { mtimeMs } = statSync(full); + if (Date.now() - mtimeMs > REFRESH_INTENT_RETENTION_MS) rmSync(full, { force: true }); + } catch { /* a concurrent writer may have removed it */ } + } + } catch { /* pruning is bookkeeping only; never block a refresh */ } +} + function writeRefreshIntent(refreshToken: string, status: RefreshIntentStatus): void { const dir = refreshIntentDir(); // Hardened, owner-only directory + atomic (temp+rename) write. Throws on // failure so the caller can fail closed instead of refreshing blind. mkdirSync(dir, { recursive: true, mode: 0o700 }); hardenConfigDir(); + pruneExpiredRefreshIntents(dir); const path = refreshIntentPath(refreshToken);Add
readdirSyncandstatSyncto thenode:fsimport at Line 37.🤖 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 `@src/oauth/nous.ts` around lines 104 - 199, Update writeRefreshIntent to prune stale intent files after ensuring refreshIntentDir exists, using readdirSync and statSync to remove files whose modification time exceeds the chosen maximum plausible refresh-token lifetime; skip entries that cannot be inspected or removed. Keep readRefreshIntent fail-closed and do not use age to allow replay or bypass the existing guard.
226-255: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The docstring claims an allowlist that the code does not implement.
Lines 226-228 state this mirrors
_NOUS_PORTAL_ALLOWED_HOSTSfrom Hermes.resolvePortalBaseUrlapplies no host allowlist. It accepts any HTTPS origin supplied throughNOUS_PORTAL_BASE_URL. The scheme, credential, query, and fragment checks are real; the host check is not.Accepting any operator-supplied HTTPS origin is consistent with how this repository treats operator-controlled provider URLs, so the behavior is defensible. The comment is not. Correct the claim so a future reader does not assume host containment that is absent.
📝 Proposed comment correction
- * Mirrors the allowlist discipline in Hermes `hermes_cli/auth.py` - * (`_NOUS_PORTAL_ALLOWED_HOSTS`, https-only default - * `DEFAULT_NOUS_PORTAL_URL`). + * Unlike Hermes `hermes_cli/auth.py`, this does NOT apply a host allowlist: + * `NOUS_PORTAL_BASE_URL` is an operator-controlled override, so any HTTPS + * origin is accepted. The scheme/credential/query/fragment checks below are + * the enforced controls; the default remains `NOUS_PORTAL_BASE_URL`.Based on learnings, operator-controlled provider URLs are valid when they reject embedded credentials, query strings, and fragments, and OAuth adapters that attach Bearer credentials must enforce HTTPS separately — which this function does.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.* Unlike Hermes `hermes_cli/auth.py`, this does NOT apply a host allowlist: * `NOUS_PORTAL_BASE_URL` is an operator-controlled override, so any HTTPS * origin is accepted. The scheme/credential/query/fragment checks below are * the enforced controls; the default remains `NOUS_PORTAL_BASE_URL`. */ function resolvePortalBaseUrl(): string { const raw = (process.env.NOUS_PORTAL_BASE_URL || NOUS_PORTAL_BASE_URL).trim(); let url: URL; try { url = new URL(raw); } catch { // Do not echo the raw value: it may contain embedded credentials. Identify // the configuration problem without reflecting secret-bearing input. throw new NousTokenError(undefined, undefined, "Nous Portal base URL is not a valid URL"); } if (url.protocol !== "https:") { throw new NousTokenError(undefined, undefined, `Nous Portal base URL must use HTTPS (got ${url.protocol})`); } if (url.username || url.password) { throw new NousTokenError(undefined, undefined, "Nous Portal base URL must not contain embedded credentials"); } if (url.search) { throw new NousTokenError(undefined, undefined, "Nous Portal base URL must not contain a query string"); } if (url.hash) { throw new NousTokenError(undefined, undefined, "Nous Portal base URL must not contain a fragment"); } // Origin only — no path/query/fragment — so callers cannot smuggle a // non-canonical endpoint through the override. return url.origin; }🤖 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 `@src/oauth/nous.ts` around lines 226 - 255, Update the comment above resolvePortalBaseUrl to remove the inaccurate claim that it mirrors Hermes’s host allowlist, and describe only the validations this function actually performs: HTTPS, no embedded credentials, query strings, or fragments, while returning the URL origin.Source: Learnings
399-410: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A 200 response without
access_tokenthrows a plainError, so the coordinator misclassifies it as retryable.Line 401 throws
new Error("Nous Portal token response did not include an access token"). Every sibling unusable-response branch in this function throws a terminalNousTokenError(Lines 404-409, 412-417, 439-444). The classifier insrc/oauth/index.tsat Lines 447-456 checksinstanceof NousTokenErrorfirst, then falls back toisTerminalRefreshError, which substring-matches onlyinvalid_grant,refresh_token_reused,revoked,access_denied, andexpired_token. This message matches none of them.Failure mode on the refresh path:
refreshNousTokenmarks the intentuncertainat Line 668 and rethrows the plainError.refreshGenericAccountWithLocksees a non-terminal error, rethrows it without callingmarkAccountNeedsReauthIfGeneration, and the account stays nominally valid while its only refresh token is permanently blocked by the replay guard. The next attempt then fails withrefresh_token_reusedinstead of the true cause. The user sees a misleading error and one wasted round trip before reauthentication is signalled.Make the branch terminal and consistent with its siblings.
🐛 Proposed fix
function parseTokenPayload(payload: NousTokenResponse, submittedRefreshToken: string): OAuthCredentials { const access = nonEmptyString(payload.access_token); - if (!access) throw new Error("Nous Portal token response did not include an access token"); + if (!access) { + throw new NousTokenError( + undefined, + "invalid_grant", + "Nous Portal token response did not include an access token", + { terminal: true }, + ); + }Note that the device-login path reaches this branch only when
response.ok && nonEmptyString(payload.access_token)already held (Line 516), so this change affects the refresh path, which is exactly where the misclassification hurts.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function parseTokenPayload(payload: NousTokenResponse, submittedRefreshToken: string): OAuthCredentials { const access = nonEmptyString(payload.access_token); if (!access) { throw new NousTokenError( undefined, "invalid_grant", "Nous Portal token response did not include an access token", { terminal: true }, ); } const refresh = nonEmptyString(payload.refresh_token); if (!refresh) { throw new NousTokenError( undefined, "refresh_token_reused", "Nous Portal did not return a replacement refresh token; refusing to reuse the consumed one (would trigger refresh_token_reused and revoke the session)", { terminal: true }, ); }🤖 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 `@src/oauth/nous.ts` around lines 399 - 410, Update parseTokenPayload so the missing access_token branch throws a terminal NousTokenError, matching the existing missing refresh_token and other unusable-response branches. Preserve the current error message/context while ensuring the error is classified as terminal by the coordinator.
513-517: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A device-login response without
refresh_tokenreports a token-reuse error to a first-time user.Line 516 calls
parseTokenPayload(payload, ""). If the Portal omitsrefresh_tokenon the device grant, Line 404 throws aNousTokenErrorwhose message is "Nous Portal did not return a replacement refresh token; refusing to reuse the consumed one (would trigger refresh_token_reused and revoke the session)" and whoseoauthErrorisrefresh_token_reused.During
ocx login nousthere is no consumed token and no prior session. The rejection is correct, but the message and the OAuth error code describe a rotation failure that did not occur. A user reads it as a revoked session and may re-authenticate in a loop.Distinguish the two callers using the already-available
submittedRefreshToken.🐛 Proposed fix in `parseTokenPayload`
const refresh = nonEmptyString(payload.refresh_token); if (!refresh) { + if (!submittedRefreshToken) { + throw new NousTokenError( + undefined, + "invalid_grant", + "Nous Portal did not return a refresh token for this device authorization; the session cannot be persisted", + { terminal: true }, + ); + } throw new NousTokenError( undefined, "refresh_token_reused", "Nous Portal did not return a replacement refresh token; refusing to reuse the consumed one (would trigger refresh_token_reused and revoke the session)", { terminal: true }, ); }🤖 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 `@src/oauth/nous.ts` around lines 513 - 517, Update parseTokenPayload to distinguish device-login responses from refresh-token rotation using the available submittedRefreshToken value. When no refresh token is submitted, report the missing refresh_token with an initial-login-appropriate message and OAuth error instead of refresh_token_reused; preserve the existing rotation error behavior when submittedRefreshToken is present, and update the caller to pass that value through.
546-554: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Add a note that
deviceCodeintentionally carries the user code, not the device code.Line 551 sets
deviceCode: device.userCode. This is correct and is the secure choice: the RFC 8628device_codeis the secret polled against the token endpoint and must never be rendered, whileuser_codeis the value the human types into the Portal. The field name inOAuthController.onAuth(src/oauth/types.tsLines 50-55) invites a future contributor to "correct" this todevice.deviceCode, which would surface a live credential in CLI output and logs.Record the intent at the call site so the guard is not silently removed.
🛡️ Proposed comment
ctrl.onAuth?.({ url: device.verificationUriComplete, instructions: `Sign in to Nous Portal and enter the code: ${device.userCode}`, + // Deliberately the USER code, not `device.deviceCode`. The device code is + // the secret polled against the token endpoint; rendering it would leak a + // live credential into CLI output and logs. deviceCode: device.userCode, });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export async function loginNous(ctrl: OAuthController): Promise<OAuthCredentials> { const device = await requestDeviceAuthorization(ctrl.signal); ctrl.onAuth?.({ url: device.verificationUriComplete, instructions: `Sign in to Nous Portal and enter the code: ${device.userCode}`, // Deliberately the USER code, not `device.deviceCode`. The device code is // the secret polled against the token endpoint; rendering it would leak a // live credential into CLI output and logs. deviceCode: device.userCode, }); return pollForToken(device.deviceCode, device.intervalMs, device.expiresInMs, ctrl.signal); }🤖 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 `@src/oauth/nous.ts` around lines 546 - 554, Add an inline comment at the onAuth payload in loginNous clarifying that deviceCode intentionally contains device.userCode for display, while the RFC 8628 device code is secret and must not be exposed. Preserve the existing assignment and polling behavior.src/providers/registry.ts (2)
1077-1091: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check how preserveCustomDestination is applied and whether other # recently added OAuth presets set it. set -uo pipefail echo "=== Where preserveCustomDestination is consumed ===" rg -nP --type=ts -C8 '\bpreserveCustomDestination\b' src echo "=== Registry entries that set it, with their ids ===" rg -nP --type=ts -B12 'preserveCustomDestination:\s*true' src/providers/registry.ts | rg -nP '\bid:\s*"|preserveCustomDestination' echo "=== All authKind: \"oauth\" entries and whether they set the flag ===" rg -nP --type=ts -A20 'authKind:\s*"oauth"' src/providers/registry.ts | rg -nP '\bid:\s*"|preserveCustomDestination|authKind'Repository: lidge-jun/opencodex
Length of output: 32723
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Routing and registry-match implementations ===" rg -n -C14 'function routedProviderConfig|const routedProviderConfig|providerMatchesRegistryTransport|preserveCustomDestination|authKind === "oauth"|authMode' src/config.ts src/providers/registry.ts src -g '*.ts' echo "=== Nous registry entry and nearby provider entries ===" sed -n '1025,1105p' src/providers/registry.ts echo "=== OAuth registry entries with ids ===" python3 - <<'PY' from pathlib import Path p = Path("src/providers/registry.ts") lines = p.read_text().splitlines() for i, line in enumerate(lines): if 'authKind: "oauth"' in line: start = max(0, i - 8) end = min(len(lines), i + 8) print(f"--- lines {start+1}-{end} ---") for n in range(start, end): print(f"{n+1}:{lines[n]}") PYRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path("src/providers/registry.ts") text = p.read_text() needle = 'id: "nous"' pos = text.index(needle) entry_end = text.find('\n },', pos) entry = text[pos:entry_end] print("Nous entry contains preserveCustomDestination:", "preserveCustomDestination" in entry) print(entry) PYRepository: lidge-jun/opencodex
Length of output: 1393
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Exact provider matching implementation ===" sed -n '2388,2420p' src/providers/registry.ts echo "=== Exact routed-provider implementation and callers ===" rg -n -l 'routedProviderConfig' src -g '*.ts' | while read -r f; do echo "--- $f ---" rg -n -C18 'routedProviderConfig' "$f" done echo "=== Auth selection around routed configuration ===" rg -n -C12 'authKind|authMode|oauthId|oauth' src/server src/providers src/oauth -g '*.ts' | rg -n -C4 'routedProvider|registry|authKind|authMode|oauthId|bearer|Authorization' | head -n 500Repository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path p = Path("src/providers/registry.ts") lines = p.read_text().splitlines() for i, line in enumerate(lines): if 'id: "nous"' in line: print(f"Nous registry entry starts at line {i+1}") if 'function providerMatchesRegistryTransport' in line: print(f"Matcher starts at line {i+1}") PYRepository: lidge-jun/opencodex
Length of output: 225
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path registry = Path("src/providers/registry.ts").read_text() router = Path("src/router.ts").read_text() nous_start = registry.index('id: "nous"') nous_end = registry.index("\n },", nous_start) nous = registry[nous_start:nous_end] matcher_start = registry.index("export function providerMatchesRegistryTransport") matcher_end = registry.index("\n}", matcher_start) + 2 matcher = registry[matcher_start:matcher_end] route_start = router.index("export function routedProviderConfig") route_end = router.index("\n}", route_start) + 2 route = router[route_start:route_end] assert 'authKind: "oauth"' in nous assert "preserveCustomDestination" not in nous assert 'if (entry.authKind !== "key" || entry.preserveCustomDestination !== true) return true;' in matcher assert 'registryEntry.authKind === "oauth"' in route assert 'registryEntry.authKind' in route and 'canonicalAuthMode' in route # Evaluate the matcher branch for the Nous entry and a pre-existing key provider. entry = {"authKind": "oauth", "preserveCustomDestination": True} custom_provider = { "authMode": "key", "adapter": "openai-chat", "baseUrl": "https://user.example/v1", } matcher_short_circuit = entry["authKind"] != "key" or entry["preserveCustomDestination"] is not True assert matcher_short_circuit is True assert custom_provider["authMode"] == "key" print("Nous is an OAuth registry entry without preserveCustomDestination.") print("The matcher short-circuits to true for OAuth entries, even for a same-named key provider.") print("The router therefore reaches the OAuth canonical-auth branch unless the matcher rejects the collision.") PYRepository: lidge-jun/opencodex
Length of output: 417
Guard OAuth registry matching by
authMode.providerMatchesRegistryTransport()returnstruefor every OAuth entry, so a pre-existing key provider namednouscan be retargeted and routed with Nous OAuth credentials. Returnfalsewhen an OAuth entry matches a provider whoseauthModeis not"oauth"; keep OAuth providers pinned to the registry endpoint.🤖 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 `@src/providers/registry.ts` around lines 1077 - 1091, Update providerMatchesRegistryTransport() to reject OAuth registry entries when the matching provider’s authMode is not "oauth", preventing pre-existing key-auth providers such as "nous" from being retargeted. Preserve the existing OAuth matching behavior for providers configured with authMode "oauth" and keep them pinned to the registry endpoint.
1092-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The
notestring duplicates the dated seed list and will go stale.Line 1099 repeats the four fallback slugs and the date
2026-08-10that already appear in the comment at Lines 1070-1074 and in themodelsarray at Line 1091.liveModels: truemeans the real catalog comes from discovery, so this list is the logged-out fallback only. When the fallback changes, three places must change together, and thenoteis the one that is user-visible.Drop the enumeration from the user-facing
noteand keep the authoritative list inmodels.♻️ Proposed change
- note: "Nous Research subscription gateway. OAuth device login with your own Portal account; mixed paid + :free models discovered live (fallback seed 2026-08-10: tencent/hy3:free, poolside/laguna-s-2.1:free, stepfun/step-3.7-flash:free, poolside/laguna-xs-2.1:free).", + note: "Nous Research subscription gateway. OAuth device login with your own Portal account; mixed paid and :free models are discovered live from the signed-in account.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.modelDiscovery: { // Resolves against effectiveBaseUrl (registry baseUrl .../v1) to the same // canonical endpoint https://inference-api.nousresearch.com/v1/models. path: "models", maxResponseBytes: 262_144, maxModels: 512, }, note: "Nous Research subscription gateway. OAuth device login with your own Portal account; mixed paid and :free models are discovered live from the signed-in account.",🤖 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 `@src/providers/registry.ts` around lines 1092 - 1099, Update the user-facing note in the Nous Research registry configuration to remove the dated fallback model list, since the authoritative fallback entries remain in the models array and live discovery supplies the catalog when available. Keep the note focused on the subscription gateway and OAuth device login details without duplicating model slugs or dates.tests/nous-oauth-live.test.ts (2)
42-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check the OAuthProviderDef required members and whether the # two-argument expect() form is used elsewhere in the suite. set -uo pipefail echo "=== OAuthProviderDef definition ===" ast-grep run --pattern 'export interface OAuthProviderDef { $$$ }' --lang typescript src/oauth/types.ts rg -nP --type=ts -C20 '\bOAuthProviderDef\b' src/oauth/types.ts echo "=== Pinned Bun version ===" rg -nP '"(bun-types|`@types/bun`)"\s*:' package.json rg -nP '"bun"\s*:' package.json echo "=== Is the two-argument expect() form used anywhere else? ===" rg -nP --type=ts -C1 'expect\([^)]+,\s*"' tests | head -40Repository: lidge-jun/opencodex
Length of output: 3111
🌐 Web query:
Does Bun 1.3 bun:test expect() accept a second message argument?💡 Result:
In Bun 1.3, the expect function in bun:test does not accept a second message argument for custom error messages [1]. While there have been community discussions and feature requests to add this capability (similar to other test frameworks), it is not a natively supported feature of the expect function itself [1]. Users typically implement custom error reporting within their test logic or through custom matchers if specific error messaging is required [2].
Citations:
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== OAuthProviderDef declarations and uses ===" rg -n -C8 --glob '*.ts' --glob '*.tsx' 'OAuthProviderDef' src tests echo "=== Relevant source files ===" fd -t f -a 'types.ts|index.ts|nous-oauth-live.test.ts|tsconfig.*|package.json' . echo "=== Target test context ===" cat -n tests/nous-oauth-live.test.ts | sed -n '35,85p' echo "=== Typecheck configuration and scripts ===" cat -n package.json | sed -n '1,110p' fd -t f -a -g 'tsconfig*.json' -x sh -c 'echo "--- $1"; cat -n "$1"' sh '{}'Repository: lidge-jun/opencodex
Length of output: 22348
🌐 Web query:
site:github.com/oven-sh/bun bun:test expect second argument message TypeScript definitions 1.3.14💡 Result:
In Bun version 1.3.14, the TypeScript definition for the expect function in bun:test includes an optional second argument, customFailMessage, which allows you to specify a custom error message to display if the test assertion fails [1]. The signature of the expect function is defined as [1]: (actual?: never, customFailMessage?: string): Matchers; <T = unknown>(actual: T, customFailMessage?: string): Matchers; By providing this second string argument, you can pass a custom message that will be surfaced in the test output when the assertion fails, providing more context than the default failure message [1][2]. This feature addresses the need for descriptive error messages in test suites, as previously requested by users [3].
Citations:
- 1: https://github.com/oven-sh/bun/blob/74e191b3/packages/bun-types/test.d.ts
- 2: https://github.com/oven-sh/bun/blob/7abe6c38/src/bun.js/test/expect.zig
- 3: oven-sh/bun#10414
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Imported OAuthProviderDef definition ===" cat -n src/oauth/types.ts | sed -n '1,220p' echo "=== All local declarations of OAuthProviderDef ===" rg -n -C12 --glob '*.ts' '(^|[[:space:]])(export[[:space:]]+)?(interface|type)[[:space:]]+OAuthProviderDef\b' . echo "=== Test inclusion in TypeScript configurations ===" rg -n -C4 '"include"|"exclude"|"types"|"typeRoots"' --glob 'tsconfig*.json' . rg -n -C3 'typecheck|tsc|bun test|tests/' package.json scripts tests echo "=== Bun 1.3.14 tag references ===" curl -fsSL 'https://api.github.com/repos/oven-sh/bun/git/refs/tags/bun-v1.3.14' | sed -n '1,80p' echo "=== Bun expect declaration candidates ===" for ref in bun-v1.3.14 main; do url="https://raw.githubusercontent.com/oven-sh/bun/$ref/packages/bun-types/test.d.ts" echo "--- $url" curl -fsSL "$url" | rg -n -C3 'function expect|expect<|customFailMessage' || true doneRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Complete private OAuthProviderDef shape ===" cat -n src/oauth/index.ts | sed -n '137,160p' echo "=== Complete imported type module and exports ===" cat -n src/oauth/types.ts printf '\nExports named OAuthProviderDef:\n' rg -n 'OAuthProviderDef|^export ' src/oauth/types.ts || true echo "=== Exact Bun 1.3.14 tag and expect declaration ===" curl -fsSL 'https://api.github.com/repos/oven-sh/bun/git/refs/tags/bun-v1.3.14' \ | jq '{ref: .ref, sha: .object.sha, type: .object.type}' curl -fsSL 'https://raw.githubusercontent.com/oven-sh/bun/bun-v1.3.14/packages/bun-types/test.d.ts' \ | rg -n -C5 'customFailMessage|function expect|expect<' | head -80 echo "=== Read-only source verifier ===" python3 - <<'PY' from pathlib import Path import re types = Path("src/oauth/types.ts").read_text() index = Path("src/oauth/index.ts").read_text() test = Path("tests/nous-oauth-live.test.ts").read_text() print("types exports OAuthProviderDef:", bool(re.search(r"\b(?:interface|type)\s+OAuthProviderDef\b", types))) m = re.search(r"interface OAuthProviderDef\s*\{(.*?)\n\}", index, re.S) if not m: raise SystemExit("private interface not found") members = re.findall(r"^\s*([A-Za-z_$][\w$]*)\s*(?:\?|:|\()", m.group(1), re.M) print("index OAuthProviderDef members:", members) print("test imports OAuthProviderDef from src/oauth/types.ts:", 'import type { OAuthProviderDef } from "../src/oauth/types"' in test) print("two-argument expect calls:", len(re.findall(r"\bexpect\([^,\n]+,\s*"', test))) PYRepository: lidge-jun/opencodex
Length of output: 5507
Use an exported provider type
tests/nous-oauth-live.test.ts:27importsOAuthProviderDeffromsrc/oauth/types.ts, but that module does not export it. The private definition insrc/oauth/index.ts:137-153also requireslogin,providerConfig, anddefaultModel, so it cannot type the minimal literal at lines 42–45. Export a refresh-only provider type forrefreshGenericAccountWithLock, or provide the complete provider definition. Bun 1.3.14 supports the two-argumentexpectform.🤖 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` around lines 42 - 51, Update the provider typing used by NOUS_DEF and refreshGenericAccountWithLock so it references an exported type from src/oauth/types.ts. Add or reuse a refresh-only provider type matching the minimal id and refresh members, rather than importing the private full provider definition from src/oauth/index.ts; preserve the existing two-argument expect usage.
61-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Determine whether a store account row id equals the credential's # accountId (JWT sub) for a provider saved through saveCredential. set -uo pipefail echo "=== newAccountId: how row ids are minted ===" ast-grep run --pattern 'function newAccountId($$$) { $$$ }' --lang typescript src/oauth/store.ts echo "=== SINGLE_SLOT_PROVIDERS: does nous get special-cased? ===" rg -nP --type=ts -C4 '\bSINGLE_SLOT_PROVIDERS\b' src/oauth/store.ts echo "=== How other tests resolve an account id for the refresh coordinator ===" rg -nP --type=ts -C2 'refreshGenericAccountWithLock\s*\(' testsRepository: lidge-jun/opencodex
Length of output: 1762
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== relevant store APIs and account-id construction ===" rg -n -C8 'function newAccountId|export function (getCredential|getAccountCredential|getAccountSet)|function saveCredential|export async function saveCredential|accounts\.find|activeAccountId|identityFromNousTokens' src/oauth/store.ts src/oauth/nous.ts echo "=== live test imports and setup ===" cat -n tests/nous-oauth-live.test.ts | sed -n '1,90p' echo "=== refresh coordinator lookup and call contract ===" rg -n -C8 'refreshGenericAccountWithLock|OAuthLoginRequiredError' src tests/oauth-refresh.test.ts tests/oauth-refresh-lock-multiprocess.test.ts echo "=== focused static verifier for identifier flow ===" python3 - <<'PY' from pathlib import Path store = Path("src/oauth/store.ts").read_text() live = Path("tests/nous-oauth-live.test.ts").read_text() nous = Path("src/oauth/nous.ts").read_text() for needle, text in [ ("SINGLE_SLOT_PROVIDERS = new Set([\"chatgpt\"])", store), ("accounts.find(a => a.id === accountId)", store), ("activeAccountId", live), ("stored!.accountId!", live), ("sub", nous), ]: print(f"{needle!r}: {needle in text}") PYRepository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== coordinator implementation ===" rg -n -C12 'export async function refreshGenericAccountWithLock|async function refreshGenericAccountWithLock|refreshGenericAccountWithLock\s*=' src/oauth echo "=== all direct account-id setup in the Nous refresh tests ===" sed -n '755,885p' tests/oauth-refresh.test.ts echo "=== account-id type definitions ===" rg -n -C5 'interface ProviderAccount|type ProviderAccount|interface OAuthCredentials|type OAuthCredentials' src/oauthRepository: lidge-jun/opencodex
Length of output: 10024
Use the store row ID for refresh operations.
stored.accountIdis the Nous JWTsub(src/oauth/nous.ts:279-287). The store derives the row ID assha256(accountId).slice(0, 8)(src/oauth/store.ts:295-297). Atsrc/oauth/index.ts:604-605, the coordinator looks up the row by that ID and throwsOAuthLoginRequiredErrorwhen it is absent.Import
getAccountSet, assignconst rowId = getAccountSet("nous")?.activeAccountId, assert that it exists, and passrowId!torefreshGenericAccountWithLockandgetAccountCredentialattests/nous-oauth-live.test.ts:63,72. Existing Nous refresh tests use this pattern.🤖 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` around lines 61 - 75, Update the Nous refresh test to use the store row ID rather than the JWT account ID: import and call getAccountSet("nous"), assign its activeAccountId to rowId, and assert that rowId exists. Pass rowId! to both refreshGenericAccountWithLock and getAccountCredential, while preserving the existing token-rotation assertions.tests/nous-oauth.test.ts (2)
10-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hoist the env-save variables into each describe block instead of sharing module-level
letbindings.Lines 12-13 declare
previousOpencodexHomeandpreviousPortalBaseat module scope. Four separate describe blocks write them from their ownbeforeEach(Lines 62-63, 274, 400-401, 528-531). Two later blocks already use block-local variables instead (previousHomeat Lines 669 and 747), which is the correct pattern.Under Bun's default sequential execution within a file this works. It breaks the moment anyone adds
test.concurrent, or runs with--randomizecombined with a nested asyncafterEach, because two blocks then interleave through one save slot and restore the wrong value. The sharedTEST_DIRat Line 10 has the same property: every block doesrmSyncthenmkdirSyncon the identical path.Make each block own its state, matching the pattern the file already uses at Lines 669 and 747.
♻️ Proposed change
const TEST_DIR = join(import.meta.dir, ".tmp-nous-oauth-test"); const TEST_PORTAL = "https://portal.test"; -let previousOpencodexHome: string | undefined; -let previousPortalBase: string | undefined;Then declare
let previousOpencodexHome: string | undefined;andlet previousPortalBase: string | undefined;inside each describe body that needs them.As per path instructions for
tests/**, tests are flat Bun tests undertests/; keeping per-block isolation self-contained keeps a focused regression test independent of its neighbours.🤖 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.test.ts` around lines 10 - 13, Move previousOpencodexHome and previousPortalBase from module scope into each describe block whose beforeEach/afterEach uses them, following the existing previousHome pattern in the later blocks. Keep each block’s environment-save state independent, and avoid sharing TEST_DIR across concurrent or interleaved blocks by giving each describe its own temporary directory state and path.Source: Path instructions
271-303: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This describe block does not save
NOUS_PORTAL_BASE_URLbefore its tests overwrite and delete it.The
beforeEachat Lines 273-278 saves and restoresOPENCODEX_HOMEbut notNOUS_PORTAL_BASE_URL. The test at Line 287 assignsprocess.env.NOUS_PORTAL_BASE_URLand itsfinallyat Line 301 issuesdelete process.env.NOUS_PORTAL_BASE_URL. The same pattern repeats at Lines 318, 340, 372, and 391.If the ambient environment already defines
NOUS_PORTAL_BASE_URL, these tests delete it permanently for the rest of the process. Bun can run several test files in one process, so the deletion is not contained to this file. Every other describe block in this file saves and restores the variable (Lines 62, 75-76, 157, 163-164, 400, 410-411, 528, 539-540); this block is the outlier.Save and restore it here as well.
💚 Proposed fix
describe("Nous Portal base URL hardening", () => { const realFetch = globalThis.fetch; beforeEach(() => { previousOpencodexHome = process.env.OPENCODEX_HOME; + previousPortalBase = process.env.NOUS_PORTAL_BASE_URL; if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; }); afterEach(() => { globalThis.fetch = realFetch; if (previousOpencodexHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = previousOpencodexHome; + if (previousPortalBase === undefined) delete process.env.NOUS_PORTAL_BASE_URL; + else process.env.NOUS_PORTAL_BASE_URL = previousPortalBase; if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); });The per-test
deletecalls in thefinallyblocks then become redundant and can be removed, sinceafterEachrestores the correct prior value.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.describe("Nous Portal base URL hardening", () => { const realFetch = globalThis.fetch; beforeEach(() => { previousOpencodexHome = process.env.OPENCODEX_HOME; previousPortalBase = process.env.NOUS_PORTAL_BASE_URL; if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); mkdirSync(TEST_DIR, { recursive: true }); process.env.OPENCODEX_HOME = TEST_DIR; }); afterEach(() => { globalThis.fetch = realFetch; if (previousOpencodexHome === undefined) delete process.env.OPENCODEX_HOME; else process.env.OPENCODEX_HOME = previousOpencodexHome; if (previousPortalBase === undefined) delete process.env.NOUS_PORTAL_BASE_URL; else process.env.NOUS_PORTAL_BASE_URL = previousPortalBase; if (existsSync(TEST_DIR)) rmSync(TEST_DIR, { recursive: true }); }); test("an HTTP override fails before fetch is invoked", async () => { process.env.NOUS_PORTAL_BASE_URL = "http://portal.test"; let fetchCalled = false; const realFetch = globalThis.fetch; globalThis.fetch = (async () => { fetchCalled = true; return new Response("{}", { status: 200 }); }) as typeof fetch; try { const ctrl: OAuthController = { onAuth() {} }; await expect(loginNous(ctrl)).rejects.toThrow(/must use HTTPS/); await expect(refreshNousToken("hardening-refresh")).rejects.toThrow(/must use HTTPS/); expect(fetchCalled).toBe(false); } finally { globalThis.fetch = realFetch; delete process.env.NOUS_PORTAL_BASE_URL; } });🤖 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.test.ts` around lines 271 - 303, Update the “Nous Portal base URL hardening” describe block to save the ambient NOUS_PORTAL_BASE_URL value in beforeEach and restore it in afterEach, preserving both defined and undefined states. Remove the redundant per-test deletion of NOUS_PORTAL_BASE_URL from the affected finally blocks, while retaining fetch cleanup and existing test behavior.tests/oauth-refresh.test.ts (1)
911-951: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add coverage for a concurrent commit that lands in the
persistedbranch, not only thesupersededbranch.This test covers the case where
mergeAccountCredentialreportssuperseded, sosrc/oauth/index.tstakes theelsebranch at Lines 678-683 and marks the old generation, which is a no-op. That path is correct and well covered.The uncovered path is the
persistedbranch at Lines 662-677. There, the code re-reads the credential withgetAccountCredentialafter a successful write and derivespersistedGenerationfrom whatever it finds. A writer that commits between the successful merge and that re-read causes the code to mark a credential it never wrote. No test exercises that window, which is why the defect I flagged onsrc/oauth/index.tsLines 662-677 is invisible to this suite.The existing
afterPrePersistReadhook onGenericRefreshDepsis the seam for a deterministic test, butpreserveNousRotatedRefreshdoes not pass it through. A simpler deterministic approach is to make the merge spy commit a newer credential and still report{ superseded: false }, which reproduces the exact interleaving.💚 Sketch of the missing regression test
+ test("Nous RT-B preservation does not mark a credential committed by a concurrent writer", async () => { + await saveCredential("nous", { access: "old", refresh: "rt-old", expires: 1, accountId: "nous-toctou" }); + const id = getAccountSet("nous")!.activeAccountId; + + const unusableAccess = nousAccessJwt("nous-toctou", "billing:manage"); + globalThis.fetch = (async () => new Response(JSON.stringify({ + access_token: unusableAccess, + refresh_token: "rt-new", + expires_in: 3600, + }), { status: 200 })) as typeof fetch; + + // The recovery write succeeds, then a concurrent login commits a fresh, + // fully usable credential before the coordinator re-reads the store. + const realMerge = storeModule.mergeAccountCredential; + const mergeSpy = spyOn(storeModule, "mergeAccountCredential").mockImplementation(async (provider, accountId, cred, opts) => { + const outcome = await realMerge(provider, accountId, cred, opts); + await saveCredential("nous", { + access: nousAccessJwt("nous-toctou"), + refresh: "rt-fresh-login", + expires: Date.now() + 3600_000, + accountId: "nous-toctou", + }); + return outcome; + }); + try { + await expect(getValidAccessTokenForAccount("nous", id)).rejects.toBeInstanceOf(OAuthLoginRequiredError); + // The concurrently committed, usable credential must remain usable. + expect(getCredential("nous")?.refresh).toBe("rt-fresh-login"); + expect(getAccountSet("nous")!.accounts[0]!.needsReauth).toBeUndefined(); + } finally { + mergeSpy.mockRestore(); + } + });As per path instructions for
tests/**, "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem." The generation-safety behavior added in this PR needs thepersistedinterleaving covered, not onlysuperseded.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.test("Nous RT-B preservation never overwrites a newer concurrent generation", async () => { await saveCredential("nous", { access: "old", refresh: "rt-old", expires: 1, accountId: "nous-superseded" }); const id = getAccountSet("nous")!.activeAccountId; const credential = getAccountCredential("nous", id)!; const unusableAccess = nousAccessJwt("nous-superseded", "billing:manage"); globalThis.fetch = (async () => new Response(JSON.stringify({ access_token: unusableAccess, refresh_token: "rt-new", expires_in: 3600, }), { status: 200 })) as typeof fetch; // A concurrent refresh already committed a newer generation (RT-C) before // this attempt's RT-B persistence runs; the merge must report superseded // and never overwrite it. const newerCredential = { access: "newer-access", refresh: "rt-concurrent", expires: Date.now() + 3600_000, accountId: "nous-superseded", }; // The concurrent refresh commits RT-C through the real store write before // this attempt's RT-B persistence is detected as superseded. const realMerge = storeModule.mergeAccountCredential; const mergeSpy = spyOn(storeModule, "mergeAccountCredential").mockImplementation(async (provider, accountId, cred, opts) => { await realMerge(provider, accountId, newerCredential as never, { expectedGeneration: opts?.expectedGeneration }); return { superseded: true, stored: newerCredential as never }; }); try { await expect(getValidAccessTokenForAccount("nous", id)).rejects.toBeInstanceOf(OAuthLoginRequiredError); // The concurrent credential is untouched. expect(getCredential("nous")?.refresh).toBe("rt-concurrent"); // RT-A's intent is not cleared (RT-A was consumed; a later refresh of the // newer generation manages its own intent). expect(nousRefreshIntentBlocksReplay("rt-old")).toBe(true); // The newer generation is NOT marked needsReauth (generation-safe no-op). expect(getAccountSet("nous")!.accounts[0]!.needsReauth).toBeUndefined(); } finally { mergeSpy.mockRestore(); } }); test("Nous RT-B preservation does not mark a credential committed by a concurrent writer", async () => { await saveCredential("nous", { access: "old", refresh: "rt-old", expires: 1, accountId: "nous-toctou" }); const id = getAccountSet("nous")!.activeAccountId; const unusableAccess = nousAccessJwt("nous-toctou", "billing:manage"); globalThis.fetch = (async () => new Response(JSON.stringify({ access_token: unusableAccess, refresh_token: "rt-new", expires_in: 3600, }), { status: 200 })) as typeof fetch; // The recovery write succeeds, then a concurrent login commits a fresh, // fully usable credential before the coordinator re-reads the store. const realMerge = storeModule.mergeAccountCredential; const mergeSpy = spyOn(storeModule, "mergeAccountCredential").mockImplementation(async (provider, accountId, cred, opts) => { const outcome = await realMerge(provider, accountId, cred, opts); await saveCredential("nous", { access: nousAccessJwt("nous-toctou"), refresh: "rt-fresh-login", expires: Date.now() + 3600_000, accountId: "nous-toctou", }); return outcome; }); try { await expect(getValidAccessTokenForAccount("nous", id)).rejects.toBeInstanceOf(OAuthLoginRequiredError); // The concurrently committed, usable credential must remain usable. expect(getCredential("nous")?.refresh).toBe("rt-fresh-login"); expect(getAccountSet("nous")!.accounts[0]!.needsReauth).toBeUndefined(); } finally { mergeSpy.mockRestore(); } });🤖 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/oauth-refresh.test.ts` around lines 911 - 951, Add a focused regression test alongside “Nous RT-B preservation never overwrites a newer concurrent generation” that has the mergeAccountCredential spy commit a newer credential through the real store write but return { superseded: false, stored: ... }, exercising the persisted branch of getValidAccessTokenForAccount. Assert the concurrent refresh remains stored and the newer generation is not marked needsReauth, while preserving the existing replay-intent expectations; restore the spy in a finally block.Source: Path instructions
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The incremental direction is correct, but I am requesting changes on current head 25d462fd3210798795e7dc4637793a5c5ee86bca for two in-scope correctness gaps:
- The live test still uses
stored.accountIdas the account-store row id in bothrefreshGenericAccountWithLock()and the newgetAccountCredential()call. Those are different identities:saveCredential()stores rows undernewAccountId(credential), an 8-character SHA-256-derived id, whilecredential.accountIdis the provider/JWT identity. As written,getAccountCredential(provider, accountId)returns null and the coordinator fails before the live refresh. ReadgetAccountSet("nous")!.activeAccountIdonce, assert it exists, and pass that row id to both the coordinator and the post-refresh read. - The same raw Kiro install pipes still exist in the English table at
docs-site/src/content/docs/guides/providers.md:117. Escaping only ja/ko/ru/zh-cn leaves the canonical English table malformed. Apply the same|substitution there and keep the displayed commands unchanged.
The current exact-head CI is also not green: test 3/4 was cancelled during its Test step after 15 minutes, which makes the aggregate ci job fail. After the focused fixes, rerun that shard/CI and keep the merge gated on a complete green head.
I am not folding the broad CodeRabbit comments against the already-merged #1397 base into this five-file follow-up review; this request is limited to defects introduced or left incomplete by the incremental diff.
- 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 (&lidge-jun#124;) 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).
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 (10)
src/oauth/nous.ts (5)
290-295: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the JWT
expclaim before you prefer it overexpires_in.
jwtExpiryMsaccepts any finite number and line 434 prefers it overexpires_in. The claim comes from a JWT that this code decodes but never verifies, and there is no sanity bound. Two concrete failure modes:
- A too-large
exp(server bug, wrong unit — for example the Portal emitting milliseconds instead of seconds — or clock skew) produces an expiry years in the future. The shared refresh gate only refreshes on expiry, so the credential is never refreshed. Every inference request then fails with 401 and the client never self-recovers. The user must re-authenticate manually.- A too-small
expproducesexpiresin the past, or even negative after subtractingOAUTH_EXPIRY_SKEW_MS. That drives an immediate refresh on every call, and each refresh consumes a single-use token. Combined with the replay guard, that path burns the session.Clamp
expto a plausible window relative to now, and fall back toexpires_inwhen the claim is out of range.🛡️ Proposed fix: reject implausible `exp` values
+/** Upper bound on a plausible inference-JWT lifetime; guards against a bad + * `exp` unit (ms instead of s) or a badly skewed clock. */ +const MAX_PLAUSIBLE_TOKEN_LIFETIME_MS = 30 * 24 * 60 * 60 * 1000; + /** JWT `exp` (epoch seconds) → expiry ms, when present and sane. */ function jwtExpiryMs(payload: NousJwtPayload | undefined): number | undefined { const exp = payload?.exp; if (typeof exp !== "number" || !Number.isFinite(exp)) return undefined; - return exp * 1000; + const expMs = exp * 1000; + // Ignore an implausible claim and let the caller fall back to `expires_in`. + if (expMs <= Date.now() || expMs > Date.now() + MAX_PLAUSIBLE_TOKEN_LIFETIME_MS) return undefined; + return expMs; }Also clamp the final value so a skew subtraction can never yield a negative expiry:
- const expires = (expMs ?? (expiresInMs !== undefined ? Date.now() + expiresInMs : Date.now() + DEFAULT_DEVICE_FLOW_TTL_MS)) - - OAUTH_EXPIRY_SKEW_MS; + const rawExpires = expMs + ?? (expiresInMs !== undefined ? Date.now() + expiresInMs : Date.now() + DEFAULT_ACCESS_TOKEN_TTL_MS); + const expires = Math.max(0, rawExpires - OAUTH_EXPIRY_SKEW_MS);Separately, line 434 uses
DEFAULT_DEVICE_FLOW_TTL_MS(the device-authorization window) as the fallback access token lifetime. Those are unrelated durations. Introduce a distinctDEFAULT_ACCESS_TOKEN_TTL_MSconstant so a later change to the device-flow window does not silently move token expiry.Also applies to: 429-435
🤖 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 `@src/oauth/nous.ts` around lines 290 - 295, Update jwtExpiryMs to accept exp only within a plausible now-relative window, returning undefined for out-of-range values so the caller falls back to expires_in, and ensure the final skew-adjusted expiry cannot be negative. In the access-token expiry flow around jwtExpiryMs, replace DEFAULT_DEVICE_FLOW_TTL_MS with a distinct DEFAULT_ACCESS_TOKEN_TTL_MS constant used only for access-token lifetime fallback.
104-109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIntent files accumulate without bound, and
updatedAtis recorded but never used.Every refresh writes one JSON file into
.nous-refresh-intent, keyed bysha256(refreshToken). Removal happens only throughclearNousRefreshIntent, whichsrc/oauth/index.tscalls best-effort and whose failures it deliberately swallows (lines 642-648 and 676-682 of that file). Any of these leaves an orphan file forever:
clearRefreshIntentthrowsRefreshIntentIOErroron a non-ENOENT unlink failure, and the caller logs and continues.- The
supersededbranch of the happy path insrc/oauth/index.ts(lines 628-631) returns before reaching the cleanup call.- Any caller of
refreshNousTokenthat is notrefreshGenericAccountWithLocknever clears anything.- A crash between the 200 response and persistence leaves the file by design.
Nothing prunes the directory. A long-lived install that refreshes hourly writes roughly 8,700 files per year into one directory. That is not fatal, but it degrades directory-scan performance and never self-heals.
RefreshIntent.updatedAtmakes this fixable and is currently dead weight:parseRefreshIntentvalidates it at line 151, andreadRefreshIntentthen discards it. No code path reads the value.Use it. Add a maximum intent age and drop expired entries on read, plus an opportunistic sweep on write. Choose the age well above any realistic crash-recovery window (a refresh token's own lifetime is the natural bound) so the fail-closed replay guard keeps its safety property.
♻️ Proposed fix: expire stale intents using the recorded `updatedAt`
+// An intent older than this can no longer correspond to a live session: the +// refresh token it keys has itself expired server-side, so retaining the +// block protects nothing and only leaks a file. +const REFRESH_INTENT_MAX_AGE_MS = 90 * 24 * 60 * 60 * 1000; + +/** Drop intent files whose recorded `updatedAt` is older than the max age. */ +function pruneStaleRefreshIntents(): void { + const dir = refreshIntentDir(); + if (!existsSync(dir)) return; + const cutoff = Date.now() - REFRESH_INTENT_MAX_AGE_MS; + for (const name of readdirSync(dir)) { + if (!name.endsWith(".json")) continue; + const path = join(dir, name); + try { + if (parseRefreshIntent(readFileSync(path, "utf8")).updatedAt < cutoff) { + rmSync(path, { force: true }); + } + } catch { + // A file we cannot read stays: fail-closed beats deleting a live guard. + } + } +}Call
pruneStaleRefreshIntents()fromwriteRefreshIntentaftermkdirSync, and wrap it so a prune failure never blocks the refresh. Note thatparseRefreshIntentreturnsupdatedAt: Date.now()for corrupt input, so a corrupt file is never pruned by age. That is the correct fail-closed behavior.Also applies to: 151-154, 190-199
🤖 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 `@src/oauth/nous.ts` around lines 104 - 109, Use RefreshIntent.updatedAt to expire stale refresh intents: define a maximum age based on the refresh token lifetime, have read logic discard entries older than that threshold, and add pruneStaleRefreshIntents to remove expired files. Invoke pruning from writeRefreshIntent after directory creation, but swallow or log pruning failures so they never block refresh; preserve parseRefreshIntent’s fail-closed handling for corrupt entries.
226-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe HTTPS enforcement is right; the docstring overstates it.
The validation matches the required discipline for a credential-bearing endpoint: HTTPS only, no embedded credentials, no query, no fragment, origin-only return. Based on learnings, operator-controlled provider URLs may use
http:orhttps:in general, but OAuth adapters that attach bearer credentials must enforce HTTPS separately — this function does exactly that.One inaccuracy: lines 226-228 say the function mirrors
_NOUS_PORTAL_ALLOWED_HOSTSfrom Hermes. No host allowlist is implemented here; any HTTPS origin passes. Either drop the allowlist claim from the comment, or add the host check the comment promises. The comment as written could lead a future maintainer to assume host pinning exists.📝 Proposed comment correction
- * Mirrors the allowlist discipline in Hermes `hermes_cli/auth.py` - * (`_NOUS_PORTAL_ALLOWED_HOSTS`, https-only default - * `DEFAULT_NOUS_PORTAL_URL`). + * Mirrors the https-only default in Hermes `hermes_cli/auth.py` + * (`DEFAULT_NOUS_PORTAL_URL`). Note: unlike Hermes, this function does not + * pin the host to an allowlist; any HTTPS origin passes.🤖 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 `@src/oauth/nous.ts` around lines 226 - 255, Correct the documentation above resolvePortalBaseUrl so it no longer claims to mirror Hermes’s host allowlist, since the function only validates HTTPS, credentials, query, fragment, and origin normalization. Keep the existing validation behavior unchanged and describe only the HTTPS/default-URL discipline actually implemented.Source: Learnings
501-553: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA single transient network error aborts the whole 15-minute device login.
pollForTokenwraps each poll inrequestSignal(signal), which applies a 30-secondAbortSignal.timeout. Nothing in the loop catches a rejectedfetch. Any transport-level failure — the 30-second timeout firing, a DNS hiccup, a dropped connection, a proxy reset — propagates straight out ofloginNousand destroys the device-authorization session, even thoughdeadlinemay still be 14 minutes away and the user is actively signing in at the Portal.This is the exact window where failures are most likely: the loop polls every 5 seconds for up to 15 minutes, so it issues up to 180 requests. One failure out of 180 kills the login and the user must restart with a new user code.
RFC 8628 polling should tolerate transient transport errors until
expires_inelapses. Only genuine cancellation should abort early.🛠️ Proposed fix: tolerate transient poll failures until the deadline
while (Date.now() < deadline) { if (signal?.aborted) throw new Error("Login cancelled"); - const response = await fetch(`${resolvePortalBaseUrl()}/api/oauth/token`, { - method: "POST", - headers: { Accept: "application/json", "Content-Type": "application/x-www-form-urlencoded" }, - body: new URLSearchParams({ - client_id: NOUS_OAUTH_CLIENT_ID, - device_code: deviceCode, - grant_type: "urn:ietf:params:oauth:grant-type:device_code", - }), - redirect: "error", - signal: requestSignal(signal), - }); + let response: Response; + try { + response = await fetch(`${resolvePortalBaseUrl()}/api/oauth/token`, { + method: "POST", + headers: { Accept: "application/json", "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ + client_id: NOUS_OAUTH_CLIENT_ID, + device_code: deviceCode, + grant_type: "urn:ietf:params:oauth:grant-type:device_code", + }), + redirect: "error", + signal: requestSignal(signal), + }); + } catch (netErr) { + // Caller cancellation must still abort; a transient transport error must + // not destroy a device session that is still within its deadline. + if (signal?.aborted) throw new Error("Login cancelled"); + await sleep(waitMs, signal); + continue; + }The device-code grant is idempotent for the pending case, so a retried poll is safe here. This is unlike the refresh path, where retrying is unsafe.
🤖 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 `@src/oauth/nous.ts` around lines 501 - 553, Update pollForToken to catch transient fetch failures and continue polling until the existing deadline, preserving the current wait interval between retries. Re-throw genuine cancellation from the provided AbortSignal immediately, while allowing request timeout, DNS, connection, and other transport errors to be retried without aborting login; keep the existing OAuth response handling unchanged.
118-125: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winHarden the existing refresh-intent directory at
src/oauth/nous.ts:176-188.
refreshIntentDir()correctly resolves to<config-dir>/.nous-refresh-intent. However,mkdirSync(..., { mode: 0o700 })does not change permissions on an existing directory, andhardenConfigDir()does not hardendir. Re-apply0o700and the platform-specific ACL hardening todiraftermkdirSync.atomicWriteFile()already protects the intent files with0o600; the remaining exposure is directory access and hash-filename listing.🤖 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 `@src/oauth/nous.ts` around lines 118 - 125, Update the refresh-intent directory setup in the function containing mkdirSync for refreshIntentDir so it reapplies 0o700 permissions to the resolved dir after creation and invokes the existing platform-specific ACL hardening on dir, rather than relying on mkdirSync or hardening only the config directory.Source: Path instructions
src/oauth/index.ts (2)
639-649: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftThe generic coordinator now hard-codes
provider === "nous"twice; move the hook ontoOAuthProviderDef.
refreshGenericAccountWithLockis the shared, provider-agnostic refresh path. This change embeds Nous-specific knowledge in it at two sites: the post-persist cleanup at line 639 and the rotated-token recovery at line 660. Both branch on a string literal and call directly into./nous.Two concrete costs:
- The shared routing layer now imports and depends on one provider's implementation module. Per the path instruction for
src/**, changes that bypass the shared routing/config layers are a flagged concern; this is the inverse — provider logic pushed into the shared layer.- Single-use rotating refresh tokens are not unique to Nous. The next provider with the same semantics requires a third and fourth
provider === "x"branch, and each one must independently re-derive the "clear only after durable persist" ordering. That ordering is subtle and easy to get wrong.Express both operations as optional callbacks on
OAuthProviderDef, and have the coordinator invoke them generically:♻️ Proposed refactor: provider-declared rotation hooks
// src/oauth/types.ts export interface OAuthProviderDef { id: string; refresh: (...) => Promise<OAuthCredentials>; + /** + * Called after a rotated credential is durably persisted, with the refresh + * token that was consumed. Best-effort bookkeeping only: the coordinator + * logs and continues if this throws. Providers with single-use rotating + * refresh tokens use this to release their replay guard. + */ + onRotationPersisted?: (consumedRefresh: string) => void; + /** + * Extracts an already-rotated refresh token from a terminal refresh error, + * so the coordinator can preserve it before forcing reauthentication. + */ + rotatedRefreshFromError?: (error: unknown) => string | undefined; }// src/oauth/index.ts — registration nous: { login: (ctrl) => loginNous(ctrl), refresh: (rt, signal) => refreshNousToken(rt, signal), + onRotationPersisted: (consumedRefresh) => clearNousRefreshIntent(consumedRefresh), + rotatedRefreshFromError: (error) => + error instanceof NousTokenError ? error.getRotatedRefresh() : undefined, providerConfig: oauthConfig("nous"), defaultModel: oauthDefaultModel("nous"), },// src/oauth/index.ts — coordinator, post-persist cleanup - if (provider === "nous") { - try { - clearNousRefreshIntent(stored.refresh); - } catch (cleanupErr) { + if (def.onRotationPersisted) { + try { + def.onRotationPersisted(stored.refresh); + } catch (cleanupErr) { logOAuthEvent("OAuth refresh intent cleanup failed (non-fatal)", { provider, accountId, cause: cleanupErr instanceof Error ? cleanupErr.message : String(cleanupErr), }); } }Apply the same substitution in the recovery branch, replacing
provider === "nous" && error instanceof NousTokenErrorwithdef.rotatedRefreshFromError?.(error).The recovery body itself (
preserveNousRotatedRefresh) can stay as-is initially; only the dispatch needs to become generic.Also applies to: 660-695
🤖 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 `@src/oauth/index.ts` around lines 639 - 649, Move Nous-specific refresh-token handling out of refreshGenericAccountWithLock by adding optional post-persist cleanup and rotated-refresh recovery callbacks to OAuthProviderDef. Configure the Nous provider with clearNousRefreshIntent and preserveNousRotatedRefresh, then invoke def’s callbacks generically in both coordinator branches, preserving cleanup-after-durable-persist ordering and the existing recovery behavior.Source: Path instructions
198-208: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEnforce Nous's lazy-only refresh policy
config.providers.nous.refreshPolicyoverridesdefaultRefreshPolicy, and the guardian refreshes any provider whose effective policy is"proactive". SetdefaultRefreshPolicy: "lazy-only"insrc/oauth/index.ts:198-208. If the constraint is absolute, also reject"proactive"for Nous to preventrefresh_token_reusedsession revocation.🤖 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 `@src/oauth/index.ts` around lines 198 - 208, Set Nous’s provider configuration in the `nous` entry to use `defaultRefreshPolicy: "lazy-only"` so guardian refreshes remain lazy-only. If `providerConfig` can override this policy, also enforce the value by rejecting or preventing `"proactive"` for Nous.tests/nous-oauth-live.test.ts (3)
1-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecurity-relevant guards in
src/oauth/nous.tshave no deterministic test coverage.This file is the only new test that targets Nous directly, and it is skipped unless
NOUS_LIVE_TEST=1.tests/oauth-refresh.test.tscovers the coordinator paths well with a mockedfetch, but several guards insrc/oauth/nous.tsare never exercised offline:
resolvePortalBaseUrlrejection branches (lines 240-251): non-HTTPS scheme, embedded credentials, query string, fragment. These prevent sending a single-use refresh token over cleartext or to an attacker-influenced URL. They are pure functions ofprocess.env.NOUS_PORTAL_BASE_URLand are trivial to test.parseRefreshIntentfail-closed behavior (lines 136-155): the{}case, a wrong-typedstatus, and a non-finiteupdatedAtmust all yielduncertain. The docstring calls out{}specifically as the bug being prevented, so a regression here would silently disable the replay guard.pollForTokenbranch handling (lines 527-549):authorization_pending,slow_downinterval escalation,expired_token, andaccess_denied.Per the path instruction for
tests/**, a behavior change insrc/should come with a focused regression test near the existing tests for that subsystem. Please add an offlinetests/nous-oauth.test.tscovering these. TheresolvePortalBaseUrlandparseRefreshIntentcases in particular are a few lines each and protect credential-transmission invariants.I can draft that test file if you want.
🤖 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` around lines 1 - 45, Add an offline focused test file for the Nous OAuth helpers, alongside the existing OAuth tests, without relying on NOUS_LIVE_TEST. Cover resolvePortalBaseUrl rejection of non-HTTPS URLs, embedded credentials, query strings, and fragments; verify parseRefreshIntent returns uncertain for {}, invalid status types, and non-finite updatedAt; and exercise pollForToken handling for authorization_pending, slow_down interval escalation, expired_token, and access_denied using mocked fetch/timing. Keep credentials out of test output and preserve existing production behavior.Source: Path instructions
81-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
NOUS_INFERENCE_BASE_URLinstead of hard-coding the catalog URL.
src/oauth/nous.ts:44exportsNOUS_INFERENCE_BASE_URL = "https://inference-api.nousresearch.com/v1". Line 82 repeats that string literally. The comment on line 81 asserts this is "the same endpoint the adapter uses", but nothing enforces that.If the constant changes, this test keeps hitting the old host and either passes against a stale endpoint or fails with a confusing 404, rather than following the code under test.
♻️ Proposed fix
-import { refreshNousToken } from "../src/oauth/nous"; +import { NOUS_INFERENCE_BASE_URL, refreshNousToken } from "../src/oauth/nous";- const res = await fetch("https://inference-api.nousresearch.com/v1/models", { + const res = await fetch(`${NOUS_INFERENCE_BASE_URL}/models`, { headers: { Authorization: `Bearer ${access}` }, });🤖 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` around lines 81 - 85, Update the live catalog discovery test to import and use NOUS_INFERENCE_BASE_URL from src/oauth/nous.ts when constructing the models endpoint, removing the duplicated hard-coded base URL while preserving the existing authorization header and status assertion.
42-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the invalid
OAuthProviderDefannotation.tests/nous-oauth-live.test.ts:27,42-45imports a type thatsrc/oauth/types.tsdoes not export. The production interface insrc/oauth/index.ts:137-153is private and requireslogin,providerConfig, anddefaultModel. The roottsconfig.jsonexcludestests/, so CI does not catch this error. Use a shared exported type or the registered Nous provider definition.🤖 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` around lines 42 - 45, Replace the invalid OAuthProviderDef annotation in NOUS_DEF with a valid shared exported type or reuse the registered Nous provider definition from the production OAuth implementation; ensure the resulting definition satisfies the required login, providerConfig, and defaultModel fields.Source: Learnings
🤖 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 117: Replace both Kiro shell-pipe entities with the table-safe form
&`#124`; in docs-site/src/content/docs/guides/providers.md:117-117,
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; make no other
changes.
---
Outside diff comments:
In `@src/oauth/index.ts`:
- Around line 639-649: Move Nous-specific refresh-token handling out of
refreshGenericAccountWithLock by adding optional post-persist cleanup and
rotated-refresh recovery callbacks to OAuthProviderDef. Configure the Nous
provider with clearNousRefreshIntent and preserveNousRotatedRefresh, then invoke
def’s callbacks generically in both coordinator branches, preserving
cleanup-after-durable-persist ordering and the existing recovery behavior.
- Around line 198-208: Set Nous’s provider configuration in the `nous` entry to
use `defaultRefreshPolicy: "lazy-only"` so guardian refreshes remain lazy-only.
If `providerConfig` can override this policy, also enforce the value by
rejecting or preventing `"proactive"` for Nous.
In `@src/oauth/nous.ts`:
- Around line 290-295: Update jwtExpiryMs to accept exp only within a plausible
now-relative window, returning undefined for out-of-range values so the caller
falls back to expires_in, and ensure the final skew-adjusted expiry cannot be
negative. In the access-token expiry flow around jwtExpiryMs, replace
DEFAULT_DEVICE_FLOW_TTL_MS with a distinct DEFAULT_ACCESS_TOKEN_TTL_MS constant
used only for access-token lifetime fallback.
- Around line 104-109: Use RefreshIntent.updatedAt to expire stale refresh
intents: define a maximum age based on the refresh token lifetime, have read
logic discard entries older than that threshold, and add
pruneStaleRefreshIntents to remove expired files. Invoke pruning from
writeRefreshIntent after directory creation, but swallow or log pruning failures
so they never block refresh; preserve parseRefreshIntent’s fail-closed handling
for corrupt entries.
- Around line 226-255: Correct the documentation above resolvePortalBaseUrl so
it no longer claims to mirror Hermes’s host allowlist, since the function only
validates HTTPS, credentials, query, fragment, and origin normalization. Keep
the existing validation behavior unchanged and describe only the
HTTPS/default-URL discipline actually implemented.
- Around line 501-553: Update pollForToken to catch transient fetch failures and
continue polling until the existing deadline, preserving the current wait
interval between retries. Re-throw genuine cancellation from the provided
AbortSignal immediately, while allowing request timeout, DNS, connection, and
other transport errors to be retried without aborting login; keep the existing
OAuth response handling unchanged.
- Around line 118-125: Update the refresh-intent directory setup in the function
containing mkdirSync for refreshIntentDir so it reapplies 0o700 permissions to
the resolved dir after creation and invokes the existing platform-specific ACL
hardening on dir, rather than relying on mkdirSync or hardening only the config
directory.
In `@tests/nous-oauth-live.test.ts`:
- Around line 1-45: Add an offline focused test file for the Nous OAuth helpers,
alongside the existing OAuth tests, without relying on NOUS_LIVE_TEST. Cover
resolvePortalBaseUrl rejection of non-HTTPS URLs, embedded credentials, query
strings, and fragments; verify parseRefreshIntent returns uncertain for {},
invalid status types, and non-finite updatedAt; and exercise pollForToken
handling for authorization_pending, slow_down interval escalation,
expired_token, and access_denied using mocked fetch/timing. Keep credentials out
of test output and preserve existing production behavior.
- Around line 81-85: Update the live catalog discovery test to import and use
NOUS_INFERENCE_BASE_URL from src/oauth/nous.ts when constructing the models
endpoint, removing the duplicated hard-coded base URL while preserving the
existing authorization header and status assertion.
- Around line 42-45: Replace the invalid OAuthProviderDef annotation in NOUS_DEF
with a valid shared exported type or reuse the registered Nous provider
definition from the production OAuth implementation; ensure the resulting
definition satisfies the required login, providerConfig, and defaultModel
fields.
🪄 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: bc495d90-556f-4f0d-9810-a6a96e0367ff
📒 Files selected for processing (9)
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.tstests/nous-oauth-live.test.tstests/oauth-refresh.test.ts
| | `kimi` | `openai-chat` | `https://api.kimi.com/coding/v1` | Kimi K2.7/K2.6/K2.5 coding models. | | ||
| | `nous` | `openai-chat` | `https://inference-api.nousresearch.com/v1` | Nous Research subscription gateway (same backend Hermes Agent uses). Device-grant login against `portal.nousresearch.com`; the access token is the per-request inference JWT. Mixed paid + `:free` model catalog (`tencent/hy3:free`, `stepfun/step-3.7-flash:free`, ...) discovered live from the signed-in account. Refresh tokens are single-use and rotated on every refresh. | | ||
| | `kiro` | `kiro` | `https://runtime.us-east-1.kiro.dev` | Initial login imports the installed, signed-in `kiro-cli` session (on Unix, install with `curl -fsSL https://cli.kiro.dev/install | bash`; on Windows PowerShell, use `irm 'https://cli.kiro.dev/install.ps1' | iex`; then run `kiro-cli login`). **Add account** logs `kiro-cli` out, starts a fresh browser login that switches the account used by `kiro-cli`, and stores account-scoped profile metadata. Existing OpenCodex accounts are preserved, and cancellation or failure restores the previous `kiro-cli` session. | | ||
| | `kiro` | `kiro` | `https://runtime.us-east-1.kiro.dev` | Initial login imports the installed, signed-in `kiro-cli` session (on Unix, install with `curl -fsSL https://cli.kiro.dev/install | bash`; on Windows PowerShell, use `irm 'https://cli.kiro.dev/install.ps1' | iex`; then run `kiro-cli login`). **Add account** logs `kiro-cli` out, starts a fresh browser login that switches the account used by `kiro-cli`, and stores account-scoped profile metadata. Existing OpenCodex accounts are preserved, and cancellation or failure restores the previous `kiro-cli` session. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one table-safe entity form in every locale.
All five Kiro table examples use |, but the required source form is &#124;.
docs-site/src/content/docs/guides/providers.md#L117-L117: replace both Kiro pipe entities.docs-site/src/content/docs/ja/guides/providers.md#L112-L112: replace both Kiro pipe entities.docs-site/src/content/docs/ko/guides/providers.md#L111-L111: replace both Kiro pipe entities.docs-site/src/content/docs/ru/guides/providers.md#L121-L121: replace both Kiro pipe entities.docs-site/src/content/docs/zh-cn/guides/providers.md#L102-L102: replace both Kiro pipe entities.
As per path instructions, Kiro shell examples in docs-site/** must use &#124; so Markdown tables render correctly, including localized copies.
📍 Affects 5 files
docs-site/src/content/docs/guides/providers.md#L117-L117(this comment)docs-site/src/content/docs/ja/guides/providers.md#L112-L112docs-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/guides/providers.md` at line 117, Replace both
Kiro shell-pipe entities with the table-safe form &`#124`; in
docs-site/src/content/docs/guides/providers.md:117-117,
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; make no other
changes.
Source: Path instructions
…eview - 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.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/oauth/nous.ts (1)
418-426: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply missing-access-token validation to device login.
The new terminal
invalid_tokenbranch does not run for a successful device-flow response withoutaccess_token. At Line 567,pollForTokencallsparseTokenPayloadonly whenaccess_tokenis already non-empty. A 200 response without that field instead reachestokenErrorFromPayload, so it does not receive the required terminalinvalid_tokenclassification.Call
parseTokenPayloadfor every successful token response. Add a device-flow regression test with a 200 response that omitsaccess_token.Proposed fix
- if (response.ok && nonEmptyString(payload.access_token)) return parseTokenPayload(payload, ""); + if (response.ok) return parseTokenPayload(payload, "");🤖 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 `@src/oauth/nous.ts` around lines 418 - 426, Update the device-flow logic in pollForToken so every successful token response is passed to parseTokenPayload, including responses missing access_token, allowing the terminal invalid_token NousTokenError to be raised. Add a regression test for a 200 response without access_token and verify it receives the terminal invalid_token classification.
🤖 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 `@src/oauth/nous.ts`:
- Around line 188-190: Update writeRefreshIntent so a chmodSync(dir, 0o700)
failure is propagated or returned as an operation failure, preventing
atomicWriteFile from running. Remove the swallowed best-effort catch while
preserving the existing hardened-directory write flow when chmod succeeds.
- Around line 541-563: Update the transport-error retry path around the
device-flow polling loop to cap the sleep duration at the remaining time until
the deadline. Use the existing deadline and waitMs values when calling sleep,
while preserving cancellation handling and retry behavior before expiration.
---
Outside diff comments:
In `@src/oauth/nous.ts`:
- Around line 418-426: Update the device-flow logic in pollForToken so every
successful token response is passed to parseTokenPayload, including responses
missing access_token, allowing the terminal invalid_token NousTokenError to be
raised. Add a regression test for a 200 response without access_token and verify
it receives the terminal invalid_token classification.
🪄 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: 7b1820bc-b5d0-492e-9cc8-fdbefb024b9e
📒 Files selected for processing (4)
src/oauth/index.tssrc/oauth/nous.tstests/nous-oauth-live.test.tstests/nous-oauth.test.ts
…e-flow deadline, classify missing access_token as terminal
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/oauth/nous.ts (2)
420-447: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve replacement refresh tokens when
access_tokenis missing.When a refresh response contains a replacement
refresh_tokenbut noaccess_token, this early error path does not carry the replacement token. The refresh coordinator insrc/oauth/index.tsLines 606-707 can preserve rotated credentials only throughNousTokenError.getRotatedRefresh(). It will otherwise mark the old generation for reauthentication and discard the replacement token.Keep the error terminal, but propagate a non-empty replacement refresh token through the error's rotated-refresh field or equivalent. Add a refresh-path regression test in
tests/nous-oauth.test.ts.🤖 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 `@src/oauth/nous.ts` around lines 420 - 447, The access-token validation in the Nous token-response handling must preserve a non-empty replacement refresh token when throwing the terminal NousTokenError. Pass the parsed refresh token through the error’s rotated-refresh field or equivalent so the refresh coordinator can retain rotated credentials, while leaving the terminal classification unchanged. Add a regression test covering a refresh response with refresh_token but no access_token in the Nous OAuth tests.
306-313: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not revive an expired access token through the fallback lifetime.
jwtExpiryMsreturnsundefinedfor an expiredexpclaim.parseTokenPayloadthen treats the claim as absent and usesexpires_inorDEFAULT_ACCESS_TOKEN_TTL_MS. An expired JWT can therefore be stored as usable.The
expires_incheck also accepts non-finite numeric values. An overflowed value can produceexpires = Infinityand prevent refresh indefinitely.Keep expired claims distinct from absent claims. Reject them or mark the credential expired. Validate
expires_inas finite, non-negative, and bounded before using it. Add tests for expiredexpvalues and overflowingexpires_invalues.Also applies to: 465-467
🤖 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 `@src/oauth/nous.ts` around lines 306 - 313, The token parsing flow around jwtExpiryMs and parseTokenPayload must distinguish an expired exp claim from an absent claim so expired credentials cannot fall back to expires_in or DEFAULT_ACCESS_TOKEN_TTL_MS; reject them or mark them expired. Validate expires_in as finite, non-negative, and within the supported lifetime bound before calculating expiry, including overflow cases, and add coverage for expired exp and overflowing expires_in values.
🤖 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 `@src/oauth/nous.ts`:
- Around line 563-566: Update the device-flow polling logic around the
transport-error, authorization_pending, and slow_down retry paths to use a
shared deadline-aware sleep helper that caps every wait by the remaining
deadline. After each fetch completes, recheck the deadline before accepting a
successful response so delayed responses cannot return credentials. Add focused
tests covering pending-response deadline handling and responses delayed past the
deadline.
- Around line 570-571: Normalize the parsed response in the token exchange flow
before calling parseTokenPayload: require a non-null object/record and replace
invalid values such as null with the existing empty-payload fallback. Preserve
successful object parsing and ensure invalid successful bodies produce the
intended NousTokenError; add a regression test covering a null response body.
---
Outside diff comments:
In `@src/oauth/nous.ts`:
- Around line 420-447: The access-token validation in the Nous token-response
handling must preserve a non-empty replacement refresh token when throwing the
terminal NousTokenError. Pass the parsed refresh token through the error’s
rotated-refresh field or equivalent so the refresh coordinator can retain
rotated credentials, while leaving the terminal classification unchanged. Add a
regression test covering a refresh response with refresh_token but no
access_token in the Nous OAuth tests.
- Around line 306-313: The token parsing flow around jwtExpiryMs and
parseTokenPayload must distinguish an expired exp claim from an absent claim so
expired credentials cannot fall back to expires_in or
DEFAULT_ACCESS_TOKEN_TTL_MS; reject them or mark them expired. Validate
expires_in as finite, non-negative, and within the supported lifetime bound
before calculating expiry, including overflow cases, and add coverage for
expired exp and overflowing expires_in values.
🪄 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: cfe7cbae-e52a-448c-8d09-162b1939fe74
📒 Files selected for processing (2)
src/oauth/nous.tstests/nous-oauth.test.ts
… tables A bare &lidge-jun#124; 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.
…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.
2 participants
Follow-up to #1397 (Nous Portal OAuth provider). This branch is #1397's head
(
de819e6c4) plus one commit; merge after #1397 so the diff here collapses tojust the incremental fix.
What
Two small fixes that could not be pushed to the PR #1397 fork head (maintainer edits were disabled):
|characters inside the Kiro CLI install commands broke the markdown tables in the ja/ko/ru/zh-cn provider guides. Replaced with the table-safe|entity, matching the English row content.tests/nous-oauth-live.test.tscalledgetCredential("nous", accountId)with an excess argument (a type error). It now reads the refreshed account withgetAccountCredential("nous", stored!.accountId!).Verification
bun run typecheck✅bun run privacy:scan✅bun test tests/nous-oauth-live.test.ts✅ (opt-in live test skipped as expected)Intended to merge after #1397. Once #1397 lands on
dev, this PR's diffreduces to the single incremental commit above.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation