This repository was archived by the owner on Aug 25, 2026. It is now read-only.
feat: port upstream core correctness fixes for Echadron - #15
Merged
Merged
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes isolate builtin agent profiles per session, improve MCP structured-result and OAuth handling, and add UTF-16 detection and transcoding to the v2 read tool. Both agent-core packages receive MCP updates, with tests and changesets covering the behavior. ChangesSession profile isolation
MCP correctness
UTF text file reading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)MCP structured result flowsequenceDiagram
participant MCPClient
participant toMcpToolResult
participant mcpResultToExecutableOutput
participant ModelOutput
MCPClient->>toMcpToolResult: provide structuredContent and _meta
toMcpToolResult->>mcpResultToExecutableOutput: return normalized MCPToolResult
mcpResultToExecutableOutput->>ModelOutput: append filtered structured-result text
OAuth registration recovery flowsequenceDiagram
participant beginAuthorization
participant McpOAuthClientProvider
participant OAuthCredentialStore
participant OAuthSDK
beginAuthorization->>McpOAuthClientProvider: validate callback redirect URI
McpOAuthClientProvider->>OAuthCredentialStore: load and invalidate stale registration
beginAuthorization->>OAuthSDK: start authorization flow
UTF text read flowsequenceDiagram
participant ReadTool
participant FileSystem
participant detectTextEncoding
participant decodeUtfText
participant readForward
ReadTool->>FileSystem: read sample and file bytes
ReadTool->>detectTextEncoding: detect UTF encoding
ReadTool->>decodeUtfText: decode supported non-UTF-8 bytes
ReadTool->>readForward: provide shared decoded line stream and encoding
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/agent-core/test/profile/agentfile.test.ts (1)
277-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the regression test to cover snapshot restoration and nested profile state.
This test covers constructor cloning only. The changed
restoreSnapshotpath is atpackages/agent-core/src/profile/agentfile/catalog.tsLine 150. Add a restore case that leaves builtin profiles active, then mutate a nested profile'stoolsordisallowedTools. Verify that the mutation does not reach another catalog orDEFAULT_AGENT_PROFILES.As per coding guidelines, prefer extending the existing test file for this module instead of adding a separate test file.
🤖 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 `@packages/agent-core/test/profile/agentfile.test.ts` around lines 277 - 298, Extend the existing “keeps builtin profile state isolated between catalogs” test to exercise restoreSnapshot while builtin profiles remain active. Restore a snapshot for one catalog, mutate a nested builtin profile’s tools or disallowedTools afterward, and assert the mutation is absent from the other catalog and DEFAULT_AGENT_PROFILES.Source: Coding guidelines
🤖 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 `@packages/agent-core-v2/src/_base/text/encoding.ts`:
- Around line 1-14: Update the top-of-file module header in encoding.ts to begin
with the required <domain> domain (Ln) — <one-line role>. format while retaining
the module’s responsibility description. Remove the separate JSDoc comment
immediately before the detection logic, keeping comments solely in the top-level
/** */ header.
In `@packages/agent-core-v2/src/agent/mcp/oauth/service.ts`:
- Line 104: Update the OAuth authorization setup in
packages/agent-core-v2/src/agent/mcp/oauth/service.ts at lines 104-104 and
packages/agent-core/src/mcp/oauth/service.ts at lines 129-129 so
invalidateStaleRegistration runs inside the existing guarded cleanup path that
closes callbackServer and preserves wrapAuthError handling; do not leave either
invalidation call outside that protected flow.
In `@packages/agent-core-v2/src/agent/mcp/output.ts`:
- Around line 192-199: Update serializeStructuredExtras in
packages/agent-core-v2/src/agent/mcp/output.ts (lines 192-199) and its
counterpart in packages/agent-core/src/mcp/output.ts (lines 246-253) to escape
the closing delimiter in serialized JSON rather than remove it, preserving
structured string values. Add matching regression cases in
packages/agent-core-v2/test/agent/mcp/output.test.ts (lines 284-302) and
packages/agent-core/test/mcp/output.test.ts (lines 286-304) using a structured
string containing the closing delimiter and asserting it is preserved.
In `@packages/agent-core-v2/src/agent/tools/os/read/read.md`:
- Line 11: Update the read.md documentation sentence describing refused files to
refer to decoded text containing NUL characters rather than raw NUL bytes, while
preserving the existing UTF-16 detection and transcoding behavior.
In `@packages/agent-core-v2/src/agent/tools/os/read/readTool.ts`:
- Around line 293-304: Update the non-UTF-8 transcoding branch around readTool’s
decodeUtfText call to read at most TRANSCODE_MAX_BYTES + 1 bytes, then reject
when the returned buffer length exceeds TRANSCODE_MAX_BYTES. Use the read buffer
for decoding and retain the existing error response and encoding handling.
In `@packages/agent-core-v2/test/agent/mcp/oauth/store.test.ts`:
- Around line 149-180: Update both tests around invalidateStaleRegistration to
keep createMemoryMcpOAuthStore() in a local store variable, then instantiate a
second McpOAuthClientProvider with that same store after invalidation. Assert
the second provider returns undefined for the changed callback URI, and retains
the cached client information for the matching URI, verifying persistence beyond
the first provider’s clientCache.
In `@packages/agent-core/src/mcp/oauth/provider.ts`:
- Around line 156-162: Update invalidateStaleRegistration in
packages/agent-core/src/mcp/oauth/provider.ts (lines 156-162) and
packages/agent-core-v2/src/agent/mcp/oauth/provider.ts (lines 165-171) to verify
the value returned by clientInformation() is non-null and an object before
applying the 'redirect_uris' in info check; preserve the existing false return
for invalid or missing client information and the remaining URI validation
behavior.
---
Nitpick comments:
In `@packages/agent-core/test/profile/agentfile.test.ts`:
- Around line 277-298: Extend the existing “keeps builtin profile state isolated
between catalogs” test to exercise restoreSnapshot while builtin profiles remain
active. Restore a snapshot for one catalog, mutate a nested builtin profile’s
tools or disallowedTools afterward, and assert the mutation is absent from the
other catalog and DEFAULT_AGENT_PROFILES.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80d50a25-30b5-4fd0-86ce-6c2e844748e8
📒 Files selected for processing (25)
.changeset/session-profile-isolation.md.changeset/upstream-core-correctness.mdpackages/agent-core-v2/src/_base/text/encoding.tspackages/agent-core-v2/src/_base/text/line-endings.tspackages/agent-core-v2/src/agent/mcp/client-shared.tspackages/agent-core-v2/src/agent/mcp/oauth/provider.tspackages/agent-core-v2/src/agent/mcp/oauth/service.tspackages/agent-core-v2/src/agent/mcp/output.tspackages/agent-core-v2/src/agent/mcp/types.tspackages/agent-core-v2/src/agent/tools/os/read/read.mdpackages/agent-core-v2/src/agent/tools/os/read/read.tspackages/agent-core-v2/src/agent/tools/os/read/readTool.tspackages/agent-core-v2/test/_base/text/encoding.test.tspackages/agent-core-v2/test/agent/mcp/oauth/store.test.tspackages/agent-core-v2/test/agent/mcp/output.test.tspackages/agent-core-v2/test/os/backends/node-local/tools/read.test.tspackages/agent-core/src/mcp/client-shared.tspackages/agent-core/src/mcp/oauth/provider.tspackages/agent-core/src/mcp/oauth/service.tspackages/agent-core/src/mcp/output.tspackages/agent-core/src/mcp/types.tspackages/agent-core/src/profile/agentfile/catalog.tspackages/agent-core/test/mcp/oauth-store.test.tspackages/agent-core/test/mcp/output.test.tspackages/agent-core/test/profile/agentfile.test.ts
Comment on lines
+1
to
+14
| /** Pure helpers for detecting and decoding UTF text files. */ | ||
|
|
||
| export type UtfTextEncoding = 'utf-8' | 'utf-16le' | 'utf-16be'; | ||
|
|
||
| export interface TextEncodingDetection { | ||
| readonly encoding: UtfTextEncoding; | ||
| readonly seemsBinary: boolean; | ||
| } | ||
|
|
||
| export const ENCODING_DETECTION_SAMPLE_BYTES = 512; | ||
|
|
||
| const MIN_ZERO_BYTES_FOR_UTF16 = 2; | ||
|
|
||
| /** Detect UTF-8/UTF-16 from BOMs or a conservative BOM-less zero-byte pattern. */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required module header format.
Update the top-of-file header to start with the required `<domain>` domain (Ln) — <one-line role>. format. Remove the function JSDoc on Line 14. Keep its responsibility description in the module header.
As per coding guidelines, “Keep comments solely in a top-of-file /** */ block” and “Start the header comment with `<domain>` domain (Ln) — <one-line role>.”.
🤖 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 `@packages/agent-core-v2/src/_base/text/encoding.ts` around lines 1 - 14,
Update the top-of-file module header in encoding.ts to begin with the required
<domain> domain (Ln) — <one-line role>. format while retaining the module’s
responsibility description. Remove the separate JSDoc comment immediately before
the detection logic, keeping comments solely in the top-level /** */ header.
Source: Coding guidelines
|
|
||
| provider.setRedirectUrl(new URL(callbackServer.redirectUri)); | ||
| await provider.ready; | ||
| await provider.invalidateStaleRegistration(callbackServer.redirectUri); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the callback server when registration invalidation fails.
invalidateStaleRegistration can fail during credential removal. Both calls run outside the existing cleanup block, so that failure leaves the callback server open and bypasses wrapAuthError. Move the call into the guarded authorization setup, or close and reset the flow before rethrowing.
packages/agent-core-v2/src/agent/mcp/oauth/service.ts#L104-L104: Include invalidation in the cleanup path that closescallbackServer.packages/agent-core/src/mcp/oauth/service.ts#L129-L129: Include invalidation in the cleanup path that closescallbackServer.
📍 Affects 2 files
packages/agent-core-v2/src/agent/mcp/oauth/service.ts#L104-L104(this comment)packages/agent-core/src/mcp/oauth/service.ts#L129-L129
🤖 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 `@packages/agent-core-v2/src/agent/mcp/oauth/service.ts` at line 104, Update
the OAuth authorization setup in
packages/agent-core-v2/src/agent/mcp/oauth/service.ts at lines 104-104 and
packages/agent-core/src/mcp/oauth/service.ts at lines 129-129 so
invalidateStaleRegistration runs inside the existing guarded cleanup path that
closes callbackServer and preserves wrapAuthError handling; do not leave either
invalidation call outside that protected flow.
Comment on lines
+192
to
+199
| function serializeStructuredExtras(extras: Record<string, unknown>): string | undefined { | ||
| if (Object.keys(extras).length === 0) return undefined; | ||
| try { | ||
| return JSON.stringify(extras).replaceAll('</mcp-structured-result>', ''); | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve closing-tag text in structured values.
Line 195 deletes valid structured data. For example, before</mcp-structured-result>after becomes beforeafter. Escape the delimiter in the serialized JSON instead of deleting it.
packages/agent-core-v2/src/agent/mcp/output.ts#L192-L199: escape the closing delimiter while preserving the JSON value.packages/agent-core/src/mcp/output.ts#L246-L253: apply the same serializer fix.packages/agent-core-v2/test/agent/mcp/output.test.ts#L284-L302: add a regression case with a structured string that contains the closing delimiter.packages/agent-core/test/mcp/output.test.ts#L286-L304: add the matching regression case.
Proposed serializer fix
- return JSON.stringify(extras).replaceAll('</mcp-structured-result>', '');
+ return JSON.stringify(extras).replaceAll(
+ '</mcp-structured-result>',
+ '<\\/mcp-structured-result>',
+ );📝 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.
Suggested change
| function serializeStructuredExtras(extras: Record<string, unknown>): string | undefined { | |
| if (Object.keys(extras).length === 0) return undefined; | |
| try { | |
| return JSON.stringify(extras).replaceAll('</mcp-structured-result>', ''); | |
| } catch { | |
| return undefined; | |
| } | |
| } | |
| function serializeStructuredExtras(extras: Record<string, unknown>): string | undefined { | |
| if (Object.keys(extras).length === 0) return undefined; | |
| try { | |
| return JSON.stringify(extras).replaceAll( | |
| '</mcp-structured-result>', | |
| '<\/mcp-structured-result>', | |
| ); | |
| } catch { | |
| return undefined; | |
| } | |
| } |
📍 Affects 4 files
packages/agent-core-v2/src/agent/mcp/output.ts#L192-L199(this comment)packages/agent-core/src/mcp/output.ts#L246-L253packages/agent-core-v2/test/agent/mcp/output.test.ts#L284-L302packages/agent-core/test/mcp/output.test.ts#L286-L304
🤖 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 `@packages/agent-core-v2/src/agent/mcp/output.ts` around lines 192 - 199,
Update serializeStructuredExtras in
packages/agent-core-v2/src/agent/mcp/output.ts (lines 192-199) and its
counterpart in packages/agent-core/src/mcp/output.ts (lines 246-253) to escape
the closing delimiter in serialized JSON rather than remove it, preserving
structured string values. Add matching regression cases in
packages/agent-core-v2/test/agent/mcp/output.test.ts (lines 284-302) and
packages/agent-core/test/mcp/output.test.ts (lines 286-304) using a structured
string containing the closing delimiter and asserting it is preserved.
| - Page larger files with `line_offset` (1-based start line) and `n_lines`. Omit `n_lines` to read up to the ${MAX_LINES}-line cap. | ||
| - Sensitive files (`.env` files, credential stores, SSH private keys, and similar secrets) are refused to protect secrets; do not attempt to read them. Templates and public keys are exempt: `.env.example` / `.env.sample` / `.env.template` and public SSH keys such as `id_rsa.pub` read normally. | ||
| - Only UTF-8 text files can be read. Non-UTF-8 encodings, binary files, and files containing NUL bytes are refused; use `ReadMediaFile` for images or video, and Bash or an MCP tool for other binary formats. | ||
| - UTF-8 text files are read directly. UTF-16 LE/BE text files (with or without a BOM) are detected automatically and transcoded to UTF-8 for display; Edit/Write still expect UTF-8. Other encodings (for example GBK), binary files, and files containing NUL bytes are refused; use `ReadMediaFile` for images or video, and Bash or an MCP tool for other binary formats. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe decoded NUL characters, not raw NUL bytes.
UTF-16 input commonly contains zero-valued bytes and is accepted by ReadTool. State that files whose decoded text contains NUL characters are refused. This keeps the documentation consistent with the UTF-16 behavior.
🤖 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 `@packages/agent-core-v2/src/agent/tools/os/read/read.md` at line 11, Update
the read.md documentation sentence describing refused files to refer to decoded
text containing NUL characters rather than raw NUL bytes, while preserving the
existing UTF-16 detection and transcoding behavior.
Comment on lines
+293
to
+304
| if (!detection.seemsBinary && detection.encoding !== 'utf-8') { | ||
| if (stat.size > TRANSCODE_MAX_BYTES) { | ||
| return { | ||
| isError: true, | ||
| output: | ||
| `"${args.path}" is ${encodingDisplayName(detection.encoding)} text but too large to transcode ` + | ||
| `(${String(stat.size)} bytes > ${String(TRANSCODE_MAX_BYTES)}). Convert it to UTF-8 first.`, | ||
| }; | ||
| } | ||
| const decoded = decodeUtfText(await this.fs.readBytes(safePath), detection.encoding); | ||
| detectedEncoding = detection.encoding; | ||
| lines = decodedLines(splitLinesKeepingTerminator(decoded)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce the transcode limit on the bytes that are read.
stat.size can become stale before Line 302. A file that grows after stat() bypasses the 10 MiB limit and readBytes(safePath) can allocate and decode an unbounded file.
Read at most TRANSCODE_MAX_BYTES + 1 bytes. Reject the file when the returned buffer exceeds TRANSCODE_MAX_BYTES.
Proposed fix
- const decoded = decodeUtfText(await this.fs.readBytes(safePath), detection.encoding);
+ const bytes = await this.fs.readBytes(safePath, TRANSCODE_MAX_BYTES + 1);
+ if (bytes.length > TRANSCODE_MAX_BYTES) {
+ return {
+ isError: true,
+ output:
+ `"${args.path}" is ${encodingDisplayName(detection.encoding)} text but too large to transcode ` +
+ `(${String(bytes.length)} bytes > ${String(TRANSCODE_MAX_BYTES)}). Convert it to UTF-8 first.`,
+ };
+ }
+ const decoded = decodeUtfText(bytes, detection.encoding);📝 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.
Suggested change
| if (!detection.seemsBinary && detection.encoding !== 'utf-8') { | |
| if (stat.size > TRANSCODE_MAX_BYTES) { | |
| return { | |
| isError: true, | |
| output: | |
| `"${args.path}" is ${encodingDisplayName(detection.encoding)} text but too large to transcode ` + | |
| `(${String(stat.size)} bytes > ${String(TRANSCODE_MAX_BYTES)}). Convert it to UTF-8 first.`, | |
| }; | |
| } | |
| const decoded = decodeUtfText(await this.fs.readBytes(safePath), detection.encoding); | |
| detectedEncoding = detection.encoding; | |
| lines = decodedLines(splitLinesKeepingTerminator(decoded)); | |
| if (!detection.seemsBinary && detection.encoding !== 'utf-8') { | |
| if (stat.size > TRANSCODE_MAX_BYTES) { | |
| return { | |
| isError: true, | |
| output: | |
| `"${args.path}" is ${encodingDisplayName(detection.encoding)} text but too large to transcode ` + | |
| `(${String(stat.size)} bytes > ${String(TRANSCODE_MAX_BYTES)}). Convert it to UTF-8 first.`, | |
| }; | |
| } | |
| const bytes = await this.fs.readBytes(safePath, TRANSCODE_MAX_BYTES + 1); | |
| if (bytes.length > TRANSCODE_MAX_BYTES) { | |
| return { | |
| isError: true, | |
| output: | |
| `"${args.path}" is ${encodingDisplayName(detection.encoding)} text but too large to transcode ` + | |
| `(${String(bytes.length)} bytes > ${String(TRANSCODE_MAX_BYTES)}). Convert it to UTF-8 first.`, | |
| }; | |
| } | |
| const decoded = decodeUtfText(bytes, detection.encoding); | |
| detectedEncoding = detection.encoding; | |
| lines = decodedLines(splitLinesKeepingTerminator(decoded)); |
🤖 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 `@packages/agent-core-v2/src/agent/tools/os/read/readTool.ts` around lines 293
- 304, Update the non-UTF-8 transcoding branch around readTool’s decodeUtfText
call to read at most TRANSCODE_MAX_BYTES + 1 bytes, then reject when the
returned buffer length exceeds TRANSCODE_MAX_BYTES. Use the read buffer for
decoding and retain the existing error response and encoding handling.
Comment on lines
+149
to
+180
| it('invalidates a client registration when the callback URI changes', async () => { | ||
| const provider = new McpOAuthClientProvider({ | ||
| serverName: 'notion', | ||
| serverUrl: 'https://mcp.notion.com/mcp', | ||
| store: createMemoryMcpOAuthStore(), | ||
| }); | ||
| await provider.ready; | ||
| await provider.saveClientInformation({ | ||
| client_id: 'cached-client', | ||
| redirect_uris: ['http://127.0.0.1:45678/callback'], | ||
| } satisfies OAuthClientInformationFull); | ||
|
|
||
| await expect(provider.invalidateStaleRegistration('http://127.0.0.1:45679/callback')).resolves.toBe(true); | ||
| await expect(provider.clientInformation()).resolves.toBeUndefined(); | ||
| }); | ||
|
|
||
| it('keeps a client registration when the callback URI still matches', async () => { | ||
| const provider = new McpOAuthClientProvider({ | ||
| serverName: 'notion', | ||
| serverUrl: 'https://mcp.notion.com/mcp', | ||
| store: createMemoryMcpOAuthStore(), | ||
| }); | ||
| await provider.ready; | ||
| await provider.saveClientInformation({ | ||
| client_id: 'cached-client', | ||
| redirect_uris: ['http://127.0.0.1:45678/callback'], | ||
| } satisfies OAuthClientInformationFull); | ||
|
|
||
| await expect(provider.invalidateStaleRegistration('http://127.0.0.1:45678/callback')).resolves.toBe(false); | ||
| await expect(provider.clientInformation()).resolves.toMatchObject({ client_id: 'cached-client' }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Verify persistence with a new provider instance.
clientInformation() returns clientCache. The stale-registration assertion can pass after invalidateCredentials('client') clears that cache even if the store still retains the registration.
Keep the store in a local variable. Create a second McpOAuthClientProvider with that store. Assert that the second provider cannot load the stale registration. Do the same for the matching-URI case to verify that the registration persists.
🤖 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 `@packages/agent-core-v2/test/agent/mcp/oauth/store.test.ts` around lines 149 -
180, Update both tests around invalidateStaleRegistration to keep
createMemoryMcpOAuthStore() in a local store variable, then instantiate a second
McpOAuthClientProvider with that same store after invalidation. Assert the
second provider returns undefined for the changed callback URI, and retains the
cached client information for the matching URI, verifying persistence beyond the
first provider’s clientCache.
Comment on lines
+156
to
+162
| invalidateStaleRegistration(redirectUri: string): boolean { | ||
| const info = this.clientInformation(); | ||
| if (info === undefined || !('redirect_uris' in info)) return false; | ||
| const uris = info.redirect_uris; | ||
| if (!Array.isArray(uris) || uris.length === 0 || uris.includes(redirectUri)) return false; | ||
| this.invalidateCredentials('client'); | ||
| return true; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate persisted client information before using the in operator.
A valid but malformed persisted JSON value can be null or a primitive. In that case, 'redirect_uris' in info throws and prevents OAuth authorization. Check that info is a non-null object before checking its properties.
packages/agent-core/src/mcp/oauth/provider.ts#L156-L162: Validate the value returned byJsonFileStorebefore readingredirect_uris.packages/agent-core-v2/src/agent/mcp/oauth/provider.ts#L165-L171: Apply the same runtime validation before readingredirect_uris.
📍 Affects 2 files
packages/agent-core/src/mcp/oauth/provider.ts#L156-L162(this comment)packages/agent-core-v2/src/agent/mcp/oauth/provider.ts#L165-L171
🤖 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 `@packages/agent-core/src/mcp/oauth/provider.ts` around lines 156 - 162, Update
invalidateStaleRegistration in packages/agent-core/src/mcp/oauth/provider.ts
(lines 156-162) and packages/agent-core-v2/src/agent/mcp/oauth/provider.ts
(lines 165-171) to verify the value returned by clientInformation() is non-null
and an object before applying the 'redirect_uris' in info check; preserve the
existing false return for invalid or missing client information and the
remaining URI validation behavior.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports a focused set of upstream correctness fixes while preserving Echadron's hybrid architecture:
structuredContentand vendor metadata while stripping protocol-reserved metadataValidation
pnpm --filter @moonshot-ai/agent-core-v2 exec vitest run test/agent/mcp/oauth/store.test.ts test/_base/text/encoding.test.ts test/agent/mcp/output.test.ts test/os/backends/node-local/tools/read.test.tspnpm --filter @moonshot-ai/agent-core exec vitest run test/mcp/oauth-store.test.ts test/mcp/output.test.tspnpm --filter @moonshot-ai/agent-core-v2 typecheckpnpm --filter @moonshot-ai/agent-core typecheckSummary by CodeRabbit
Bug Fixes
New Features
Documentation