Repository navigation
[Agent Chat feedback] - Step 1: Let agents reach a Copilot-hosted feedback tool - #3593
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ee1ac8095
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for await (const chunk of request) { | ||
| body += chunk; | ||
| if (body.length > MAX_REQUEST_BYTES) throw new Error("Request body is too large."); |
There was a problem hiding this comment.
MAX_REQUEST_BYTES is enforced with JavaScript string length after UTF-8 decoding, so it counts UTF-16 code units rather than received bytes. For example, a body containing 30,000 CJK characters is about 90 KiB but has length 30,000 and is accepted, contradicting the advertised 64 KiB input bound. Accumulate raw buffer lengths or use Buffer.byteLength() while reading so non-ASCII agent output cannot bypass the limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2eab740. readBody now counts chunk.byteLength and decodes with a streaming TextDecoder. The size test now sends 30,000 CJK characters (about 90 KB in UTF-8) and expects a 400.
6ee1ac8 to
2eab740
Compare
2eab740 to
168faa2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 168faa2222
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async connect(sessionKey: string, onOffer: FeedbackOfferHandler): Promise<AgentMcpServer> { | ||
| if (disposed) throw new Error("The feedback MCP server is closed."); | ||
| urlPromise ??= start(); | ||
| const url = await urlPromise; |
There was a problem hiding this comment.
When a new session connects while plugin teardown races it after the server has already started, this await still yields even though urlPromise is fulfilled. dispose() can therefore close the listener and clear both maps before the continuation resumes; the continuation then installs a fresh token and resolves with a dead MCP URL. The session may proceed with an endpoint that cannot accept connections, while the supposedly disposed server retains the new handler and token. Check disposed again immediately after the await before creating the channel.
Useful? React with 👍 / 👎.
| const next = http.createServer((request, response) => { | ||
| void handleHttpRequest(request, response, handlersByToken); | ||
| }); |
There was a problem hiding this comment.
If a valid send_feedback request reaches an onOffer callback that throws—for example, because constructing the feedback card fails—handleHttpRequest() rejects. This listener explicitly discards that promise, so Node sends no JSON-RPC response, the MCP client can wait indefinitely, and the failure becomes an unhandled rejection instead of an internal-error reply. Attach a rejection handler here or catch callback failures inside the request handler and complete the response.
AGENTS.md reference: AGENTS.md:L64-L66
Useful? React with 👍 / 👎.
168faa2 to
205f23c
Compare
A loopback streamable-HTTP MCP server on 127.0.0.1 exposes one send_feedback tool, and each chat connects with its own bearer token so a call reaches only that chat's offer handler. Session new, resume and load accept HTTP MCP servers: ACP backends pass them on only when the agent advertises HTTP MCP, and the Claude SDK adapter registers them and pre-approves their tools. Nothing starts the server yet. Relates to Brevilabs/obsidian-copilot-private#672 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rver Report an issue already runs every attached log through redactLogText, which strips Bearer tokens, so the debug-tap redaction repeated it. The 64 KiB request cap already bounds a draft, so the per-field cap is gone too. A JSON array body now gets an invalid-request error instead of a silent 202. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
205f23c to
6cb4384
Compare
1 participant
Relates to Brevilabs/obsidian-copilot-private#672
Why
When an Agent Chat turn goes wrong, the user usually says so in the chat, and that complaint is the best bug report we will ever get. The agent cannot help send it, because Agent Chat gives agents no Copilot-owned tools: every session opens with
mcpServers: []for Codex and OpenCode, and the Claude SDK adapter registers none. The feedback card in the next step needs agents to be able to call one Copilot tool,send_feedback, and a tool that drafts a report must be reachable only by the chat it belongs to.What
Agent Chat sessions can now carry Copilot-hosted MCP servers, and Copilot can host one. Nothing starts or attaches the server yet, so chats behave exactly as today.
initialize,ping,tools/listandtools/call send_feedbackare answered; a valid call is handed to that chat onlyBearertoken with<token>, so a chat's token never leaves the machineNon goal
send_feedback.Screenshot
Not applicable: this PR adds plumbing with no rendered change; nothing starts the server until a later step.
Risk
High
AcpBackendProcesstests cover new/resume/load with and without HTTP MCP,ClaudeSdkBackendProcesstests cover registration and pre-approval,feedbackMcpServertests cover handshake, auth, errors, size limits, disconnect and disposesession/new,session/resumeandsession/loadcan now carry MCP serversReview: inspect
createFeedbackMcpServer,handleHttpRequest,reply,callTool,parseDraftandreadBodyinsrc/agentMode/session/feedback/feedbackMcpServer.ts;mcpServersFieldinsrc/agentMode/acp/AcpBackendProcess.ts; and the MCP server block inprompt()insrc/agentMode/sdk/ClaudeSdkBackendProcess.ts, line by line. Review the tool text inSEND_FEEDBACK_TOOL. Then run Verification.Verification