Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Adds per-request Key changes
Summary ｜ 6 files ｜ 7 commits ｜ base: Per-request execution mode override
The same
Execution Modes documentation
The page now has separate subsections for Visual Builder configuration and per-request override, with an inline JSON example showing the Composable
|
There was a problem hiding this comment.
Clean, well-scoped change. The per-request executionMode override is applied consistently in both /chat and /chatDataStream routes, the enum values match the existing DB schema and validation, the new learnMoreHref prop on SectionHeader is additive and optional, and the docs page is solid. No issues found.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
🟠 1) system Mid-conversation mode switching may cause unexpected behavior
Issue: The PR allows clients to switch execution mode per-request, but a conversation can span multiple requests. If a conversation starts in classic mode (in-memory tool approval state) and a subsequent request specifies durable mode (workflow-persisted state), or vice versa, the state management mechanisms are fundamentally different.
Why: Classic mode uses pendingToolApprovalManager (in-memory), while durable mode uses workflow persistence with toolApprovalHook.resume(). The code in chatDataStream.ts lines 141-197 checks for suspended durable executions on approval responses, but there is no validation or warning if the conversation's previous requests used a different mode. This could lead to orphaned tool approvals or unexpected behavior if a client switches modes mid-conversation.
Fix: Consider one of:
- Persist the execution mode used for the first request of a conversation and enforce consistency
- Add a warning/error if a client attempts to switch modes mid-conversation
- Explicitly document that mode switching mid-conversation is not recommended and may cause tool approval state to be lost
Refs:
🟠 2) packages/ai-sdk-provider/src/inkeep-chat-options.ts SDK does not expose the new executionMode parameter
Issue: The @inkeep/ai-sdk-provider package exposes InkeepChatOptions with conversationId, runConfig, and headers but not executionMode. Customers using the Vercel AI SDK provider cannot use the per-request override without modifying the SDK.
Why: This creates an API surface gap: the REST API supports executionMode but the official SDK abstraction does not. SDK users would need to drop down to raw fetch calls to use this feature.
Fix: Add executionMode?: 'classic' | 'durable' to InkeepChatOptions interface and wire it through the request body in inkeep-chat-language-model.ts. This ensures SDK users have parity with direct API consumers.
Refs:
🟡 Minor (1) 🟡
🟡 1) agents-docs/content/talk-to-your-agents/chat-api.mdx Chat API docs should document executionMode parameter
Issue: The Chat API documentation does not document the new executionMode request body parameter. The PR adds executionMode to both chat endpoints but only updates the visual builder docs, not the API reference.
Why: Developers reading the Chat API docs will not discover the executionMode parameter. They would only find it via the OpenAPI auto-generated docs or by reading the visual builder page.
Fix: Add executionMode to the Request Body Schema section of chat-api.mdx:
executionMode— Optional; override the agent's default execution mode for this request. Values:classic(low-latency streaming) ordurable(crash recovery, persistent tool approvals). See Execution Modes.
Refs:
Inline Comments:
- 🟡 Minor:
section.tsx:39-48Use existing ExternalLink component - 🟡 Minor:
execution-modes.mdx:26Internal implementation detail exposed - 💭 Consider:
chat.ts:98-101Improve parameter description
💭 Consider (2) 💭
💭 1) chat.ts + chatDataStream.ts Extract shared executionMode schema
Issue: The z.enum(['classic', 'durable']).optional().describe(...) schema is copy-pasted across two route files.
Why: The codebase already has a pattern for this in packages/agents-core/src/validation/schemas.ts where executionMode is defined within AgentInsertSchema. A standalone exported schema would allow reuse and prevent future drift if enum values change.
Fix: Extract a shared schema like export const ExecutionModeSchema = z.enum(['classic', 'durable']) to a shared location and import it in both route files.
💭 2) execution-modes.mdx Add cautions about mid-conversation mode switching
Issue: The docs explain what the parameter does but do not guide customers on safe usage patterns.
Why: Users may not realize the implications of switching modes during a conversation, particularly around tool approval state.
Fix: Add a "Cautions" section explaining: (1) avoid switching modes mid-conversation as tool approval state may be lost, (2) the override is useful for A/B testing or gradual rollout scenarios, (3) for most production use cases, set the mode at the agent level.
📋 Missing Changeset
This PR adds a new API parameter (executionMode) to agents-api which is a user-facing change. Per AGENTS.md guidelines, a changeset is required:
pnpm bump patch --pkg agents-api "Add per-request executionMode override for chat endpoints"🚫 REQUEST CHANGES
Summary: The implementation is clean and the feature is well-documented, but there are two blocking concerns: (1) the SDK gap means customers using @inkeep/ai-sdk-provider cannot use this feature, and (2) there's no handling for the edge case of mode switching mid-conversation which could cause tool approval state issues. A changeset is also required for this API change.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
chatDataStream.ts:75 |
Schema field ordering differs from chat.ts | Cosmetic inconsistency that doesn't affect runtime behavior; low priority |
execution-modes.mdx:54-63 |
Code example only shows one endpoint format | The example is correct for the primary use case; adding tabs would be nice-to-have but not blocking |
execution-modes.mdx |
Missing Related section | Valid improvement but not blocking for initial docs page |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-product |
5 | 2 | 1 | 0 | 1 | 0 | 1 |
pr-review-docs |
4 | 1 | 0 | 0 | 1 | 0 | 2 |
pr-review-consistency |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-frontend |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 13 | 3 | 2 | 0 | 3 | 0 | 5 |
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
Delta Review: This review is scoped to changes since the previous automated review. The delta correctly addresses prior feedback items.
✅ Delta Changes Look Good
The following feedback items from the prior review have been addressed in this update:
-
Improved
executionModedescription — Bothchat.tsandchatDataStream.tsnow document the full precedence chain: "Takes precedence over the agent config default. Falls back to classic if unset." -
Removed internal implementation detail — The
execution-modes.mdxdocs no longer expose "pg-boss in production" — now just says "workflow backend" -
Switched to composition pattern —
SectionHeadernow uses achildrenprop instead oflearnMoreHref, per dimaMachina's feedback. Themetadata-editor.tsxcorrectly usesExternalLinkwithDOCS_BASE_URLconstant.
🕐 Pending Recommendations (2)
These issues were raised in the previous review and remain unaddressed:
- 🟠
ai-sdk-providerSDK does not expose theexecutionModeparameter — customers using@inkeep/ai-sdk-providercannot use per-request override - 🟠
systemMid-conversation mode switching may cause unexpected behavior with tool approval state
📋 Missing Changeset
Per AGENTS.md guidelines, this PR requires a changeset for the new API parameter:
pnpm bump patch --pkg agents-api "Add per-request executionMode override for chat endpoints"🚫 REQUEST CHANGES
Summary: The delta changes correctly address prior feedback — the description improvement, pg-boss detail removal, and composition pattern switch are all well-implemented. However, the two major issues from the previous review (SDK parity and mid-conversation mode switching) remain unaddressed, and a changeset is still needed.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator-delta |
3 | 0 | 0 | 0 | 0 | 2 | 0 |
| Total | 3 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: Delta review — prior issues from previous automated review are tracked in Pending Recommendations.
Ito Test Report ❌17 test cases ran. 1 failed, 16 passed. Overall, 17 test cases were executed with 16 passing and 1 failing, showing that executionMode behavior is largely correct and consistent across /run/v1/chat/completions and /run/api/chat (including durable/classic request-level override precedence, classic fallback when unset, stable SSE and x-workflow-run-id semantics), with additional validation that OpenAPI/docs exposure, invalid payload schema rejection, replay/noisy-header resilience, and execution-mode UI rendering/usability/persistence (desktop and mobile) all worked as expected. The one significant issue is a medium-severity authentication-boundary defect in development/test where unauthenticated run requests are allowed into a synthetic fallback context and then fail later as 404 project-not-found instead of being rejected up front with 401/403, which can mask real auth regressions and mislead operators. ❌ Failed (1)
🟠 Unauthorized run route access attempt
Relevant code:
function createDevContext(reqData: RequestData): AuthResult {
const result: AuthResult = {
apiKey: 'development',
tenantId: reqData.tenantId || 'test-tenant',
projectId: reqData.projectId || 'test-project',
agentId: reqData.agentId || 'test-agent',
apiKeyId: 'test-key',
...(reqData.runAsUserId
if (isDev) {
logger.info({}, 'development environment');
const attempt = await authenticateRequest(reqData);
if (attempt.authResult) {
// ...
c.set('executionContext', buildExecutionContext(attempt.authResult, reqData));
} else {
logger.info({}, 'Development/test environment - no API key provided, using default context');
c.set('executionContext', buildExecutionContext(createDevContext(reqData), reqData));
}
}✅ Passed (16)
Commit: Tell us how we did: Give Ito Feedback |
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Low
Re-review: No code changes since last automated review (
b398f4f). The PR merged from main but the actual feature implementation is unchanged. Prior feedback items were addressed in previous commits.
✅ Previously Addressed
The following feedback items from prior reviews have been correctly addressed:
- Improved
executionModedescription — Bothchat.tsandchatDataStream.tsnow document the full precedence chain - Removed internal implementation detail — The docs no longer expose "pg-boss in production"
- Switched to composition pattern —
SectionHeadernow uses achildrenprop per dimaMachina's feedback
🕐 Pending Recommendations (3)
These issues were raised in the previous review and remain unaddressed:
- 🟡
ai-sdk-providerSDK does not expose theexecutionModeparameter — customers using@inkeep/ai-sdk-providercannot use per-request override (can be follow-up work) - 🟡
chat-api.mdxChat API docs should document the newexecutionModeparameter in the Request Body Schema section - 📋 Missing Changeset — Per AGENTS.md guidelines, this PR requires a changeset:
pnpm bump patch --pkg agents-api "Add per-request executionMode override for chat endpoints"
💭 Reconsidered
The prior "mid-conversation mode switching" concern has been downgraded. While technically valid, this is an advanced edge case that:
- Only affects users who explicitly choose different modes per-request within the same conversation
- The behavior (tool approval state differences) is inherent to the mode semantics documented on the execution-modes page
- Adding enforcement would add complexity without clear user benefit
If desired, a brief note in the docs about mode consistency within conversations would suffice.
💡 APPROVE WITH SUGGESTIONS
Summary: The core implementation is solid and well-documented. The composition pattern refactor is clean. The remaining items are:
- Changeset (required before merge)
- Chat API docs update (minor — add
executionModeto the field list) - SDK parity (optional follow-up)
Nice work on the feature! 🎉
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator-revalidation |
3 | 0 | 0 | 0 | 0 | 3 | 1 |
| Total | 3 | 0 | 0 | 0 | 0 | 3 | 1 |
Note: Re-review — validated status of prior recommendations. No new issues found.
Ito Test Report ✅8 test cases ran. 8 passed. Overall, the unified verification run passed with 8 reportable test cases all succeeding (0 failures), while several additional attempts produced no includable outcomes due to harness stalls or non-reportable artifact reviews. Key findings were that executionMode is correctly documented and supported on both chat endpoints (with localhost /visual-builder/execution-modes 404 expected because links use DOCS_BASE_URL), invalid/injection/oversized executionMode payloads fail safely with controlled 400s and no sensitive leakage, unauthenticated or forged x-target-* requests are blocked without workflow-execution signals, and Manage UI persistence plus the mobile Learn more link behavior work as expected. ✅ Passed (8)
Commit: Tell us how we did: Give Ito Feedback |
* durable-exec-docs * durable-exec-docs * client execution mode * client execution mode
No description provided.