@qwen-code /triage

Thanks for the PR, @tt-a1i!

Template: the PR body uses custom headings ("Summary", "Test plan") instead of the template sections. Missing: "Why it's needed", structured "Reviewer Test Plan" (How to verify / Evidence / Tested on table), "Risk & Scope", and "中文说明". The essentials are there — "Fixes #5355" links the issue, and the test commands are listed — but please align with the template so reviewers get the context they need. Not blocking the review on this, but worth fixing.

Direction: clearly aligned. Issue #5355 describes a real correctness bug — expires_in=0 silently treated as non-expiring, and parseInt accepting malformed values like "3600abc". This is a straightforward MCP OAuth bug that affects token lifecycle. No CHANGELOG reference needed for a fix this scoped.

Approach: minimal and focused. Extract a parseExpiresIn helper, apply it consistently across both parse paths (JSON + form-urlencoded) and both call sites (authenticate + refreshAccessToken), preserve 0 as valid, reject malformed strings. +215 is mostly tests (5 new test cases), which is the right ratio. No scope creep.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal (before reading the diff): given the bug — if (tokenResponse.expires_in) drops 0, and parseInt is too lenient — I'd extract a small validator that (a) treats undefined/null as "no expiry", (b) accepts 0 as valid, (c) rejects non-numeric strings. Then replace every expires_in consumption site with it. Two files max: the provider and its tests.

Comparison: the PR does exactly this. parseExpiresIn handles all three cases cleanly: undefined/null → undefined, digits-only string → Number(), anything else → NaN → throw. normalizeTokenResponse wraps it for the parse-response path. The re-parse in authenticate() and getValidToken() via parseExpiresIn(tokenResponse.expires_in) is redundant (already parsed) but harmless — idempotent on numbers.

No correctness bugs, no security issues, no AGENTS.md violations. The error re-throw in the JSON catch block (checking error.message.startsWith('Invalid expires_in value')) is slightly fragile but functional and only needed to prevent the JSON parse error handler from swallowing validation errors.

Test Results

$ cd packages/core && npx vitest run src/mcp/oauth-provider.test.ts

 RUN  v3.2.4 packages/core

 ✓ src/mcp/oauth-provider.test.ts (27 tests) 188ms

 Test Files  1 passed (1)
      Tests  27 passed (27)
   Duration  4.40s

$ npx tsc --noEmit --project packages/core/tsconfig.json
(clean — no output)

$ npx eslint packages/core/src/mcp/oauth-provider.ts
(clean — no output)

All 27 tests pass (22 existing + 5 new). Typecheck and lint clean.

Real-Scenario Testing

N/A for this fix — the expires_in parsing is internal token-handling logic, not user-observable at the TUI level. Reproducing it would require a live MCP server configured to return expires_in=0, which isn't feasible in a CLI smoke test. Unit tests are the correct verification layer here and they cover all edge cases: expires_in=0 in both auth and refresh flows, JSON string "3600", malformed "3600abc" in both JSON and form-urlencoded formats, and expires_in=0 during getValidToken refresh.

— Qwen Code · qwen3.7-max

This is a clean, well-scoped bug fix. The parseExpiresIn helper is exactly what the code needed — it replaces a falsy check that dropped 0 and a parseInt that accepted garbage, with a single function that gets both right. The diff is minimal (two files, mostly tests), follows project conventions, and every new test targets a specific edge case from the issue.

My independent proposal matched the PR's approach almost exactly, which gives me confidence this is the right solution. The only minor observation is that parseExpiresIn gets called redundantly on already-parsed values in authenticate() and getValidToken() — harmless, and arguably defensive in a good way.

Template compliance should be tightened up (see Stage 1), but that's cosmetic. The substance is solid.

Approving. ✅

— Qwen Code · qwen3.7-max

✅ Local runtime verification (real tmux, live OAuth token server) — PR #5356

Verdict: fixes #5355 as intended, no regressions, type-checks & lints clean — safe to merge.

I drove the real MCPOAuthProvider against a real local HTTP token endpoint (no fetch/http mocks) plus the real MCPOAuthTokenStorage (plain-JSON backend, HOME redirected to a throwaway dir), inside an isolated tmux session. A/B baseline is origin/main (the PR is based on current main — 0 commits behind, clean apply).

What the PR does

Adds parseExpiresIn: accepts a non-negative safe integer (or a ^\d+$ string), otherwise throws Invalid expires_in value. It runs through normalizeTokenResponse on JSON responses and directly on the form-urlencoded path (replacing parseInt), and the token-save guard changes from truthy if (expires_in) to if (expiresIn !== undefined) so expires_in=0 produces an immediate expiresAt instead of being dropped.

Evidence — PR head

Static checks

vitest oauth-provider.test.ts → 27 passed (5 new)   VITEST_EXIT=0
eslint (both changed files)   → ESLINT_EXIT=0
typecheck core (tsc --noEmit) → TYPECHECK_CORE_EXIT=0

Real runtime — live token endpoint http://127.0.0.1:<port>/token:

refreshAccessToken (real HTTP):
  [json_number_3600] OK    expires_in=3600   (type=number)
  [json_string_3600] OK    expires_in=3600   (type=number)   <- "3600" normalized
  [json_bad_3600abc] THROW Invalid expires_in value: 3600abc
  [form_bad_3600abc] THROW Invalid expires_in value: 3600abc

getValidToken + expires_in=0 (real storage, refresh an expired token):
  saved_token_expiresAt        = <≈ Date.now()>
  EXPIRESAT_SET                = true
  EXPIRESAT_IS_IMMEDIATE       = true
  ISTOKENEXPIRED_AFTER_REFRESH = true          <- correctly expired, not "valid forever"

A/B vs origin/main (same harness, same live server)

                              PR head                 origin/main (base)
refresh json string "3600"    3600  (number)          "3600" (string, not normalized)
refresh json  "3600abc"       THROW                   "3600abc" (string, silently accepted)
refresh form  "3600abc"       THROW                   3600   (parseInt truncated, accepted)
getValidToken expires_in=0:
  saved expiresAt             ≈ now (set)              undefined  (dropped)
  isTokenExpired(saved)       true                     false      <- non-expiring (#5355 bug)

The 5 new tests have teeth — all fail on base:

× authenticate      > should preserve expires_in=0 as an immediate expiry
× refreshAccessToken > should normalize JSON string expires_in values
× refreshAccessToken > should reject malformed JSON expires_in values
× refreshAccessToken > should reject malformed form-urlencoded expires_in values
× getValidToken     > should preserve expires_in=0 when refreshing an expired token
Tests 5 failed | 22 skipped (27)   TEETH_VITEST_EXIT=1

Observations (non-blocking)

• Both response paths reject malformed values consistently: a non-JSON body fails JSON.parse (a SyntaxError, not Invalid expires_in value) and falls through to the form-urlencoded branch, where parseExpiresIn is applied directly — confirmed by form_bad_3600abc throwing.
• The JSON path distinguishes a parseExpiresIn rejection from a genuine JSON parse error via error.message.startsWith('Invalid expires_in value'). It works (the message is internal/controlled); just note it's a string-based discriminator.
• ^\d+$ + Number.isSafeInteger also rejects negatives, decimals (e.g. a JSON expires_in: 3.5), and oversized values — a reasonable tightening.
• Verified on Linux.

vitest oauth-provider.test.ts → 27 passed（5 个新增）  VITEST_EXIT=0
eslint（两个改动文件）        → ESLINT_EXIT=0
typecheck core (tsc --noEmit) → TYPECHECK_CORE_EXIT=0

refreshAccessToken（真实 HTTP）:
  [json_number_3600] OK    expires_in=3600   (type=number)
  [json_string_3600] OK    expires_in=3600   (type=number)   <- 字符串 "3600" 被规范化
  [json_bad_3600abc] THROW Invalid expires_in value: 3600abc
  [form_bad_3600abc] THROW Invalid expires_in value: 3600abc

getValidToken + expires_in=0（真实存储，刷新一个已过期的 token）:
  saved_token_expiresAt        = <≈ Date.now()>
  EXPIRESAT_SET                = true
  EXPIRESAT_IS_IMMEDIATE       = true
  ISTOKENEXPIRED_AFTER_REFRESH = true          <- 正确地判定为已过期，而非“永久有效”

                              PR head                 origin/main (base)
refresh json 字符串 "3600"     3600（number）          "3600"（string，未规范化）
refresh json  "3600abc"       THROW                   "3600abc"（string，被静默接受）
refresh form  "3600abc"       THROW                   3600（parseInt 截断后被接受）
getValidToken expires_in=0:
  保存的 expiresAt             ≈ now（已设置）          undefined（被丢弃）
  isTokenExpired(saved)       true                     false      <- 永不过期（即 #5355 bug）

× authenticate      > should preserve expires_in=0 as an immediate expiry
× refreshAccessToken > should normalize JSON string expires_in values
× refreshAccessToken > should reject malformed JSON expires_in values
× refreshAccessToken > should reject malformed form-urlencoded expires_in values
× getValidToken     > should preserve expires_in=0 when refreshing an expired token
Tests 5 failed | 22 skipped (27)   TEETH_VITEST_EXIT=1

✅ Local verification report — PR #5356 fix(core): validate oauth expires_in values

Verdict: strong LGTM. This is a correct, well-scoped fix for a real OAuth bug. The new tests genuinely catch the bugs, the new parseExpiresIn parser is robust across a wide edge-case sweep, and there are no regressions or type/lint/format issues. One minor behavioral note for your awareness (not a blocker).

What it fixes

1. expires_in=0 dropped — main used a falsy check (if (tokenResponse.expires_in)), so an immediate-expiry 0 left expiresAt unset → the token was treated as never expiring and never refreshed. The PR sets expiresAt = now.
2. Silent parseInt truncation — main did parseInt(expiresIn, 10), so '3600abc' → 3600, '3.5' → 3, '0x10' → 0, '1e3' → 1 were silently accepted. The PR validates with a strict /^\d+$/ parser and throws Invalid expires_in value.
3. Un-normalized JSON values — a JSON expires_in: '3600' (string) stayed a string on main; the PR routes both JSON and form-urlencoded responses through one parseExpiresIn so the value is always a normalized number (or undefined).

How it was verified

Real vitest under tmux in two isolated worktrees: AFTER = 3457f4e8 (PR head), BEFORE = a4797b9a (origin/main), repo node_modules symlinked. Node v22.22.2, vitest 3.2.4. Merges cleanly; target files identical between merge-base and main.

Genuine-regression detail (base failure signatures)

× preserve expires_in=0 as an immediate expiry      → expected undefined to be 1700000000000
× normalize JSON string expires_in values           → expected '3600' to be 3600
× reject malformed JSON expires_in values           → promise resolved instead of rejecting
× reject malformed form-urlencoded expires_in values→ promise resolved instead of rejecting
× preserve expires_in=0 when refreshing             → expiresAt not set

Adversarial parser sweep (PR vs base)

The new parser handles every edge case correctly, where main's parseInt silently corrupted many:

Minor note (non-blocking)

An empty expires_in= in a form-urlencoded response now throws (Invalid expires_in value:), whereas main silently treated it as undefined (no expiry). An empty value is non-conformant (RFC 6749 says expires_in is optional → a compliant server omits it entirely, which still yields undefined correctly). So this is a defensible fail-closed tightening, but it does mean a buggy server that emits an explicit empty expires_in= would now have its token rejected rather than treated as non-expiring. Worth a mention; not a blocker.

Reproduce

git worktree add --detach /tmp/wt-after  $(gh pr view 5356 --json headRefOid -q .headRefOid)
git worktree add --detach /tmp/wt-before origin/main
for w in wt-after wt-before; do ln -s "$PWD/node_modules" /tmp/$w/node_modules; \
  ln -s "$PWD/packages/core/node_modules" /tmp/$w/packages/core/node_modules; done
(cd /tmp/wt-after/packages/core && ../../node_modules/.bin/vitest run --coverage.enabled=false src/mcp/oauth-provider.test.ts)  # 27/27
# Catch the bug: copy PR's test onto BASE source -> 5 fail
cp /tmp/wt-after/packages/core/src/mcp/oauth-provider.test.ts /tmp/wt-before/packages/core/src/mcp/oauth-provider.test.ts
(cd /tmp/wt-before/packages/core && ../../node_modules/.bin/vitest run --coverage.enabled=false src/mcp/oauth-provider.test.ts)

× expires_in=0 立即过期            → expected undefined to be 1700000000000
× 归一化 JSON 字符串 expires_in     → expected '3600' to be 3600
× 拒绝非法 JSON expires_in          → promise 被 resolve 而非 reject
× 拒绝非法 form-urlencoded expires_in→ promise 被 resolve 而非 reject
× 刷新时保留 expires_in=0           → expiresAt 未设置

_{Verified locally with real vitest under tmux on isolated worktrees of the PR head and origin/main; bug reality proven by running the PR's tests against base source; the parseExpiresIn parser audited with a 26-input adversarial sweep against the old parseInt behavior.}