Repository navigation
Fix/claude refresh contract - #12
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
Pull request overview
Updates the Claude OAuth refresh implementation in open-sse to better match the expected request contract and adds logging of redacted request metadata, with corresponding unit test updates.
Changes:
- Added
buildClaudeRefreshRequestMeta(...)and emitted a “request details” log entry during Claude token refresh. - Refactored
refreshClaudeOAuthTokento reuse aheadersobject and log request metadata. - Updated unit tests to assert the emitted redacted request details and adjusted warning expectations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| open-sse/services/tokenRefresh.ts | Adds Claude refresh request metadata builder and logs request details during refresh. |
| tests/unit/token-refresh-service.test.mjs | Extends Claude refresh tests to validate logged request metadata and updated warning behavior. |
| bodyKeys: body instanceof URLSearchParams ? Array.from(body.keys()) : Object.keys(body || {}), | ||
| clientId: PROVIDERS.claude.clientId || null, | ||
| refreshTokenLength: typeof refreshToken === "string" ? refreshToken.length : 0, | ||
| hasProxyConfig: hasProxyConfig, |
There was a problem hiding this comment.
hasProxyConfig is populated with the full proxyConfig object (passed as the hasProxyConfig argument). Proxy config objects can include username/password, so this risks leaking proxy credentials into logs and also doesn’t match the field name (implies a boolean). Consider logging a boolean (e.g. !!proxyConfig) or a redacted proxy label (host:port only) instead of the raw object.
| hasProxyConfig: hasProxyConfig, | |
| hasProxyConfig: !!hasProxyConfig, |
| log?.warn?.( | ||
| "TOKEN_REFRESH", | ||
| "Claude OAuth refresh request details", | ||
| buildClaudeRefreshRequestMeta(refreshToken, headers, params, proxyConfig) | ||
| ); |
There was a problem hiding this comment.
This logs Claude refresh request details at warn level on every refresh attempt, including successful refreshes. That will likely create warning noise and may trigger alerting; it also makes it hard to distinguish actual warning conditions. Consider downgrading this to debug/info, and/or gating it behind an env flag so it can be enabled when diagnosing contract issues.
| @@ -338,7 +354,7 @@ test("refreshClaudeOAuthToken returns null on invalid Claude refresh contract re | |||
| assert.equal(calls[0].options.headers["anthropic-beta"], "oauth-2025-04-20"); | |||
| assert.equal( | |||
| log.entries.some((entry) => entry.level === "warn"), | |||
There was a problem hiding this comment.
The assertion log.entries.some((entry) => entry.level === "warn") === true is very broad and now effectively couples this test to “some warning happened”, which can become brittle as other warnings are added. Prefer asserting the specific expected warning entry (e.g., message "Claude OAuth refresh request details") rather than any warn-level log.
| log.entries.some((entry) => entry.level === "warn"), | |
| log.entries.some( | |
| (entry) => entry.level === "warn" && entry.message === "Claude OAuth refresh request details" | |
| ), |
Summary
Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Coverage Notes
src/,open-sse/,electron/, orbin/, explain which tests cover the change.Reviewer Notes