fix(executor): align interactive form defer handling - #2594
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughClaude deferred-form handling now distinguishes stale answered forms from later clarification forms by interactive-form type and ChangesClaude interactive-form resume
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeCode
participant Executor
participant InteractiveMCP
ClaudeCode->>Executor: Submit interactive-form answer
Executor->>Executor: Match answered tool_use_id
Executor->>InteractiveMCP: Forward later form with a different tool_use_id
InteractiveMCP-->>Executor: Return deferred request
Executor-->>ClaudeCode: WaitingForUserInput
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
frontend/e2e/tests/tasks/agent-conversation-regression.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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: 1
🧹 Nitpick comments (1)
frontend/e2e/utils/mock-model-server.ts (1)
380-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the Anthropic SSE block serialization from
writeAnthropicToolUseResponse.This 98-line function handles validation, headers, message start, text blocks, tool blocks, and message completion. Extract focused helpers and reuse the shared
message_startserialization withwriteAnthropicStreamingResponse.🤖 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 `@frontend/e2e/utils/mock-model-server.ts` around lines 380 - 476, Refactor writeAnthropicToolUseResponse into focused helpers for Anthropic SSE message start, optional text content, tool-use content, and message completion. Reuse the shared message_start serialization already used by writeAnthropicStreamingResponse, preserving the current event order, indexes, payloads, validation, headers, and response termination.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 `@frontend/e2e/tests/tasks/agent-conversation-regression.spec.ts`:
- Around line 761-790: Update the Skill Kind lookup request in the
skill-resolution flow to include the expected user_id, retain namespace=default,
and use exact_match=true. Replace the positional body.items?.[0] selection with
a search for the item whose name, namespace, and user_id match the requested
identity, then preserve the existing skillId validation and returned metadata
behavior.
---
Nitpick comments:
In `@frontend/e2e/utils/mock-model-server.ts`:
- Around line 380-476: Refactor writeAnthropicToolUseResponse into focused
helpers for Anthropic SSE message start, optional text content, tool-use
content, and message completion. Reuse the shared message_start serialization
already used by writeAnthropicStreamingResponse, preserving the current event
order, indexes, payloads, validation, headers, and response termination.
🪄 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: a58f45b1-1c8a-4e9e-90ed-70509cee5d92
📒 Files selected for processing (6)
docs/en/wegent/developer-guide/architecture.mddocs/zh/wegent/developer-guide/architecture.mdexecutor/src/process/mod.rsexecutor/tests/agent_runtime_capabilities_contract.rsfrontend/e2e/tests/tasks/agent-conversation-regression.spec.tsfrontend/e2e/utils/mock-model-server.ts
| const response = await request.get( | ||
| `${API_BASE_URL}/api/v1/kinds/skills?name=${encodeURIComponent(skillName)}&namespace=default&exact_match=false`, | ||
| { headers: authHeaders() } | ||
| ) | ||
| expect(response.status()).toBe(200) | ||
|
|
||
| const body = (await response.json()) as { | ||
| items?: Array<{ | ||
| metadata?: { | ||
| namespace?: string | ||
| labels?: Record<string, string> | ||
| } | ||
| status?: { | ||
| fileHash?: string | ||
| } | ||
| }> | ||
| } | ||
| const skill = body.items?.[0] | ||
| const skillId = Number(skill?.metadata?.labels?.id) | ||
| expect( | ||
| skillId, | ||
| `Skill ${skillName} should expose a numeric metadata.labels.id` | ||
| ).toBeGreaterThan(0) | ||
|
|
||
| const fileHash = skill?.status?.fileHash | ||
| return { | ||
| skill_id: skillId, | ||
| namespace: skill?.metadata?.namespace || 'default', | ||
| is_public: skill?.metadata?.labels?.user_id === '0', | ||
| content_hash: fileHash ? `sha256:${fileHash}` : undefined, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Query the Skill Kind with its full identity.
This request omits user_id, sets exact_match=false, and then selects body.items?.[0]. If another user owns a matching skill name, this test can attach the wrong skill metadata. Query with namespace, exact name, and the expected user_id. Then select the item that matches all three values.
🤖 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 `@frontend/e2e/tests/tasks/agent-conversation-regression.spec.ts` around lines
761 - 790, Update the Skill Kind lookup request in the skill-resolution flow to
include the expected user_id, retain namespace=default, and use
exact_match=true. Replace the positional body.items?.[0] selection with a search
for the item whose name, namespace, and user_id match the requested identity,
then preserve the existing skillId validation and returned metadata behavior.
Source: Coding guidelines
27737b2 to
79334ec
Compare
1 participant
What changed
tool_use_idmatches the form that was just answeredRoot cause
The Rust executor treated any deferred form returned with non-empty completed content after an interactive answer as stale. That condition was broader than the Python implementation, which scoped stale resume output by the answered form's
tool_use_id. As a result, a legitimate second clarification could be swallowed.Validation
cargo fmt --checkcargo test --test agent_runtime_capabilities_contractcargo test --test agent_command_contractcargo test --test interactive_mcp_contractGitHub CI is expected to run the real executor E2E scenario because the local Docker runtime could not start new containers.
Summary by CodeRabbit
Bug Fixes
Documentation