fix(orchestrator): forward credential input masks to tool node execution - #13151
Conversation
OrchestratorBlock._execute_single_tool_with_manager passed nodes_input_masks=None when calling on_node_execution for tool blocks. This meant credential metadata (stored in the graph execution's nodes_input_masks) was never forwarded to tool nodes, causing "missing credentials" errors when agents were launched from the Library or AutoPilot (where credentials come from stored presets via graph_credentials_inputs → nodes_input_masks). The Builder path worked because it sends credentials_inputs directly in the request body, which gets merged into nodes_input_masks at graph execution creation time — but the orchestrator's internal tool dispatch bypassed this by passing None. Fix: - Store nodes_input_masks on ExecutionProcessor during graph execution - Pass execution_processor.nodes_input_masks to on_node_execution in the orchestrator's tool dispatch path Fixes Significant-Gravitas#13144 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
|
This PR targets the Automatically setting the base branch to |
github-actions
Bot
added
platform/backend
|
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:
WalkthroughExecutionProcessor stores graph-level ChangesNode Input Mask Threading
Sequence Diagram(s)sequenceDiagram
participant GraphExecution
participant ExecutionProcessor
participant OrchestratorBlock
participant ExecutionManager
GraphExecution->>ExecutionProcessor: provide graph_exec.nodes_input_masks
OrchestratorBlock->>ExecutionProcessor: getattr(nodes_input_masks) -> mapping or None
OrchestratorBlock->>OrchestratorBlock: merge nodes_input_masks[sink_node_id] into node_exec_entry.inputs (if present)
OrchestratorBlock->>ExecutionProcessor: on_node_execution(node_exec_entry, nodes_input_masks)
ExecutionProcessor->>ExecutionManager: execute node with merged inputs and forwarded masks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a production issue where Orchestrator-triggered tool blocks lose credential metadata when runs are started from Library/AutoPilot (vs Builder) by ensuring node input masks (which include credential meta) are forwarded into the orchestrator’s internal tool-node execution path.
Changes:
- Persist
graph_exec.nodes_input_masksonto theExecutionProcessorinstance during graph execution setup. - Use
execution_processor.nodes_input_maskswhen the Orchestrator directly dispatches a tool node viaon_node_execution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| autogpt_platform/backend/backend/executor/manager.py | Stores graph_exec.nodes_input_masks on ExecutionProcessor for later access during nested/orchestrated dispatch. |
| autogpt_platform/backend/backend/blocks/orchestrator.py | Forwards nodes_input_masks into on_node_execution during orchestrator tool-node execution. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #13151 +/- ##
==========================================
- Coverage 72.33% 72.28% -0.05%
==========================================
Files 2299 2299
Lines 173127 173125 -2
Branches 17518 17519 +1
==========================================
- Hits 125235 125147 -88
- Misses 44177 44269 +92
+ Partials 3715 3709 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Verify that _execute_single_tool_with_manager forwards execution_processor.nodes_input_masks to on_node_execution, ensuring Library/AutoPilot credential overrides reach tool nodes. Covers: - Masks forwarded when present - None masks handled gracefully - ExecutionProcessor stores masks from graph_exec
|
|
![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)
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py (1)
217-217: ⚡ Quick winMove
ExecutionProcessorimport to module scope.This local import is not a heavy optional dependency and should be top-level for consistency with backend import rules.
As per coding guidelines, “Use top-level imports only — avoid local/inner imports except for lazy imports of heavy optional dependencies like
openpyxl.”🤖 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 `@autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py` at line 217, Move the local import of ExecutionProcessor to the module/top-level scope in the test module so it follows import guidelines; remove the in-function/local import and add "from backend.executor.manager import ExecutionProcessor" alongside the other top-level imports in test_orchestrator_credential_masks.py so the symbol is imported once at module load time.
🤖 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 `@autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py`:
- Around line 221-225: The test is only checking direct attribute assignment on
ExecutionProcessor instead of exercising the real flow that sets
nodes_input_masks from a GraphExecution; update the test to call the actual
method that receives the graph execution (use
ExecutionProcessor._on_graph_execution or whichever public initializer handles
graph_exec) with a fake/mocked graph_exec containing
nodes_input_masks==expected_masks, then assert that processor.nodes_input_masks
is expected_masks; reference ExecutionProcessor, _on_graph_execution (or the
real method that accepts graph_exec), graph_exec.nodes_input_masks and
nodes_input_masks in the test to ensure the production wiring is validated.
---
Nitpick comments:
In `@autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py`:
- Line 217: Move the local import of ExecutionProcessor to the module/top-level
scope in the test module so it follows import guidelines; remove the
in-function/local import and add "from backend.executor.manager import
ExecutionProcessor" alongside the other top-level imports in
test_orchestrator_credential_masks.py so the symbol is imported once at module
load time.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbaeeb35-841f-408d-85e9-9f7847e4c75f
📒 Files selected for processing (1)
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: check API types
- GitHub Check: Check PR Status
- GitHub Check: end-to-end tests
- GitHub Check: test (3.13)
- GitHub Check: Analyze (typescript)
- GitHub Check: test (3.12)
- GitHub Check: type-check (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: type-check (3.13)
- GitHub Check: test (3.11)
- GitHub Check: type-check (3.11)
🧰 Additional context used
📓 Path-based instructions (4)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
autogpt_platform/backend/**/*.py: Usepoetry run ...command for executing Python package dependencies
Use top-level imports only — avoid local/inner imports except for lazy imports of heavy optional dependencies likeopenpyxl
Use absolute imports withfrom backend.module import ...for cross-package imports; single-dot relative imports are acceptable for sibling modules within the same package; avoid double-dot relative imports
Do not use duck typing — avoidhasattr/getattr/isinstancefor type dispatch; use typed interfaces/unions/protocols instead
Use Pydantic models over dataclass/namedtuple/dict for structured data
Do not use linter suppressors — no# type: ignore,# noqa,# pyright: ignore; fix the type/code instead
Prefer list comprehensions over manual loop-and-append patterns
Use early return with guard clauses first to avoid deep nesting
Use%sfor deferred interpolation indebuglog statements for efficiency; use f-strings elsewhere for readability (e.g.,logger.debug("Processing %s items", count)vslogger.info(f"Processing {count} items"))
Sanitize error paths by usingos.path.basename()in error messages to avoid leaking directory structure
Be aware of TOCTOU (Time-Of-Check-Time-Of-Use) issues — avoid check-then-act patterns for file access and credit charging
Usetransaction=Truefor Redis pipelines to ensure atomicity on multi-step operations
Usemax(0, value)guards for computed values that should never be negative
Keep files under ~300 lines; if a file grows beyond this, split by responsibility (extract helpers, models, or a sub-module into a new file)
Keep functions under ~40 lines; extract named helpers when a function grows longer
...
Files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
autogpt_platform/backend/**/test/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use snapshot testing with '--snapshot-update' flag in backend tests when output changes; always review with 'git diff'
Files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
autogpt_platform/backend/**/test_*.py
📄 CodeRabbit inference engine (autogpt_platform/AGENTS.md)
Create a failing test first using
@pytest.mark.xfaildecorator (backend) when fixing a bug or adding a feature, then implement the fix and remove the xfail marker
Files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
🧠 Learnings (9)
📚 Learning: 2026-02-26T17:02:22.448Z
Learnt from: Pwuts
Repo: Significant-Gravitas/AutoGPT PR: 12211
File: .pre-commit-config.yaml:160-179
Timestamp: 2026-02-26T17:02:22.448Z
Learning: Keep the pre-commit hook pattern broad for autogpt_platform/backend to ensure OpenAPI schema changes are captured. Do not narrow to backend/api/ alone, since the generated schema depends on Pydantic models across multiple directories (backend/data/, backend/blocks/, backend/copilot/, backend/integrations/, backend/util/). Narrowing could miss schema changes and cause frontend type desynchronization.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📚 Learning: 2026-03-05T15:42:08.207Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12297
File: .claude/skills/backend-check/SKILL.md:14-16
Timestamp: 2026-03-05T15:42:08.207Z
Learning: In Python files under autogpt_platform/backend (recursively), rely on poetry run format to perform formatting (Black + isort) and linting (ruff). Do not run poetry run lint as a separate step after poetry run format, since format already includes linting checks.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📚 Learning: 2026-03-16T16:30:11.452Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 12417
File: autogpt_platform/backend/backend/blocks/agent_mail/threads.py:80-102
Timestamp: 2026-03-16T16:30:11.452Z
Learning: Do not wrap synchronous AgentMail SDK calls with asyncio.to_thread() in blocks under autogpt_platform/backend/backend/blocks (and across the codebase). The block executor runs node execution in dedicated threads via asyncio.run_coroutine_threadsafe (see manager.py around lines ~745-752 and ~1079). The existing pattern avoids using asyncio.to_thread for SDK calls inside async run() methods, so maintain that approach and do not add to_thread usage in these code paths.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📚 Learning: 2026-03-16T16:35:40.236Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12440
File: autogpt_platform/backend/backend/api/features/workflow_import.py:54-63
Timestamp: 2026-03-16T16:35:40.236Z
Learning: Avoid using the word 'competitor' in public-facing identifiers and text. Use neutral naming for API paths, model names, function names, and UI text. Examples: rename 'CompetitorFormat' to 'SourcePlatform', 'convert_competitor_workflow' to 'convert_workflow', '/competitor-workflow' to '/workflow'. Apply this guideline to files under autogpt_platform/backend and autogpt_platform/frontend.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📚 Learning: 2026-03-19T15:10:50.676Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12483
File: autogpt_platform/backend/backend/copilot/tools/test_dry_run.py:298-303
Timestamp: 2026-03-19T15:10:50.676Z
Learning: When using Python’s `unittest.mock.patch` in tests, choose the patch target based on how the imported name is resolved:
- If the code under test uses an **eager/module-level import** (e.g., `from foo.bar import baz` at module top), patch **the module where the name is looked up** (i.e., where it is used in the SUT), e.g. `patch("mymodule.baz")`.
- If the code under test uses a **lazy import** executed later (e.g., `from foo.bar import baz` inside a function/branch), patch **the source module** (e.g., `patch("foo.bar.baz")`) because the late `from ... import` will read the (potentially patched) name from the source module at call time.
For a concrete example: if `simulate_block` is imported inside an `if dry_run:` block in the SUT, then the correct test patch target is the source module path for `simulate_block` as it exists at call time (e.g., `patch("backend.executor.simulator.simulate_block")`), not the test file’s import location.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📚 Learning: 2026-04-15T02:43:36.890Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12780
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:0-0
Timestamp: 2026-04-15T02:43:36.890Z
Learning: When reviewing Python exception handlers, do not flag `isinstance(e, X)` checks as dead/unreachable if the caught exception `X` is a subclass of the exception type being handled. For example, if `X` (e.g., `VirusScanError`) inherits from `ValueError` (directly or via an intermediate class) and it can be raised within an `except ValueError:` block, then `isinstance(e, X)` inside that handler is reachable and should not be treated as dead code.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📚 Learning: 2026-04-22T11:46:04.431Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12881
File: autogpt_platform/backend/backend/copilot/config.py:0-0
Timestamp: 2026-04-22T11:46:04.431Z
Learning: Do not flag the Claude Sonnet 4.6 model ID as incorrect when it uses the project’s established hyphenated convention: `anthropic/claude-sonnet-4-6`. This hyphen form is the intentional, production convention and should be treated as valid (including in files like llm.py, blocks tests, reasoning.py, `_is_anthropic_model` tests, and config defaults). Note that OpenRouter also accepts the dot variant `anthropic/claude-sonnet-4.6`, so either form may be tolerated, but `anthropic/claude-sonnet-4-6` should be considered the standard to match project usage.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📚 Learning: 2026-04-22T11:46:12.892Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12881
File: autogpt_platform/backend/backend/copilot/baseline/service.py:322-332
Timestamp: 2026-04-22T11:46:12.892Z
Learning: In this codebase (Significant-Gravitas/AutoGPT), OpenRouter-routed Anthropic model IDs should use the hyphen-separated convention (e.g., `anthropic/claude-sonnet-4-6`, `anthropic/claude-opus-4-6`). Although OpenRouter may accept both hyphen and dot variants, treat the hyphen-separated form as the intended, correct codebase-wide convention and do not flag it as an error. Only flag the dot-separated variant (e.g., `anthropic/claude-sonnet-4.6`) as incorrect when reviewing/validating model ID strings for OpenRouter-routed Anthropic models.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
📚 Learning: 2026-05-07T18:48:14.242Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13040
File: autogpt_platform/backend/backend/blocks/llm.py:0-0
Timestamp: 2026-05-07T18:48:14.242Z
Learning: In this repository, isort may split imports from the same module into separate blocks when some imports are aliased (e.g., `from module import X as Y`) and others are not. Preserve the two-block layout when it results from isort (such as keeping `from openai.types.chat import ChatCompletion as OpenAIChatCompletion` separate from non-aliased imports from `openai.types.chat`). Do not treat that split as a style issue during review; merging them into a single block can fail CI with `Imports are incorrectly sorted and/or formatted`.
Applied to files:
autogpt_platform/backend/test/blocks/test_orchestrator_credential_masks.py
The orchestrator dispatch path forwarded nodes_input_masks to on_node_execution but never merged the sink node's mask into node_exec_entry.inputs. The normal queue-based path in _on_graph_execution does `queued_node_exec.inputs.update(node_input_mask)` before execution — this commit mirrors that behavior so credential fields from Library/AutoPilot are present for the block run. Also rewrites tests to verify masks are actually merged into inputs (not just forwarded), adds edge case for masks targeting different nodes, and removes tautological ExecutionProcessor attribute test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Pushed a follow-up for the failing backend CI checks. Changes:
Local checks run: I also attempted the targeted pytest run, but this local checkout is missing the generated Prisma/client dependency ( |
|
@mangod12 why was I tagged? |
|
For review if anyone else has not taken up this pr and the solution is good |
Adds two regression tests for _execute_single_tool_with_manager that exercise the new defensive paths in orchestrator.py: - non-Mapping nodes_input_masks is coerced to None (lines 1113-1114) - empty Mapping masks skip the merge but are still forwarded Brings codecov/patch coverage on the orchestrator change from 60% to 100% on the new branch logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dentials-library-autopilot
…patch tests The tool-dispatch tests left charge_node_usage as a plain MagicMock and get_execution_outputs_by_node_exec_id unmocked, so the post-execution path raised TypeError (await on MagicMock) and json.dumps recursed on a MagicMock. The swallowed failure produced ~2390 warnings and made the recorded on_node_execution call args unreliable under an exhausted recursion stack. Mock both so the tool path completes cleanly and assertions are deterministic.
E2E Test Report — PR #13151 (NATIVE mode, --fix)Tested the orchestrator credential-mask forwarding fix natively (docker infra deps +
Final: 8/8 scenarios pass. Orchestrator unit suites: Runtime data-path assertions (against the running native backend)Confirmed the PR's wiring is present in the loaded modules:
Fix applied during this run (--fix mode)The PR's own test file left two mocks incomplete: ScreenshotsLogin Page
Authenticated Copilot / AutoPilot Home
Library (Agents) Page
|
majdyz
left a comment
majdyz
left a comment
There was a problem hiding this comment.
E2E Test Evaluation — APPROVED
Results: 8/8 scenarios passed (native mode).
Coverage: The fix forwards nodes_input_masks from ExecutionProcessor to orchestrator tool-node dispatch. Verified end to end: the unit suite covers the merge path and three negative paths (other-node isolation, non-Mapping coercion, empty-mask), and a runtime assertion against the running native backend confirms manager.py stores graph_exec.nodes_input_masks, the orchestrator reads it through the Mapping guard, merges the sink-node mask into inputs, and forwards the masks (no longer hardcoded None).
Negative tests: Other-node mask isolation, non-Mapping coercion, and empty-mask no-merge all pass.
Evidence: Source/runtime assertions logged for the data path; full-stack regression screenshots for login + authenticated Copilot/AutoPilot home + Library — the two entry points this fix targets — all healthy.
Regressions: None. Orchestrator unit suites 27 passed (credential-masks + execution-mode).
Note (addressed in this run): The PR's test file left charge_node_usage and get_execution_outputs_by_node_exec_id unmocked, so the tool-dispatch path crashed mid-flight (TypeError on awaiting a MagicMock + json.dumps recursion), emitting ~2390 warnings and making on_node_execution call-args nondeterministic. Hardened the fixtures (commit 17b3d00) — warnings 2392 → 2, all 5 tests deterministic. The product change itself was already correct.
github-project-automation
Bot
moved this from 🆕 Needs initial review
to 👍🏼 Mergeable
in AutoGPT development kanban
… getattr/isinstance ExecutionProcessor's per-graph-execution state attributes are populated in on_graph_execution; nodes_input_masks now has a class-level annotation (`Optional[NodesInputMasks] = None`) matching how the other state holders (running_node_execution, execution_stats, execution_stats_lock) are accessed from the tool-dispatch path. The orchestrator can now read it directly, and the runtime isinstance(..., Mapping) check is dropped — NodesInputMasks is already typed as Mapping[str, NodeInputMask], so the guard was paranoia against a type-system lie. Drops the matching defensive test.
… compat Removing the isinstance(..., Mapping) check broke three test shards: orchestrator tests pass MagicMock/AsyncMock processors whose attribute chain returns coroutines under AsyncMock semantics, and node_exec_entry.inputs.update(...) on those crashed with 'coroutine object is not iterable'. The coercion is a small runtime safety boundary that's load-bearing for the test harness; keep it alongside the typed direct attribute access. Restores the matching test.
majdyz
left a comment
majdyz
left a comment
There was a problem hiding this comment.
Re-approving after the refactor commit. e3f87ac3f9 swaps the getattr-based duck-typed access for typed attribute access (ExecutionProcessor.nodes_input_masks: Optional[NodesInputMasks] = None), so callers get pyright-clean reads. The isinstance(..., Mapping) coercion is kept — orchestrator tests pass MagicMock/AsyncMock processors where the attribute chain otherwise returns a coroutine, breaking inputs.update(...). All required CI green; only Vercel fails (non-required fork-preview auth limitation, this PR is backend-only).
bef67a1
github-project-automation
Bot
moved this from 👍🏼 Mergeable
to ✅ Done
in AutoGPT development kanban
Summary
OrchestratorBlock._execute_single_tool_with_managerpassednodes_input_masks=Nonetoon_node_execution, so credential metadata was never forwarded to tool nodesWhy Builder worked but Library/AutoPilot didn't
credentials_inputsin the request body → gets merged intonodes_input_masksat execution creation time → flows through the normal execution pathnodes_input_maskscorrectly (viamake_node_credentials_input_map) → stored ongraph_exec.nodes_input_masksnodes_input_masks=Noneatorchestrator.py:1145Fix (2 lines)
executor/manager.py: Storegraph_exec.nodes_input_masksonExecutionProcessorso it's accessible during orchestrator tool dispatchblocks/orchestrator.py: Passexecution_processor.nodes_input_masksinstead ofNoneFixes #13144
Test plan
execute_graph→add_graph_execution→validate_and_construct_node_execution_input→make_node_credentials_input_map→nodes_input_masks→on_node_executionNonefornodes_input_masksmanager.py:1107)🤖 Generated with Claude Code