fix(copilot): fix dry-run simulation showing INCOMPLETE/error status - #12580
Conversation
The dry-run block simulation was showing misleading "INCOMPLETE" status and "error" output even on successful simulations. Root causes: 1. The simulator always includes an empty "error" pin (set to "") for blocks that define one. This caused the frontend to render a misleading "error" output section and the LLM to misinterpret the result as a failure, reporting "INCOMPLETE" to the user. 2. The frontend's isRunBlockErrorOutput() check was too broad — using `"error" in output` which could match non-error responses. 3. The parseOutput() fallback for untyped payloads could incorrectly classify a BlockOutputResponse as an ErrorResponse. Fixes: - Backend: Strip empty "error" pins from dry-run outputs and add explicit "Status: COMPLETED" to the response message - Frontend: Tighten isRunBlockErrorOutput() to only match actual error responses (type=error), not block outputs with error in outputs dict - Frontend: Fix parseOutput() to exclude BlockOutputResponse from error fallback matching - Frontend: Filter empty error pins from BlockOutputCard display and output key counting in accordion metadata
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
github-actions
Bot
added
platform/frontend
…lper - Scope empty-error pin filter to dry-run only in BlockOutputCard (real executions keep empty error as meaningful "no error" signal) - Extract shared isEmptyErrorPin() helper in helpers.tsx for DRY - Add vacuous truth guard (and v) to backend filter for clarity
🔍 PR Overlap DetectionThis check compares your PR against all other open PRs targeting the same branch to detect potential merge conflicts early. 🔴 Merge Conflicts DetectedThe following PRs have been tested and will have merge conflicts if merged after this PR. Consider coordinating with the authors.
Summary: 2 conflict(s), 0 medium risk, 0 low risk (out of 2 PRs with file overlap) Auto-generated on push. Ignores: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSimulator and dry-run handling were tightened: empty Changes
Sequence DiagramsequenceDiagram
participant FE as Frontend (RunBlock)
participant Cop as Copilot Tools (execute_block)
participant Sim as Executor Simulator
participant Resp as Response Processing
FE->>Cop: execute_block(..., dry_run=True)
Cop->>Sim: invoke simulator with prompt (exclude "error" from MUST include)
Sim->>Sim: execute, parse outputs, omit blank "error" pins
Sim-->>Cop: yield (pin, value) tuples (no empty "error")
Cop->>Resp: assemble outputs, include non-empty "error" if present, set dry-run message with COMPLETED
Resp-->>FE: return BlockOutputResponse
FE->>FE: detect true errors via ResponseType.error and render
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…un only Aligns getAccordionMeta with BlockOutputCard: only filter out empty error pins for dry-run outputs. For real executions, empty error pins are meaningful signals and should be counted in accordion metadata.
…x/dry-run-simulation-streaming
E2E Test ReportDate: 2026-03-26 | Branch: Test Results
Key Verification
4/4 scenarios passed. Bug confirmed fixed. |
majdyz
left a comment
majdyz
left a comment
There was a problem hiding this comment.
🤖 PR Review — Round 2 
Scope: Full re-review of all 4 changed files after addressing Round 1 feedback.
Backend (helpers.py)
- Empty error pin filter (L123-127): Correctly strips
errorkeys where all values are"". Theand vguard prevents vacuous truth on empty lists. The SIMULATOR ERROR detection above (L106-116) runs first, so real simulator failures are never accidentally stripped. Sound. - Message update (L131-133): Explicit
Status: COMPLETED.prevents LLM misinterpretation. Clear and unambiguous. - f-string consolidation (L553): Minor cleanup, no behavioral change. Good.
Backend tests (test_dry_run.py)
- Three new tests (
test_execute_block_dry_run_filters_empty_error_pin,test_execute_block_dry_run_keeps_nonempty_error_pin,test_execute_block_dry_run_message_includes_completed_status) cover the primary scenarios. The tuple formatting fix intest_execute_block_dry_run_simulator_error_returns_error_responseis cosmetic only.
Frontend (helpers.tsx)
isRunBlockErrorOutput(L99-113): Properly narrowed —type === erroris the primary check; fallback requires notypefield AND noblock_id. This preventsBlockOutputResponsemisclassification. Correct.parseOutputfallback (L137): Added!("block_id" in output)guard. Order of checks matters:block_idmatch at L132 runs first, so this guard is defense-in-depth for edge cases whereblock_id+errorboth exist but the type discriminator check at L128 didn't match. Sound.isEmptyErrorPin(L224-231): Clean helper.v == nullcovers bothnullandundefined.items.length > 0prevents vacuous truth. Used consistently in bothBlockOutputCardandgetAccordionMeta.getAccordionMeta(L254-258): Correctly scoped tooutput.is_dry_runonly — matchesBlockOutputCardbehavior.
Frontend (BlockOutputCard.tsx)
- Filter at L123-125 correctly scoped to
output.is_dry_run. Import placement is clean (relative import after@/imports).
Verdict
All Round 1 findings have been addressed. No new issues found. CI is fully green (all 30 checks pass including tests on Python 3.11/3.12/3.13, type-check, lint, e2e, integration). The code is clean, well-tested, and ready to merge.
…atching downstream The simulator was yielding empty "error" pins (error="") for every block with an error output, then helpers.py and the frontend had to filter them out. This moves the fix to the source: the simulator now omits empty/blank error pins entirely, so no downstream patching is needed. - simulator.py: skip yielding error pins with empty/blank string values - helpers.py: remove the clean_outputs filtering (no longer needed) - helpers.tsx: remove isEmptyErrorPin helper and dry-run-specific filtering - BlockOutputCard.tsx: remove dry-run-specific empty error pin filter - tests updated to reflect new behavior
|
A review is already queued or running for this commit (6d4f8a4). |
…x/dry-run-simulation-streaming
|
/review |
|
/review |
|
I couldn't load the latest pull request details from GitHub, so the review was not queued: Failed to load pull request metadata: Server error '504 Gateway Timeout' for url 'https://api.github.com/repos/Significant-Gravitas/AutoGPT/pulls/12580' |
Review Summary3 rounds of review completed. PR is clean after all previous iterations. Round 1-3 findings: 0 new issuesAll 12 review threads from prior rounds are resolved. The code is solid:
CI: All checks pass (tests, lint, type-check, e2e, integration). |
|
/review |
github-actions
Bot
added
the
conflicts
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
…x/dry-run-simulation-streaming # Conflicts: # autogpt_platform/backend/backend/copilot/tools/helpers.py # autogpt_platform/backend/backend/copilot/tools/test_dry_run.py
github-actions
Bot
removed
the
conflicts
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
|
/review |
![autogpt-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/130738209?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA has been stuck at step 181 for ~15 minutes. It successfully navigated the UI (login, copilot page, build page) and took screenshots, but hit a wall with the copilot reasoning (likely no OpenAI API key) and then stalled on the build page. I have 7/8 complete reports plus QA's partial observations. Time to compile the verdict.
PR #12580 — fix(copilot): fix dry-run simulation showing INCOMPLETE/error status
Author: majdyz | Files: simulator.py (+30/-8), helpers.tsx (+18/-4), useRunGraph.ts (+9/-0), useFlowRealtime.ts (+13/-1), helpers.py (+1/-0), test_dry_run.py (+112/-6)
🎯 Verdict: APPROVE
What This PR Does
Dry-run simulations in the copilot were incorrectly showing INCOMPLETE/error status even when the simulation succeeded. Three root causes are fixed: (1) the backend simulator was emitting empty error: "" pins that downstream consumers interpreted as real errors, (2) the frontend isRunBlockErrorOutput type guard was over-broad — any response with an "error" key was classified as an error, even successful BlockOutputResponse objects with an error output pin, and (3) stale results from previous runs persisted in the UI. A race condition where fast-completing dry-runs finished before the WebSocket subscription was established is also closed.
Specialist Findings
🛡️ Security ✅ — No auth changes, no new inputs, no secrets exposure. The LLM prompt change (simulator.py:124) shifts from "set error to empty string" to "OMIT error entirely" which is safe — input_data is still escaped via json.dumps through _truncate_input_values, and output property names come from Python class definitions (not user input). The strip() guard (simulator.py:185) only fires on isinstance(value, str) values. The isRunBlockErrorOutput fallback using block_id presence is a structural heuristic rather than explicit type discrimination, but it's strictly better than the old "error" in output check.
simulator.py:~191 — No log line when error pins are stripped. Recommend adding logger.debug() for observability.
🏗️ Architecture ✅ — Source-level filtering at the simulator is the correct layer — follows "make the wrong thing hard to do" by preventing empty error pins from ever reaching consumers. Frontend changes are well-layered: useRunGraph.ts owns execution lifecycle, useFlowRealtime.ts owns real-time sync, helpers.tsx owns type discrimination. The AsyncIterator → AsyncGenerator type annotation change is correct (function uses yield).
helpers.tsx:111-114 — block_id as a type discriminator is a structural typing hack. Both BlockOutputResponse and ErrorResponse have type?: ResponseType (optional). Works today because block_id is required on BlockOutputResponse, but fragile if ErrorResponse ever gains a block_id field. Pre-existing debt made less fragile by this PR. Consider making type required server-side long-term.
useRunGraph.ts:145-148 — clearAllNodeExecutionResults() + cleanNodesStatuses() only fire in the else branch (no inputs/credentials or dry-run). The if branch (opens dialog) doesn't clear stale state. Not a regression — pre-existing — but inconsistent. Both architect and product flagged this independently, strengthening the signal.
⚡ Performance ✅ — All changes are in the dry-run/simulation path, not the production execution hot path. strip() on error pins is O(k) on short strings, negligible. invalidateQueries is O(1) trigger with React Query deduplication. Two concerns noted:
useRunGraph.ts:148-149 — Two separate O(N) nodes.map() passes creating 2×N node objects. Mergeable into a single set() call to halve allocations and avoid an intermediate Zustand render. Minor optimization opportunity for large graphs.
useFlowRealtime.ts:80-87 — invalidateQueries fires on every WebSocket reconnect (including reconnects after heartbeat timeouts), not just initial subscription. On flaky networks this could cause a burst of refetches. React Query deduplication mitigates this naturally, but consider guarding against invalidation when execution is already terminal.
🧪 Testing
- ✅
test_simulate_block_keeps_nonempty_error— verifies non-empty errors pass through - ✅
test_build_simulation_prompt_excludes_error_from_must_include— validates prompt correctness - ✅
test_execute_block_dry_run_no_empty_error_from_simulator— end-to-end passthrough ⚠️ Missing: whitespace-only error string test —simulator.py:185usesstrip()to catch" ","\n"etc., but no test exercises this boundary. Should addtest_simulate_block_drops_whitespace_only_error.⚠️ Missing: non-string error value test —simulator.py:184checksisinstance(value, str)soNone,0,{}would pass through. Should document whether that's intentional.⚠️ Missing:isRunBlockErrorOutputunit tests — Nohelpers.test.tsexists inRunBlock/. This function is the root-cause fix for the UI mis-classification and has no tests at all. Should test:BlockOutputResponsewitherrorin outputs +block_id→false;ErrorResponsewithtype: "error"→true; untyped witherror+ noblock_id→true.⚠️ Missing: frontend integration tests for stale-result clearing andinvalidateQueriesrace condition fix.
📖 Quality ✅ — Readability score: A. Comments consistently explain why, not just what. The helpers.tsx:102-110 comment block is long but justified — it documents a subtle type-discrimination bug for future maintainers. The simulator.py:185-187 comment explaining strip() vs == "" with examples is excellent.
AsyncGenerator import uses collections.abc (PEP 585 modern form) while some other files in the codebase use typing.AsyncGenerator. Correct directionally but inconsistent. Not blocking.
📦 Product ✅ — All three root causes are addressed with matching code changes. The UX improvement is clear: dry-runs that succeed now show success instead of confusing INCOMPLETE/error status. Non-empty errors (real simulation failures) are preserved. The race condition fix ensures fast dry-runs display results on first click.
isGraphRunning. Imperceptible in practice.
📬 Discussion ✅ — 27/29 CI checks pass (2 skipping: Vercel Agent Review, Chromatic — expected). No merge conflicts. Author (@majdyz) self-reviewed across 4 rounds addressing 24 findings + 2 CodeRabbit nitpicks. Zero independent human reviews. Author posted extensive manual test reports (4/4 scenarios pass, 17/17 unit tests, lint clean). Docstring coverage at 65.22% vs 80% threshold (pre-existing, non-blocking).
🔎 QA curl localhost:8006/health returned OK). Screenshots were taken of: landing page, login, dashboard, copilot chat, copilot reasoning state, and build page.
- Limitation: Could not exercise the actual dry-run flow due to environment constraints (no OpenAI API key for copilot backend). The core fix is well-covered by unit tests but lacks live validation.
Blockers (Must Fix)
None.
Should Fix (Follow-up OK)
helpers.tsx:102-116— Add unit tests forisRunBlockErrorOutputreclassification logic (the root-cause frontend fix has zero test coverage)simulator.py:~191— Addlogger.debug("Dropping empty error pin for block=%s", block_name)when error pins are strippedtest_dry_run.py— Add test for whitespace-only error string:{"error": " "}should be droppedtest_dry_run.py— Add test for non-string error value (None,0,{}) to document intended behavioruseRunGraph.ts:148-149— Consider mergingclearAllNodeExecutionResults()+cleanNodesStatuses()into single store update to avoid double renderuseRunGraph.ts— Consider clearing stale results in the dialog submit path too (currently onlyelsebranch)
Risk Assessment
Merge risk: LOW | Rollback: EASY
The changes are well-scoped to the dry-run/simulation path with no impact on real block execution. Three independent fixes address three independent root causes — any one of them alone would improve the UX. Backend changes have strong test coverage. Frontend changes are logical and well-commented but lack unit tests — recommend adding those as a fast follow-up. The block_id-as-discriminator pattern is acknowledged as fragile but strictly better than the previous approach. CI is green. Author has been thorough in self-review (4 rounds, 24 findings addressed).
REVIEW_COMPLETE
PR: #12580
Verdict: APPROVE
Blockers: 0
github-project-automation
Bot
moved this from 🆕 Needs initial review
to 👍🏼 Mergeable
in AutoGPT development kanban
github-project-automation
Bot
moved this from 👍🏼 Mergeable
to ✅ Done
in AutoGPT development kanban
2 participants
Summary
errorpins from dry-run simulation outputs that the simulator always includes (set to""meaning "no error"). This was causing the LLM to misinterpret successful simulations as failures and report "INCOMPLETE" status to userserrorfrom the "MUST include" keys list, and instruct LLM to omit error unless simulating a logical failureisRunBlockErrorOutput()type guard that was too broad ("error" in outputmatched BlockOutputResponse objects, not just ErrorResponse), causing dry-run results to be displayed as errorsparseOutput()fallback matching to not classify BlockOutputResponse as ErrorResponseBlockOutputCarddisplay and accordion metadata output key countingTest plan
poetry run pytest backend/copilot/tools/test_dry_run.py -x -v)poetry run pytest backend/copilot/tools/helpers_test.py -x -v)poetry run pytest backend/copilot/tools/run_block_test.py -x -v)