This repository was archived by the owner on Aug 25, 2026. It is now read-only.
fix(v2): apply compatible upstream correctness follow-ups - #27
Merged
Merged
Conversation
…otAI#2740) (cherry picked from commit 01c74e9)
… update (MoonshotAI#2609) * feat(mcp): carry an absolute expiresAt on the OAuth authorization-url update The authenticate flow waits for the OAuth callback with a fixed budget, but the authorization-url tool update surfaced to embedding hosts did not say when that window ends — hosts had to hardcode a mirror of the 15-minute constant to render countdowns. Include the absolute deadline (now + effective wait timeout) in the update payload for v1 and v2. Resolve MoonshotAI#2607 * fix(protocol,kap-server): accept expiresAt in the OAuth authorization-url update schemas The zod validators mirrored the pre-expiresAt payload shape and would strip the new field at the kap-server boundary. --------- Co-authored-by: zouying <zouying@moonshot.cn> (cherry picked from commit 3126422)
📝 WalkthroughWalkthroughThe PR adds optional OAuth authorization expiry timestamps across protocol and authentication flows. It also isolates builtin profile clones per session and adds regression coverage for catalog isolation. ChangesOAuth authorization expiry metadata
Session-local builtin profiles
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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: 4
🧹 Nitpick comments (1)
packages/agent-core/test/profile/agentfile.test.ts (1)
377-403: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExpand the regression to cover array mutation and snapshot restoration.
The test replaces
firstCoder.toolswith a new array. It does not prove that the clonedtoolsarray stays isolated during an in-place mutation. Capture the baseline list, mutate the session list in place, and compare the global defaults and second catalog with that baseline. Also exerciserestoreSnapshot, which now callssessionLocalBuiltinProfiles()atpackages/agent-core/src/profile/agentfile/catalog.tsLine 155.As per coding guidelines, extend this existing test file instead of adding an excessive new 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 377 - 403, Expand the existing test around firstCoder.tools to capture the baseline tools array, mutate that array in place, and verify DEFAULT_AGENT_PROFILES and the second catalog retain the baseline values. Also invoke restoreSnapshot and assert the restored session-local builtin profile still matches the baseline, covering sessionLocalBuiltinProfiles() behavior without adding a new test file.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 @.changeset/isolate-session-profile-catalogs.md:
- Line 5: Update the public release note text in
isolate-session-profile-catalogs.md to use the standard spelling “built-in
profiles” instead of “builtin profiles,” without changing the rest of the
wording.
In `@packages/agent-core-v2/src/agent/mcp/tools/auth.ts`:
- Around line 111-115: Use the same start time for the advertised OAuth expiry
and completion timeout in both
packages/agent-core-v2/src/agent/mcp/tools/auth.ts:111-115 and
packages/agent-core/src/mcp/auth-tool.ts:116-122. In the corresponding
flow.complete calls, calculate and pass the remaining duration after
update-handler work rather than the original full timeout, using the expiresAt
value established in each auth flow.
- Around line 111-115: The OAuth status messages in auth.ts
(packages/agent-core-v2/src/agent/mcp/tools/auth.ts, lines 111-115) and
auth-tool.ts (packages/agent-core/src/mcp/auth-tool.ts, lines 116-122) use a
fixed 15-minute timeout; update both to interpolate the effective timeout value,
such as waitTimeoutMs, so user-visible instructions match the configured
deadline. Both sites require the same direct change.
In `@packages/agent-core-v2/test/agent/mcp/tools/auth.test.ts`:
- Around line 65-76: Update the expiry assertions in auth.test.ts lines 65-76
and auth-tool.test.ts lines 69-80 to verify that expiresAt is approximately
Date.now() plus the configured timeoutMs of 100, using a controlled clock or an
appropriate tolerance. Replace the broad 15-minute upper bound in both test
sites while preserving the existing authorization URL and serverName assertions.
---
Nitpick comments:
In `@packages/agent-core/test/profile/agentfile.test.ts`:
- Around line 377-403: Expand the existing test around firstCoder.tools to
capture the baseline tools array, mutate that array in place, and verify
DEFAULT_AGENT_PROFILES and the second catalog retain the baseline values. Also
invoke restoreSnapshot and assert the restored session-local builtin profile
still matches the baseline, covering sessionLocalBuiltinProfiles() behavior
without adding a new test file.
🪄 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: 0b716159-3c2d-46b6-8628-fe3617965390
📒 Files selected for processing (9)
.changeset/isolate-session-profile-catalogs.mdpackages/agent-core-v2/src/agent/mcp/tools/auth.tspackages/agent-core-v2/test/agent/mcp/tools/auth.test.tspackages/agent-core/src/mcp/auth-tool.tspackages/agent-core/src/profile/agentfile/catalog.tspackages/agent-core/test/mcp/auth-tool.test.tspackages/agent-core/test/profile/agentfile.test.tspackages/kap-server/src/protocol/events-zod.tspackages/protocol/src/events.ts
| "@moonshot-ai/kimi-code": patch | ||
| --- | ||
|
|
||
| Prevent one session's subagent tool projection from changing builtin profiles in later sessions. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the standard spelling built-in.
Replace builtin profiles with built-in profiles in the public release note.
Proposed wording
-Prevent one session's subagent tool projection from changing builtin profiles in later sessions.
+Prevent one session's subagent tool projection from changing built-in profiles in later sessions.📝 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
| Prevent one session's subagent tool projection from changing builtin profiles in later sessions. | |
| Prevent one session's subagent tool projection from changing built-in profiles in later sessions. |
🧰 Tools
🪛 LanguageTool
[grammar] ~5-~5: Ensure spelling is correct
Context: ... subagent tool projection from changing builtin profiles in later sessions.
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 @.changeset/isolate-session-profile-catalogs.md at line 5, Update the public
release note text in isolate-session-profile-catalogs.md to use the standard
spelling “built-in profiles” instead of “builtin profiles,” without changing the
rest of the wording.
Source: Linters/SAST tools
Comment on lines
+111
to
+115
| const waitTimeoutMs = timeoutMs ?? DEFAULT_AUTH_TIMEOUT_MS; | ||
| const customData: McpOAuthAuthorizationUrlUpdateData = { | ||
| serverName, | ||
| authorizationUrl: urlText, | ||
| expiresAt: Date.now() + waitTimeoutMs, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use one start time for expiresAt and OAuth completion.
Both implementations compute expiresAt before update dispatch, then start completion with the full relative timeout. Update-handler work can make the engine wait beyond the advertised expiry.
packages/agent-core-v2/src/agent/mcp/tools/auth.ts#L111-L115: pass the remaining duration toflow.completeat Line 132.packages/agent-core/src/mcp/auth-tool.ts#L116-L122: pass the remaining duration toflow.completeat Line 139.
📍 Affects 2 files
packages/agent-core-v2/src/agent/mcp/tools/auth.ts#L111-L115(this comment)packages/agent-core/src/mcp/auth-tool.ts#L116-L122
🤖 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/tools/auth.ts` around lines 111 - 115,
Use the same start time for the advertised OAuth expiry and completion timeout
in both packages/agent-core-v2/src/agent/mcp/tools/auth.ts:111-115 and
packages/agent-core/src/mcp/auth-tool.ts:116-122. In the corresponding
flow.complete calls, calculate and pass the remaining duration after
update-handler work rather than the original full timeout, using the expiresAt
value established in each auth flow.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the configured timeout in all user-visible OAuth messages.
Both implementations calculate a configurable deadline but keep the status text at timeout 15 min. This gives users incorrect instructions when a non-default timeoutMs is used.
packages/agent-core-v2/src/agent/mcp/tools/auth.ts#L111-L115: interpolate the effective timeout into the status message.packages/agent-core/src/mcp/auth-tool.ts#L116-L122: interpolate the effective timeout into the status message.
📍 Affects 2 files
packages/agent-core-v2/src/agent/mcp/tools/auth.ts#L111-L115(this comment)packages/agent-core/src/mcp/auth-tool.ts#L116-L122
🤖 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/tools/auth.ts` around lines 111 - 115,
The OAuth status messages in auth.ts
(packages/agent-core-v2/src/agent/mcp/tools/auth.ts, lines 111-115) and
auth-tool.ts (packages/agent-core/src/mcp/auth-tool.ts, lines 116-122) use a
fixed 15-minute timeout; update both to interpolate the effective timeout value,
such as waitTimeoutMs, so user-visible instructions match the configured
deadline. Both sites require the same direct change.
Comment on lines
+65
to
+76
| const authUpdate = updates.find( | ||
| (u) => u.kind === 'custom' && u.customKind === MCP_OAUTH_AUTHORIZATION_URL_TOOL_UPDATE, | ||
| ); | ||
| expect(authUpdate?.customData).toMatchObject({ | ||
| serverName: 'notion', | ||
| authorizationUrl: 'https://example.com/authorize?state=abc', | ||
| }); | ||
| // The deadline is absolute (now + wait timeout), so hosts never mirror | ||
| // the engine-side constant. | ||
| const { expiresAt } = authUpdate?.customData as { expiresAt?: number }; | ||
| expect(expiresAt).toBeGreaterThan(Date.now()); | ||
| expect(expiresAt).toBeLessThanOrEqual(Date.now() + 15 * 60 * 1000); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make expiry assertions match the configured timeout.
Both tests pass timeoutMs: 100 but accept any expiry up to 15 minutes. This does not detect regressions that ignore the configured timeout.
packages/agent-core-v2/test/agent/mcp/tools/auth.test.ts#L65-L76: assert an approximately 100 ms deadline with a controlled clock or tolerance.packages/agent-core/test/mcp/auth-tool.test.ts#L69-L80: assert an approximately 100 ms deadline with a controlled clock or tolerance.
📍 Affects 2 files
packages/agent-core-v2/test/agent/mcp/tools/auth.test.ts#L65-L76(this comment)packages/agent-core/test/mcp/auth-tool.test.ts#L69-L80
🤖 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/tools/auth.test.ts` around lines 65 -
76, Update the expiry assertions in auth.test.ts lines 65-76 and
auth-tool.test.ts lines 69-80 to verify that expiresAt is approximately
Date.now() plus the configured timeoutMs of 100, using a controlled clock or an
appropriate tolerance. Replace the broad 15-minute upper bound in both test
sites while preserving the existing authorization URL and serverName assertions.
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.
3 participants
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
These are the remaining behavior-compatible upstream fixes after the consolidated Echadron v2 parity PR. Other newer upstream commits were already present under Echadron-specific implementations or require architecture replacements that are not safe to cherry-pick.
Validation
pnpm --filter @moonshot-ai/agent-core test(4107 passed, 3 expected failures, 30 skipped, 1 todo)pnpm --filter @moonshot-ai/agent-core-v2 test(4263 passed)pnpm --filter @moonshot-ai/agent-core exec vitest run test/mcp/auth-tool.test.ts(5 passed)pnpm --filter @moonshot-ai/agent-core-v2 exec vitest run test/agent/mcp/tools/auth.test.ts(5 passed)pnpm --filter @moonshot-ai/kimi-code-sdk exec vitest run test/v1-v2-parity.test.ts(79 passed)Summary by CodeRabbit
New Features
Bug Fixes