Make Build mode use the agentic loop - #4381
Conversation
![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: 3075ad4e6e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge This is a large, well-structured routing change: Build now streams through Two things block merge: the Playwright suite was not migrated alongside the deleted auto-approve UI and the tag-based Build flow, and The diff was provided in full (not truncated), so the findings below are grounded in the complete change set. I could not run the test suites in this environment, so the e2e claim below is based on reading the unchanged helpers rather than on an observed failure. Issues Summary
Playwright setup still toggles the deleted Auto-approve switch. Security review runs with no codebase context.
Build turns now commit unrelated working-tree changes. Build prompt omits the git-context block. Turbo Edits and Smart Context silently become no-ops. Both features only ever acted on the legacy Build pipeline: 🟢 Low Priority Notes (5 items)
Generated by Dyadbot persona-based code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8a35ea07a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Routing Build through the shared local-agent loop is well executed on the main-process side. The fail-closed allowlist is checked first in The blocking problems are in test infrastructure that the PR did not migrate. Deleting Also worth noting: with all four chat modes now returning early into I verified the tool-profile filter ordering, the Issues Summary
🟢 Low Priority Notes (4 items)
Generated by Dyadbot persona-based code review |
🤖 Claude Code Review SummaryPR Confidence: 4/5All trusted review threads are resolved and local formatting, lint, type checking, unit/integration tests, and targeted packaged E2Es pass; GitHub CI is still running, and the Codex review check has an unrelated authentication failure. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions 🤖 Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7208a4705c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Build mode is routed through I found no HIGH issues. The MEDIUM items below are mostly about what the cutover leaves behind: an unreachable approval UI for legacy pending proposals, a large now-dead legacy streaming path, token-estimate gaps, and removed test coverage. Issues Summary
🟢 Low Priority Notes (6 items)
Confidence notes: the diff was provided in full (no truncation), but this review is based on the patch plus the base-branch sources, not on a running build — I did not execute the test suites, so I cannot confirm the updated snapshots and integration assertions actually pass. Generated by Dyadbot persona-based code review |
github-actions
Bot
added
the
needs-human:review-issue
🤖 Claude Code Review SummaryPR Confidence: 4/5All trusted review threads are resolved and local formatting, lint, type checks, and 102 targeted tests pass; fresh CI and automated review jobs are still pending. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions 🤖 Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 692bdab4e5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge This is a large, well-structured migration: Build now routes through Two things block merge, both in the shared plumbing rather than in Build's tool surface itself: the new turn-limiting is applied to all agent-backed modes (not just Build) at a default cap of 4 user turns, and the auto-approve compatibility migration only fires for users whose stored value is literally Note on context: the aggregate Issues Summary
🔴 Agent/Ask/Plan history now truncated to the legacy Build turn cap
🔴 Auto-approve migration misses the default (unset) case
🟢 Low Priority Notes (7 items)
Generated by Dyadbot persona-based code review |
🤖 Claude Code Review SummaryPR Confidence: 5/5All trusted review threads are resolved, the CI artifact failures were reproduced and addressed, focused E2E coverage passes, and formatting, lint, type-check, unit tests, fake-server build, and application packaging are clean. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions 🤖 Generated by Claude Code |
🤖 Claude Code Review SummaryPR Confidence: 5/5No unresolved review threads remain, and this audit required no additional code changes. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions 🤖 Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0211822e73
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Build mode is routed through The issues below are all behavioral/maintainability concerns rather than defects in the new agentic path; none of them block merge on their own, but the first two are user-visible changes that deserve an explicit product sign-off. Confidence note: the diff spans 194 files, ~5.4k additions and ~19.2k deletions. I reviewed all production source, prompt, and test-harness changes in full; the large Issues Summary
🟢 Low Priority Notes (7 items)
Generated by Dyadbot persona-based code review |
|
Addressed the latest review feedback and CI run 33107870640 in Review threads:
CI/E2E fixes:
Verification: targeted local-agent tests (90 passed), stable ARIA snapshot tests (15 passed), affected E2E paths (9/10 before the intended baseline refresh, then the full corrected |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31839e514a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const mcpToolDefs = | ||
| selectedChatMode === "local-agent" ? getCachedMcpToolDefs() : []; |
There was a problem hiding this comment.
When Agent mode has enabled MCP servers on the first turn after launch—or immediately after the server configuration changes—this reads a module cache that starts empty and is refreshed only later by collectMcpToolDefs() inside handleLocalAgentStream. The estimate therefore omits declarations that the actual request registers, so the context-limit banner can fail to warn for a large MCP tool surface. Fresh evidence since the earlier fix is that repository-wide search shows the runtime handler is the cache's only producer; prewarm or invalidate and recollect the definitions before estimating. rules/chat-modes.mdL7-L7
Useful? React with 👍 / 👎.
| let codebaseTokens = 0; | ||
| const isDyadPro = isDyadProEnabled(settings); | ||
| const mcpToolDefs = | ||
| selectedChatMode === "local-agent" ? getCachedMcpToolDefs() : []; |
There was a problem hiding this comment.
🟡 MEDIUM
Agent MCP tool tokens are undercounted because the cache is only populated in sandbox mode
The new token accounting feeds getCachedMcpToolDefs() into estimateAgentToolTokens, but the module cache in mcp_type_defs.ts is only written by collectMcpToolDefs(), and local_agent_handler.ts only calls that when mcpInSandboxEnabled is true. In the opposite configuration — sandbox script execution off, so MCP tools are registered directly as LLM tools via getMcpTools() — the cache is never populated at all. That is exactly the case where estimateAgentToolTokens would push the inline MCP declarations into its estimate, so the tokens that actually cost the most are the ones reliably missing. The cache is also empty for the whole session until the first Agent turn runs, and afterwards reflects whatever servers were enabled during that turn rather than current state.
💡 Suggestion: Call collectMcpToolDefs() (or a cheap DB-backed equivalent) from the token-count handler instead of reading a cache that only a sandbox-enabled Agent turn fills, or populate the cache unconditionally in handleLocalAgentStream before the sandbox check.
| break; | ||
| } | ||
|
|
||
| if (toolCall) { |
There was a problem hiding this comment.
🟡 MEDIUM
Legacy fixture converter silently drops unconvertible tags and their preceding text
convertLegacyFixtureToLocalAgent advances precedingEnd past every matched tag but only pushes a turn when toolCall is defined. A <dyad-search-replace> block whose body does not match the strict SEARCH/=======/REPLACE regex therefore discards both the tag and the narration text that preceded it, with no error. Separately, a <dyad-add-dependency> tag missing a packages attribute throws TypeError: Cannot read properties of undefined (reading 'split') from inside fixture loading. Because this adapter now silently backstops every tc=<name> that resolves to a legacy .md fixture, either failure mode surfaces as a confusing downstream Playwright timeout rather than as a fixture-conversion error.
💡 Suggestion: Throw an explicit conversion error when a matched tag cannot be converted (unparsable search-replace body, missing required attribute) so fixture problems fail loudly at load time.
| </p> | ||
| </div> | ||
|
|
||
| <div id={SETTING_IDS.appBlueprint} className="space-y-1.5"> |
There was a problem hiding this comment.
🟡 MEDIUM
Retiring Build auto-approve silently enables auto-applied file writes with no migration
Build now authorizes tools through agentToolConsents, where write_file, search_replace, copy_file, delete_file and rename_file all have defaultConsent: "always". The Auto-approve switch and its settings entry are deleted, and src/lib/schemas.test.ts explicitly asserts that autoApproveChanges is not migrated into agentToolConsents. The net effect is that an existing user who deliberately left auto-approve off (the previous default) upgrades into Build applying file edits immediately, with no prompt and no notice that the control moved. Database and dependency mutations still prompt (execute_sql and add_dependency default to ask), and Git checkpoints remain, so this is a deliberate product change rather than a broken guard — but the silent default flip for existing users deserves a conscious decision.
💡 Suggestion: Either migrate a stored autoApproveChanges: false into ask consents for the mutating Build tools, or surface a one-time notice pointing users at the renamed "Build and Agent Permissions" section.
| }: { | ||
| showContextFilesPicker?: boolean; | ||
| }) { | ||
| export function ChatInputControls() { |
There was a problem hiding this comment.
🟡 MEDIUM
Context-paths hook and IPC handlers are left with no remaining consumer
Deleting ContextFilesPicker.tsx and the showContextFilesPicker / hideContextFilesPicker props removes the only consumer of src/hooks/useContextPaths.ts, which in turn is the only caller of ipc.context.getContextPaths / ipc.context.setContextPaths. src/ipc/handlers/context_paths_handlers.ts stays registered and still does glob expansion plus token counting, but nothing in the renderer can reach it. This matches the PR's own note that persisted context scopes are ignored by the agentic runtime, so the surface is now unreachable infrastructure rather than a compatibility shim.
💡 Suggestion: Delete useContextPaths.ts and the context-paths IPC handlers/contract in this PR, or add a comment stating why the channel is intentionally retained without a caller.
| testSkipIfWindows("write to index, approve, check preview", async ({ po }) => { | ||
| await po.setUp(); | ||
| testSkipIfWindows("write to index and check preview", async ({ po }) => { | ||
| await po.setUp({ autoApprove: true }); |
There was a problem hiding this comment.
🟡 MEDIUM
Legacy proposal approve/reject path loses all remaining test coverage
src/ipc/handlers/__tests__/reject.integration.test.ts is deleted, PageObject.approveProposal/rejectProposal are removed, and every remaining E2E call site (approve, debugging_logs, logs_server, visual_editing, package_manager) now opts into auto-approval instead. The approve/reject code path is still live, though: ChatInputActions renders whenever getProposal returns non-null, and proposal_handlers deliberately keeps returning legacy proposals for assistant messages without aiMessagesJson — i.e. exactly the pre-upgrade chats this compatibility path exists to serve. That path now ships with no automated coverage at any level, and ChatInput.tsx simultaneously relaxes its render guard by dropping the selectedMode !== "ask" && selectedMode !== "local-agent" condition.
💡 Suggestion: Keep one integration test that seeds an assistant message with legacy XML content and a null aiMessagesJson, then drives the real Approve and Reject buttons, so the stored-response compatibility path stays verified.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Build mode is routed through The findings below are all MEDIUM: one real defect in the new token accounting, one silent-failure mode in the legacy fixture adapter, and three consent/dead-code/coverage concerns that follow from retiring the Build controls. Two confidence caveats: this review is based on the provided diff only — I did not execute the unit, integration, or Playwright suites, so I cannot confirm the rebased snapshots and the many adapted E2E fixtures actually pass. The diff is large (200 files, ~5.4k additions / ~19.2k deletions), and roughly half of it is snapshot/fixture churn that I reviewed at summary level rather than line by line. Issues Summary
🟢 Low Priority Notes (5 items)
Generated by Dyadbot persona-based code review |
|
Deflaked CI run https://github.com/dyad-sh/dyad/actions/runs/33116397081 in The merged Playwright report had 14 failing specs (16 unexpected results, 0 flaky passes). They shared two deterministic stale-baseline causes from the preceding change:
Regenerated only the 14 affected baselines and discarded an unrelated neighboring EOF rewrite. Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe4aa7ddbd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // logs, verification commands, sandbox scripts, or MCP servers. | ||
| if (isBuildMode) { | ||
| const readOnlyBuildTurn = isSecurityReviewIntent || isSummarizeIntent; | ||
| finishedNaturally = await handleLocalAgentStream( |
There was a problem hiding this comment.
When a Build turn has already completed a mutating tool such as write_file and the user then presses Stop, this new route leaves those edits on disk, but cancelTrackedStreams unconditionally publishes updatedFiles: false at lines 645-650. ChatStreamRemoteManager gates preview auto-expansion, reload, and capture on that flag, so a cancelled Build can leave the UI showing the pre-edit app even though its files changed. Track whether the agentic Build mutated the workspace and propagate that fact through the cancellation terminal event. rules/chat-modes.mdL7-L7
Useful? React with 👍 / 👎.
| @@ -482,13 +480,6 @@ export function WorkflowSettings() { | |||
| <DefaultChatModeSelector /> | |||
| </div> | |||
|
|
|||
There was a problem hiding this comment.
🟡 MEDIUM
Build loses its approval gate with no migration for users who kept auto-approve off
Build previously never touched the working tree until the user clicked Approve (autoApproveChanges defaults to falsy, so this was the behavior for most users). After this PR, Build runs the agentic loop under agentToolConsents, where write_file, search_replace, delete_file, rename_file, copy_file, add_integration, enable_nitro and restart_app all default to "always". The auto-approve switch and its settings row are removed, and src/lib/schemas.test.ts explicitly asserts that a stored autoApproveChanges value is NOT migrated into consents. The net effect is that a user who deliberately opted out of auto-approve now gets file writes and deletions applied without any prompt or proposal review, with no in-app notice and no obvious path back other than discovering the renamed "Build and Agent Permissions" settings section.
💡 Suggestion: Either migrate stored autoApproveChanges !== true into agentToolConsents "ask" for the mutating Build tools, or show a one-time notice pointing at Settings > Build and Agent Permissions the first time an agentic Build turn runs for a user who had auto-approve disabled.
| // Generate requestId early so it can be saved with the message | ||
| dyadRequestId = uuidv4(); | ||
| } | ||
| const willUseLocalAgentStream = isLocalAgentBackedMode(selectedChatMode); |
There was a problem hiding this comment.
🟡 MEDIUM
Unreachable legacy Build streaming path is retained behind a hard throw
isLocalAgentBackedMode now returns true for every ChatMode, and the new guard throws DyadErrorKind.Internal when it is false, so the entire legacy non-agentic streaming tail (full-codebase injection, smart-context/deep-context handling, proposal generation, and the autoApproveChanges auto-approval check around line 2742) is dead code. The added comment even states that none of its auto-approval or destructive-SQL checks are live safety boundaries. Leaving several hundred lines of unreachable code that still contains auto-approval and SQL-execution logic is a maintenance hazard: a later refactor that relaxes the guard would silently re-activate an unaudited approval path, and the dead ternaries it leaves behind (for example approvalState: willUseLocalAgentStream ? "approved" : null, and the unused MAX_CHAT_TURNS_IN_CONTEXT import) obscure the actual control flow.
💡 Suggestion: Delete the unreachable legacy branch in this PR, or file a tracked follow-up and mark the block clearly so it is removed once stored legacy responses no longer need migration support.
| @@ -941,9 +939,7 @@ export function ChatInput({ chatId }: { chatId?: number }) { | |||
| {/* Only render ChatInputActions if proposal is loaded and no pending consent */} | |||
| {!pendingToolConsent && | |||
| proposal && | |||
There was a problem hiding this comment.
🟡 MEDIUM
Legacy approve/reject proposal flow stays reachable but loses all automated coverage
The mode gate on ChatInputActions is removed, so the Approve/Reject bar now renders in any mode whenever getProposal returns a proposal, which still happens for pre-agentic assistant messages that have XML content, no aiMessagesJson, and a null approvalState. That path still calls processFullResponseActions and rejectProposal in proposal_handlers.ts. At the same time this PR deletes reject.integration.test.ts and the approveProposal/rejectProposal page objects, and rewrites approve.spec.ts to skip approval entirely, so the shipping approve and reject code now has zero unit, integration, or E2E coverage while remaining user-reachable for existing chats.
💡 Suggestion: Keep a minimal integration test for the legacy approve and reject handlers (or explicitly retire the UI and handlers if legacy proposals are no longer meant to be actionable).
|
|
||
| let cachedMcpToolDefs: McpToolDef[] = []; | ||
|
|
||
| export function getCachedMcpToolDefs(): McpToolDef[] { |
There was a problem hiding this comment.
🟡 MEDIUM
MCP token estimate reads a cache that is empty at startup and never invalidated
getCachedMcpToolDefs returns a module-level array that is only populated as a side effect of collectMcpToolDefs during an agent turn. token_count_handlers uses it to size MCP tool declarations, so after a fresh app launch the token bar reports zero MCP tokens for Agent mode until the user has run at least one Agent turn, even though those declarations are sent on the very first request. The cache is also never invalidated when a user enables, disables, or edits an MCP server, so the estimate can stay stale in the other direction for the rest of the session. Avoiding MCP connections during debounced counting is reasonable, but the resulting estimate is silently wrong in the common first-turn case.
💡 Suggestion: Invalidate the cache when MCP server settings change, and/or persist the last known tool defs so the first token count after launch is not systematically low.
| return match ? match[1] : null; | ||
| if (content.startsWith("Fix error: Error Line 6 error")) { | ||
| return "fix-runtime-error"; | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM
Prompt-content heuristics override explicit tc=local-agent fixture selection
extractLocalAgentFixture now checks four hardcoded prompt prefixes/substrings (fix-runtime-error, fix-all-runtime-errors, fix-typescript-errors, security-fix) before it looks for the explicit tc=local-agent/ marker. Any test whose prompt happens to contain a phrase like "TypeScript compile-time error" is silently routed to the fix-typescript-errors fixture even when it explicitly named a different one, and the failure mode is a confusing wrong-response assertion rather than a clear fixture-not-found error. Explicit selection should win over content sniffing.
💡 Suggestion: Match the explicit tc=local-agent/ (and tc=) markers first and only fall back to the generated-prompt heuristics when no marker is present.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues This PR routes Build through the native agentic loop with a fail-closed tool allowlist. The core mechanics hold up well under review: The issues below are all maintainability, migration, or test-coverage concerns rather than functional defects — none of them block merge. Note on confidence: the combined Issues Summary
🟢 Low Priority Notes (9 items)
Generated by Dyadbot persona-based code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 278 passed, 2 failed, 12 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/queued_message.spec.ts \
e2e-tests/setup_flow.spec.ts |
1 participant
Summary
Build mode now uses the same native agentic loop and lifecycle as Agent mode, with a deliberately smaller, fail-closed capability profile focused on building applications.