fix: use workspaces when chat capabilities are insufficient - #29579
Conversation
|
@codex review Please review the workspace fallback guidance, including consistency across prompts, tool descriptions, planning mode, delegated agents, and documentation. Generated by Coder Agents on behalf of @ibetitsmike. |
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
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: 582b47799b
ℹ️ 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".
Documentation CheckUpdates Needed
Automated review via Coder Agents |
|
@codex review Both findings from the first review are addressed in 8be2786: delegated workspace needs return to the parent, and Plan Mode provisioning cannot unlock workspace MCP tools. Also fixed the stale API-level prompt expectation found by CI. The focused prompt/workspace tests, Plan Mode MCP boundary test, API prompt-composition tests with and without the race detector, and full make pre-commit now pass. Please review the latest head for remaining issues. Generated by Coder Agents on behalf of @ibetitsmike. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8be27869c0
ℹ️ 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".
ibetitsmike
left a comment
ibetitsmike
left a comment
There was a problem hiding this comment.
Reviewed 8be27869c0efe37d9eba081abdb197bfca219f18. The positive fallback is the right direction; the inline suggestion makes the original coding-refusal case explicit.
The existing comments about reserving start_workspace for an attached workspace and keeping the tool description consistent with Plan Mode are worth addressing; I have not duplicated them. I will reply separately to the capability-refresh finding because the runner does prepare a fresh generation after a successful tool-result commit.
Validation: prompt/fallback tests, chat-creation and API prompt-composition tests, template-selection regression, workspace-creation end-to-end test, and Plan Mode external-MCP boundary test passed locally. These use assertions/mock providers, so they do not establish live-model compliance. A behavioral check should cover a detached implementation request, a self-contained snippet, an ambiguous template choice, and a genuine setup failure.
Generated by Coder Agents on behalf of @ibetitsmike.
|
@codex review Please review latest head 6f51880. Fixed the remaining Plan Mode tool-description conflict and detached start_workspace guidance. Replied to the capability-refresh finding with evidence that successful generation tasks execute one action, and the next task reloads state and prepares context/tools again within the same user turn. Existing per-preparation MCP and runner-rescheduling tests pass, along with the affected prompt/workspace/API tests and full make pre-commit. Generated by Coder Agents on behalf of @ibetitsmike. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f5188088e
ℹ️ 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".
| "Create a new workspace from a template when missing tools, skills, "+ | ||
| "MCPs, or context prevent progress, when workspace-backed work is "+ | ||
| "needed, or when the user explicitly asks for one. Use the "+ | ||
| "workspace's available context and capabilities to continue the "+ |
There was a problem hiding this comment.
When a detached Ask-mode chat provisions a workspace specifically for a missing workspace MCP, this promise can advance the request before that MCP is available. The fresh evidence beyond the earlier capability-refresh discussion is that agent/agent.go:1534-1554 reports lifecycle Ready before releasing the context gate and connecting MCP servers, while waitForAgentReady returns immediately upon observing Ready (coderd/x/chatd/chattool/createworkspace.go:680-681); the rescheduled task can therefore prepare an empty pinned MCP catalog at generation_preparer.go:407-410 and generate its final response before the later context push arrives. Wait for the initial MCP/context synchronization, or reschedule generation when that synchronization completes, before directing the model to continue with the new capabilities.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed the narrower startup race: lifecycle Ready precedes the context gate and MCP reload. There is no exposed terminal discovery-completion signal; context snapshot existence cannot distinguish completed empty/failed discovery from pending discovery. The private agentmcp.Manager.firstSyncSettled signal is not exposed to chatd. A reliable runtime barrier requires an agent/API or snapshot lifecycle change, beyond the requested wording-only change.
The latest wording and docs now explicitly avoid promising immediate skills/MCP/context availability, direct use of actually exposed capabilities and file/shell tools where possible, and prohibit workspace recreation to retry discovery. This does not fix the runtime race. I have asked the user whether to expand this PR or keep the synchronization work separate; leaving this finding open pending that decision.
Generated by Coder Agents on behalf of @ibetitsmike.
|
@codex review Please review 847c350. The stale detached-awareness finding is fixed, and wording now explicitly distinguishes workspace readiness from skill/MCP/context discovery. The initial MCP discovery synchronization race is confirmed and remains open pending a user decision on expanding this wording-only PR; no runtime fix is claimed. Please check the latest wording for other actionable regressions. Generated by Coder Agents on behalf of @ibetitsmike. |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Please review f4f6133, which incorporates the PR author’s comments: explicit necessary-setup authorization and self-contained-example guidance in root-only awareness, plus a deterministic next-model-request test proving published instruction/skill/MCP capabilities appear after detached workspace creation in the same user turn. Also corrected the tools documentation introduction flagged by doc-check. Full make pre-commit, affected prompt/API/guardrail tests, and the creation test with the race detector pass. The separately documented asynchronous discovery limitation remains unchanged. Generated by Coder Agents on behalf of @ibetitsmike. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Comment follow-up is complete in f4f6133: necessary-setup authorization is explicit, self-contained examples remain workspace-free when possible, and the existing provisioning E2E now checks published instruction/skill/MCP capabilities on the next model request within the same user turn. The start-versus-create and per-step-refresh comments were already addressed; no redundant runtime refresh was added. The older doc-check finding is also fixed: the Workspace creation tools introduction now includes missing capabilities/context rather than only compute. All CI checks pass (23 passed, 8 skipped), full local make pre-commit passes, and Codex reports no major issues. Mock-provider tests do not establish live-model compliance. The asynchronous startup-discovery issue remains separately documented and open. Generated by Coder Agents on behalf of @ibetitsmike. |
3 participants
Chatd's guidance discouraged workspace creation even when the chat lacked the tools or context needed to finish a request. Make a suitable workspace the fallback when missing tools, skills, MCPs, or context block progress, while preferring existing capabilities when sufficient.
Align the system prompt, workspace awareness, planning guidance, tool description, regression tests, and user documentation. Preserve template-selection guardrails and delegated-agent restrictions.
Open review finding: workspace readiness can precede initial MCP/context discovery. The wording now acknowledges this limitation; a runtime synchronization fix is pending a scope decision.
Implementation and verification context
Generated by Coder Agents on behalf of @ibetitsmike.