fix(backend): make graph save atomic + surface clear credential errors#13264
fix(backend): make graph save atomic + surface clear credential errors#13264Pwuts wants to merge 14 commits into
Conversation
…ded on graph activation (SECRT-2370) OAuth-refresh failures during graph activation (e.g. revoked refresh token producing `invalid_grant: Bad Request`) were propagating as opaque 500s and were not handled the same way as a missing credential. They now raise a `GraphActivationError` naming the credential, node, and asking the user to reconnect the integration. Optional credential fields are still cleared in the same way when a refresh fails, so they don't block a save. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ve (SECRT-2370)
Save and activation were happening in the wrong order: `graph_db.create_graph`
+ `create_library_agent` ran first, and only then was `on_graph_activate`
called to validate node credentials. A credential failure there left a half-
saved graph (and library agent) in the DB while the user got an opaque 500
back, and any input_default cleanups performed by activation (clearing stale
optional credentials) were also lost because the save had already happened.
Move activation in front of the persistence step in the four call sites that
create or version a graph (`POST /api/graphs`, `PUT /api/graphs/{id}`,
external API `POST /api/graphs`, `create_graph_in_library`,
`update_graph_in_library`). When activation fails the route now returns a
400 with the `GraphActivationError` message instead of a 500, and nothing
is persisted. `set_graph_active_version` (which has nothing to roll back)
also maps `GraphActivationError` to 400.
`fork_library_agent` still activates after the fork's own transaction —
splitting that would mean reaching into the data layer, so it's left as a
follow-up.
…due-to-agent-image-generation
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
|
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:
WalkthroughThis PR moves graph activation from post-persistence to pre-persistence by introducing ChangesGraph Activation Pre-persist and Credential Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
🔍 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: 3 conflict(s), 0 medium risk, 0 low risk (out of 3 PRs with file overlap) Auto-generated on push. Ignores: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #13264 +/- ##
==========================================
+ Coverage 72.55% 72.57% +0.01%
==========================================
Files 2349 2350 +1
Lines 174823 174954 +131
Branches 17726 17735 +9
==========================================
+ Hits 126836 126965 +129
- Misses 44220 44224 +4
+ Partials 3767 3765 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Pwuts
changed the title
![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/backend/api/features/library/db_test.py (1)
307-313: ⚡ Quick winAdd a companion test for the active-graph branch.
Setting
is_active=Falsekeeps this regression focused on archived agents, but it also means the new pre-persist activation path inupdate_graph_in_library()is no longer exercised here. Please add a sibling case withis_active=Truethat assertson_graph_activateruns beforegraph_db.create_graph.🤖 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/backend/api/features/library/db_test.py` around lines 307 - 313, Add a sibling test that mirrors test_update_graph_in_library_allows_archived_library_agent but sets existing_version.is_active=True (and graph_model.is_active=True if needed) to exercise the active-graph branch in update_graph_in_library; in that new test assert that on_graph_activate is called before graph_db.create_graph (use mocks and call order assertions or a sentinel to verify order) and name it e.g. test_update_graph_in_library_runs_on_activate_for_active_agent so it verifies on_graph_activate runs prior to invoking graph_db.create_graph.
🤖 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/backend/api/features/v1.py`:
- Around line 1767-1770: The activation call to
on_graph_activate(new_active_graph, user_id=user_id) may return a
rewritten/cleaned graph but its result is discarded, so subsequent calls (e.g.,
update_library_agent_version_and_settings(...)) still use the stale
new_active_graph; change the try block to capture the returned graph (e.g.,
activated_graph = await on_graph_activate(...)) and then use that
activated_graph in downstream calls, preserving the same GraphActivationError
handling (raise HTTPException(... ) from e).
---
Nitpick comments:
In `@autogpt_platform/backend/backend/api/features/library/db_test.py`:
- Around line 307-313: Add a sibling test that mirrors
test_update_graph_in_library_allows_archived_library_agent but sets
existing_version.is_active=True (and graph_model.is_active=True if needed) to
exercise the active-graph branch in update_graph_in_library; in that new test
assert that on_graph_activate is called before graph_db.create_graph (use mocks
and call order assertions or a sentinel to verify order) and name it e.g.
test_update_graph_in_library_runs_on_activate_for_active_agent so it verifies
on_graph_activate runs prior to invoking graph_db.create_graph.
🪄 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: 09252676-dcad-4fbe-a486-acf8ae63d120
📒 Files selected for processing (6)
autogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/api/features/library/db.pyautogpt_platform/backend/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.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: type-check (3.13)
- GitHub Check: type-check (3.11)
- GitHub Check: type-check (3.12)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: end-to-end tests
- GitHub Check: Analyze (python)
- GitHub Check: Check PR Status
- GitHub Check: Analyze (typescript)
🧰 Additional context used
📓 Path-based instructions (5)
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/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.py
autogpt_platform/backend/backend/api/features/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update routes in '/backend/backend/api/features/' and add/update Pydantic models in the same directory for API development
Files:
autogpt_platform/backend/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.py
autogpt_platform/backend/**/api/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/api/**/*.py: UseSecurity()instead ofDepends()for authentication dependencies to get proper OpenAPI security specification
Follow SSE (Server-Sent Events) protocol: usedata:lines for frontend-parsed events (must match Zod schema) and: commentlines for heartbeats/status
Files:
autogpt_platform/backend/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.py
autogpt_platform/backend/**/*_test.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/*_test.py: Use pytest with snapshot testing for API responses
Colocate test files with source files using*_test.pynaming convention
Mock at boundaries — mock where the symbol is used, not where it's defined; after refactoring, update mock targets to match new module paths
UseAsyncMockfromunittest.mockfor async functions in tests
When writing tests, use Test-Driven Development (TDD): write failing tests marked with@pytest.mark.xfailbefore implementation, then remove the marker once the implementation is complete
When creating snapshots in tests, usepoetry run pytest path/to/test.py --snapshot-update; always review snapshot changes withgit diffbefore committing
Files:
autogpt_platform/backend/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.py
🧠 Learnings (10)
📚 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/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.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/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.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/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.py
📚 Learning: 2026-03-31T15:37:38.626Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12623
File: autogpt_platform/backend/backend/copilot/tools/agent_generator/fixer.py:37-47
Timestamp: 2026-03-31T15:37:38.626Z
Learning: When validating/constructing Anthropic API model IDs in Significant-Gravitas/AutoGPT, allow the hyphen-separated Claude Opus 4.6 model ID `claude-opus-4-6` (it corresponds to `LlmModel.CLAUDE_4_6_OPUS` in `autogpt_platform/backend/backend/blocks/llm.py`). Do NOT require the dot-separated form in Anthropic contexts. Only OpenRouter routing variants should use the dot separator (e.g., `anthropic/claude-opus-4.6`); `claude-opus-4-6` should be treated as correct when passed to Anthropic, and flagged only if it’s used in the OpenRouter path where the dot form is expected.
Applied to files:
autogpt_platform/backend/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.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/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.py
📚 Learning: 2026-05-23T05:29:43.085Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13200
File: autogpt_platform/backend/backend/executor/scheduler.py:590-593
Timestamp: 2026-05-23T05:29:43.085Z
Learning: When reviewing Python code that uses Pydantic discriminated/tagged unions (e.g., `Annotated[Union[...], Field(discriminator="kind")]`), recognize that using `isinstance(x, SomeVariantInfo)` to narrow the union is an intentional and correct runtime guard and should also enable static type narrowing in tools like Pyright. Do not recommend replacing such `isinstance`-based narrowing with `cast(...)` when the check already proves the variant at runtime.
Applied to files:
autogpt_platform/backend/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.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/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.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/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.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/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.py
📚 Learning: 2026-05-26T14:24:34.866Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 13217
File: autogpt_platform/backend/backend/api/features/search/service.py:137-137
Timestamp: 2026-05-26T14:24:34.866Z
Learning: In the Significant-Gravitas/AutoGPT backend, treat `user_id` (an opaque UUID used only for correlation/tracing) as non-PII. Do not flag direct logging of `user_id` in `logger.warning`/`logger.info` statements as a PII exposure issue, as the established convention is to log `user_id` for tracing while reserving PII for fields like email or display name.
Applied to files:
autogpt_platform/backend/backend/api/features/library/db_test.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/library/db.py
🔇 Additional comments (5)
autogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.py (3)
22-28: LGTM!
37-42: LGTM!
64-115: LGTM!autogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.py (2)
11-29: LGTM!
32-53: LGTM!Also applies to: 56-71, 74-91, 94-106
|
/review |
![autogpt-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/130738209?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Automated Review — PR #13264
PR #13264 — fix(backend): make graph save atomic + surface clear credential errors
Author: Pwuts | Files: 6
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR description clearly explains the problem (half-saved graphs on credential failures producing opaque 500s), the solution (reorder activation before persistence), and acknowledges known gaps (fork_library_agent).
What This PR Does
Previously, when a user saved a graph with revoked or missing OAuth credentials, the graph and library agent rows were written to the database first, then activation failed with an opaque HTTP 500 — leaving orphaned rows and a confused user. This PR reorders the flow so credential validation (on_graph_activate) runs before any database writes. If credentials are bad, the user gets an HTTP 400 with a clear "please reconnect" message and nothing is persisted. It also introduces GraphActivationError as a proper domain exception.
Specialist Findings
🛡️ Security ✅ — Net security improvement. Atomic saves prevent half-written state, and 500→400 conversion removes a class of error-triggered information leakage.
- 🟠 Bare
except Exceptionatgraph_lifecycle_hooks.py:76could swallow infrastructure failures (DB timeouts, Redis errors) and misreport them as credential problems. (Flagged by: security, architect, quality — 3) - 🟡 Raw provider error strings and internal UUIDs exposed in user-facing error messages at
graph_lifecycle_hooks.py:104-114. Low exploitation risk behind auth, but unnecessary. (Flagged by: security, product — 2)
🏗️ Architecture ✅ — Core validate-before-persist pattern is sound and consistently applied across 4 call sites. GraphActivationError hierarchy is clean.
- 🟠
fork_library_agentatdb.py:2071still activates after the DB write and no caller catchesGraphActivationError— an activation failure leaves an orphaned graph row and surfaces as a 500. (Flagged by: architect, security, performance, product — 4) - 🟠 Dropped return value in
set_graph_active_versionatv1.py:1767-1770— ifon_graph_activateclears stale optional credentials, those mutations aren't persisted, contradicting the PR's stated goal. (Flagged by: discussion — CodeRabbit finding, unaddressed) - 🟡 Unconditional activation in
create_new_graph(v1.py:1648) and externalcreate_graph(routes.py:170) — noif graph.is_activeguard, unlikeupdate_graphand library paths. - 🟡 Return value divergence: routes now return the pre-persist model instead of the DB result — may be missing DB-generated fields (timestamps, version numbers) at
v1.py:1656androutes.py:178.
⚡ Performance ✅ — Acceptable trade-off. Activation now blocks the save response (adds latency), but this is correct for atomicity.
- 🟡 Sequential
await get_credentials()per node atgraph_lifecycle_hooks.py:60-115is O(N) serial network calls. Could be parallelised withasyncio.gatherfor graphs with many distinct credentials — pre-existing issue made more visible by moving onto the critical path.
🧪 Testing _on_graph_activate are well-written and cover the key credential scenarios. However, the primary behavioral change (activate-before-persist atomicity) has zero route-level tests verifying that create_graph is never called when activation fails.
- 🔴 No route-level test for
create_new_graphreturning 400 onGraphActivationErroror verifyingcreate_graph/create_library_agentare NOT called (v1.py:1648-1655). (Flagged by: testing — this is the core atomicity guarantee) - 🔴 No route-level test for
update_graphreturning 400 onGraphActivationError(v1.py:1706-1710). (Flagged by: testing) - 🟠 No tests for
set_graph_active_versionerror path (v1.py:1767), external APIcreate_grapherror path (routes.py:171), or library DB functions withis_active=True(db.py:691,db.py:730). (Flagged by: testing — 4 untested error paths) - 🟡 Tests call private
_on_graph_activateonly — publicon_graph_activatewith sub_graph error propagation throughasyncio.gatheris untested.
📖 Quality ✅ — Clean, well-structured code. Naming is excellent (GraphActivationError, creds_field_optional). Comments explain "why" not "what."
- 🟡
GraphActivationError → HTTPException(400)pattern duplicated 4× acrossv1.pyandroutes.py— could be extracted into a helper. - 🔵 Docstrings for
create_graph_in_library(db.py:684) andupdate_graph_in_library(db.py:714) don't mentionRaises: GraphActivationError.
📦 Product ✅ — Meaningful UX improvement: users get "please reconnect this integration" instead of an opaque 500, and no orphaned graph rows on the main save paths.
- 🟠 Error messages reference internal UUIDs (
Credential #cred-abc for 'credentials' on node #node-xyz) instead of human-readable names (node label, provider name) atgraph_lifecycle_hooks.py:90-100. Users can't act on UUIDs. (Flagged by: product, security — 2)
📬 Discussion
- 🟠 CodeRabbit's finding on dropped
on_graph_activatereturn value atv1.py:1770— potentially valid bug, no author response. - 🔵 Author self-noted rename (
on_graph_activate→before_graph_activate) and comment wording fix — not yet applied. - Manual testing checklist item unchecked.
🔎 QA ✅ — End-to-end validation confirmed the core fix works:
- Happy path: graph + library agent created successfully (HTTP 200).
- Bad credential: HTTP 400 with actionable message, zero rows persisted in
AgentGraphorLibraryAgent. - Auth enforcement: 401 on missing/invalid tokens.
- Build UI loads normally — no regressions observed.
🔴 Blockers
- Missing route-level tests for atomicity guarantee (
v1.py:1648-1655,v1.py:1706-1710) — The PR's core promise is "activation before persistence." There are no tests verifying thatcreate_graph/create_library_agentare never called whenon_graph_activateraisesGraphActivationError. These are the tests that would catch a regression if someone reorders the calls back. Add at least tests forcreate_new_graphandupdate_graphthat mockon_graph_activateto raise, assert HTTP 400, and assert no DB writes. (Flagged by: testing)
🟠 Should Fix
- Dropped return value in
set_graph_active_version(v1.py:1767-1770) —on_graph_activatemay mutateinput_default(clearing stale optional creds), but the return value is discarded. Either capture and re-persist, or add a comment explaining why mutation loss is acceptable here. CodeRabbit flagged this and it's unanswered. (Flagged by: discussion, architect) - Broad
except Exceptionin credential loading (graph_lifecycle_hooks.py:76) — Catches all errors including infrastructure failures (DB pool exhaustion, Redis timeouts, coding bugs likeTypeError). These get silently misreported as "please reconnect." Narrow to credential/OAuth-specific exceptions if a base class exists, or at minimum log unexpected exception types aterrorlevel. (Flagged by: security, architect, quality — 3) fork_library_agentcallers don't catchGraphActivationError(db.py:2071) — While moving activation before the fork is acknowledged as follow-up, callers should at minimum catchGraphActivationErrorand map to 400 today, matching the pattern everywhere else. Otherwise users forking agents with bad creds still get a 500. (Flagged by: architect, security, product, performance — 4)- User-facing error messages use internal UUIDs (
graph_lifecycle_hooks.py:90-100) — Messages likeCredential #cred-abc for 'credentials' on node #node-xyzare meaningless to users. Usenode.block.nameor the node's display label and the credential provider name so users know which node and integration to fix. (Flagged by: product, security — 2)
🟡 Nice to Have
- Parallelise credential checks (
graph_lifecycle_hooks.py:60-115) — Sequentialawait get_credentials()per unique credential ID could be batched withasyncio.gather. Pre-existing issue, not introduced by this PR. (performance) - DRY up
GraphActivationError → HTTPException(400)pattern (v1.py,routes.py) — Repeated 4× across call sites; could be a small helperactivate_or_400(). (quality) - Add
Raises: GraphActivationErrorto docstrings (db.py:684,db.py:714) — Library functions let the exception propagate but don't document it. (quality) - Unify
is_activeguard across call sites (v1.py:1648vsv1.py:1705) — Create paths activate unconditionally while update paths gate onis_active. Add comment or align. (architect)
🔵 Nits
- Rename
on_graph_activate→before_graph_activate(graph_lifecycle_hooks.py:31) — Author self-noted, better reflects pre-persist semantics. - Comment wording (
routes.py:168) — Author self-noted: "Activate" → "Validate" to clarify intent.
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Build UI loads normally after changes ✅ |
Human Review Needed
YES — This PR changes error handling semantics across multiple API endpoints (graph create, update, activate, fork), modifies the persistence order for database writes, and introduces a new exception type that crosses module boundaries. The fork_library_agent gap and dropped return value in set_graph_active_version need human judgment on acceptable trade-offs.
Risk Assessment
Merge risk: LOW | Rollback: EASY — Backend-only, no migrations, no schema changes. Revert restores previous behavior.
CI Status
❌ 2/6 local quality checks passed (lint ✅), 4 failed (typecheck, tests, build). However, all 37 GitHub CI checks are green ✅ — local failures appear to be environment issues, not code regressions.
UI Testing — Variant Results
✅ local: Graph save atomicity and credential error surfacing verified end-to-end: bad credentials now return HTTP 400 with clear messages and zero DB side-effects.
✅ hosted: All credential error paths return HTTP 400 with clear messages; graph save atomicity verified via DB — no orphaned rows on failure.
github-project-automation
Bot
moved this from 🆕 Needs initial review
to 🚧 Needs work
in AutoGPT development kanban
… propagate cleanup mutations Addresses PR review on #13264: - Rename `on_graph_activate`/`_on_graph_activate` to `before_graph_activate`/ `_before_graph_activate` to make it clear the hook is a pre-activation validation step and must run before the graph is persisted. Docstring spells out the contract — no DB writes, no use for post-activation state changes. - `set_graph_active_version` now captures the returned graph from `before_graph_activate` so any input_default cleanups (clearing stale optional credential references) propagate to `update_library_agent_version_and_settings`. - Update "Activate BEFORE persisting" comments to "Validate BEFORE persisting" to match the new name.
…tivate rename Tests still patched the old `on_graph_activate` name on `library_db` and `v1` modules, breaking CI with AttributeError after the rename in 8216e5c. Mirror the rename in the patch targets.
POST /graphs and PUT /graphs/{id} must return 400 (not 500) and persist
nothing when before_graph_activate raises GraphActivationError. The pre-
save activation order is the regression we care about — these tests fail
if anyone reorders create_graph back ahead of activation.
The unit tests on _before_graph_activate cover the credential failure
modes; this layer covers the route-level guarantee that bad credentials
can't leave a half-saved graph behind.
…during graph activation Bare `except Exception` while loading a node's credentials would also swallow infra failures (DB pool exhaustion, Redis timeout, programming errors like TypeError) and misreport them as "please reconnect" — silently hiding real incidents. Heuristically distinguish known credential-side failures (OAuth `invalid_grant`, 401/403 from the provider) from anything else: the former stay at warning (expected, user-actionable), the latter log at error with a stack trace so they surface in alerting. The user-facing GraphActivationError still produces the same reconnect-style message either way. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… + block name in activation errors `Credential #cred-abc for 'credentials' on node #node-xyz could not be loaded` is unactionable — users can't act on UUIDs. Build the user-facing `GraphActivationError` message from the credential's title (or provider name as fallback) and the block name instead. UUIDs still appear in the warning/ error log line above for support lookups. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…the 400 mapping The fork route was missing the GraphActivationError → 400 mapping that every other graph-creation/update route has, so users forking an agent with a revoked credential still got a 500. Wrap it at the route boundary too; making the fork's save itself atomic is a separate follow-up since fork_graph owns its own transaction. Also extract the repeated try/except into a `raise_400_on_activation_error` context manager in `backend.api.utils.graph_activation` and use it across the five call sites (create, update, set-active, external create, fork).
…tion `_before_graph_activate` walked each node serially, awaiting one `get_credentials` call at a time. For a graph with several distinct OAuth credentials that each need a refresh round-trip, that's O(N) serial network latency on the critical path of every save. Collect the credential refs in one pre-pass, batch the lookups through `asyncio.gather(return_exceptions=True)`, then apply the per-ref result (usable / clear-if-optional / raise) in a small extracted helper. Behaviour is identical — only the timing changes.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
autogpt_platform/backend/backend/api/features/v1_test.py (1)
847-947: ⚡ Quick winSnapshot the full 400 payloads in these API tests.
These tests already verify the atomicity behavior well, but they only assert a
"reconnect"substring. Snapshottingresponse.json()would lock the HTTP 400 response shape/message contract too, which is what the backend test guideline asks for.As per coding guidelines,
Use pytest with snapshot testing for API responses.🤖 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/backend/api/features/v1_test.py` around lines 847 - 947, Update the two tests test_create_new_graph_returns_400_and_persists_nothing_on_activation_error and test_update_graph_returns_400_and_persists_nothing_on_activation_error to snapshot the full 400 JSON payload instead of only asserting the "reconnect" substring: add the pytest snapshot fixture to each test signature (e.g. snapshot), replace the assert "reconnect" in response.json()["detail"] with a snapshot assertion on response.json() (e.g. snapshot.assert_match(response.json())), and keep the existing activation and persistence assertions (activate_mock.assert_awaited_once(), create_graph_mock.assert_not_awaited(), update_lib_agent_mock.assert_not_awaited()) unchanged so the atomicity checks remain.
🤖 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.
Nitpick comments:
In `@autogpt_platform/backend/backend/api/features/v1_test.py`:
- Around line 847-947: Update the two tests
test_create_new_graph_returns_400_and_persists_nothing_on_activation_error and
test_update_graph_returns_400_and_persists_nothing_on_activation_error to
snapshot the full 400 JSON payload instead of only asserting the "reconnect"
substring: add the pytest snapshot fixture to each test signature (e.g.
snapshot), replace the assert "reconnect" in response.json()["detail"] with a
snapshot assertion on response.json() (e.g.
snapshot.assert_match(response.json())), and keep the existing activation and
persistence assertions (activate_mock.assert_awaited_once(),
create_graph_mock.assert_not_awaited(),
update_lib_agent_mock.assert_not_awaited()) unchanged so the atomicity checks
remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bf1a41f-3f9a-4657-a727-19c41dbb212a
📒 Files selected for processing (8)
autogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.pyautogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.pyautogpt_platform/backend/test/test_migrate_webhook_presets.py
✅ Files skipped from review due to trivial changes (1)
- autogpt_platform/backend/test/test_migrate_webhook_presets.py
🚧 Files skipped from review as they are similar to previous changes (3)
- autogpt_platform/backend/backend/api/external/v1/routes.py
- autogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks.py
- autogpt_platform/backend/backend/integrations/webhooks/graph_lifecycle_hooks_test.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
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/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
autogpt_platform/backend/**/api/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/api/**/*.py: UseSecurity()instead ofDepends()for authentication dependencies to get proper OpenAPI security specification
Follow SSE (Server-Sent Events) protocol: usedata:lines for frontend-parsed events (must match Zod schema) and: commentlines for heartbeats/status
Files:
autogpt_platform/backend/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
autogpt_platform/backend/backend/api/features/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update routes in '/backend/backend/api/features/' and add/update Pydantic models in the same directory for API development
Files:
autogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
autogpt_platform/backend/**/*_test.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/*_test.py: Use pytest with snapshot testing for API responses
Colocate test files with source files using*_test.pynaming convention
Mock at boundaries — mock where the symbol is used, not where it's defined; after refactoring, update mock targets to match new module paths
UseAsyncMockfromunittest.mockfor async functions in tests
When writing tests, use Test-Driven Development (TDD): write failing tests marked with@pytest.mark.xfailbefore implementation, then remove the marker once the implementation is complete
When creating snapshots in tests, usepoetry run pytest path/to/test.py --snapshot-update; always review snapshot changes withgit diffbefore committing
Files:
autogpt_platform/backend/backend/api/features/v1_test.py
🧠 Learnings (10)
📚 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/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
📚 Learning: 2026-03-31T15:37:38.626Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12623
File: autogpt_platform/backend/backend/copilot/tools/agent_generator/fixer.py:37-47
Timestamp: 2026-03-31T15:37:38.626Z
Learning: When validating/constructing Anthropic API model IDs in Significant-Gravitas/AutoGPT, allow the hyphen-separated Claude Opus 4.6 model ID `claude-opus-4-6` (it corresponds to `LlmModel.CLAUDE_4_6_OPUS` in `autogpt_platform/backend/backend/blocks/llm.py`). Do NOT require the dot-separated form in Anthropic contexts. Only OpenRouter routing variants should use the dot separator (e.g., `anthropic/claude-opus-4.6`); `claude-opus-4-6` should be treated as correct when passed to Anthropic, and flagged only if it’s used in the OpenRouter path where the dot form is expected.
Applied to files:
autogpt_platform/backend/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
📚 Learning: 2026-05-23T05:29:43.085Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13200
File: autogpt_platform/backend/backend/executor/scheduler.py:590-593
Timestamp: 2026-05-23T05:29:43.085Z
Learning: When reviewing Python code that uses Pydantic discriminated/tagged unions (e.g., `Annotated[Union[...], Field(discriminator="kind")]`), recognize that using `isinstance(x, SomeVariantInfo)` to narrow the union is an intentional and correct runtime guard and should also enable static type narrowing in tools like Pyright. Do not recommend replacing such `isinstance`-based narrowing with `cast(...)` when the check already proves the variant at runtime.
Applied to files:
autogpt_platform/backend/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
📚 Learning: 2026-05-26T14:24:34.866Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 13217
File: autogpt_platform/backend/backend/api/features/search/service.py:137-137
Timestamp: 2026-05-26T14:24:34.866Z
Learning: In the Significant-Gravitas/AutoGPT backend, treat `user_id` (an opaque UUID used only for correlation/tracing) as non-PII. Do not flag direct logging of `user_id` in `logger.warning`/`logger.info` statements as a PII exposure issue, as the established convention is to log `user_id` for tracing while reserving PII for fields like email or display name.
Applied to files:
autogpt_platform/backend/backend/api/utils/graph_activation.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
🔇 Additional comments (3)
autogpt_platform/backend/backend/api/features/v1.py (1)
128-130: LGTM!Also applies to: 1648-1649, 1704-1705, 1766-1769
autogpt_platform/backend/backend/api/utils/graph_activation.py (1)
1-21: LGTM!autogpt_platform/backend/backend/api/features/library/routes/agents.py (1)
8-8: LGTM!Also applies to: 217-225
|
🤖 Addressed the autogpt-pr-reviewer findings, one commit per concern: Blocker
Should Fix
Nice to Have (taken)
Plus
Leaving the remaining nice-to-haves (docstring |
- graph_lifecycle_hooks.py: widen the credential-refs tuple's node type from `NodeModel` to `Node | NodeModel`, matching `BaseGraph.nodes` (pyright failed on `refs.append((new_node, …))` for sub-graphs). - v1_test.py: include matching `id` in the update_graph atomicity test payload — without it the route short-circuits on the 'Graph ID does not match ID in URI' 400 before activation runs. - test_migrate_webhook_presets.py: switch the `before_graph_activate` patches in the `set_graph_active_version` tests from `return_value=None` to the pass-through `side_effect=lambda g, user_id: g` used by the other patches, since `set_graph_active_version` now captures the returned graph.
…ception handler instead of try/except + helper Drops the `raise_400_on_activation_error` context manager and the per-route try/except blocks in favour of `app.add_exception_handler(GraphActivationError, handle_internal_http_error(400))` on both FastAPI apps — matching the existing pattern for `FolderValidationError`, `NotFoundError`, etc. Routes call `before_graph_activate(...)` directly; the raised exception is mapped at the app boundary. - rest_api.py: register the handler alongside the other domain-error mappings. - external/fastapi_app.py: same, on the separate external FastAPI instance. - v1_test.py: register on the test app too so the atomicity tests verify the same 400 mapping the real app exposes. - Delete backend/api/utils/graph_activation.py (the context manager). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Reverted 9aa67d2d4's context-manager approach in 55102a80c: per @Pwuts the existing pattern in Re: coderabbit's snapshot nitpick on the atomicity tests — leaving substring assertions for now; the message wording is intentionally informal and likely to drift, snapshotting would just force unrelated snapshot updates each time we tune it. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
autogpt_platform/backend/backend/api/features/v1_test.py (1)
895-896: ⚡ Quick winSnapshot the 400 error payload.
These tests currently only check
status_codeand adetailsubstring, so they'd still pass if the shared 400 handler changed the rest of the response shape. Snapshotting the full body here would lock the API contract the tests are meant to mirror. As per coding guidelines, "Use pytest with snapshot testing for API responses".Also applies to: 948-949
🤖 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/backend/api/features/v1_test.py` around lines 895 - 896, The test currently only asserts response.status_code == 400 and that "reconnect" is in response.json()["detail"]; update the test to snapshot the entire 400 payload so the API contract is locked—use the pytest snapshot fixture to capture response.json() (or response.text) and assert against that snapshot (e.g., snapshot.assert_match(response.json())), keeping the existing status_code and detail substring checks if desired; apply the same change to the other occurrence that currently checks response.status_code and response.json()["detail"].
🤖 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.
Nitpick comments:
In `@autogpt_platform/backend/backend/api/features/v1_test.py`:
- Around line 895-896: The test currently only asserts response.status_code ==
400 and that "reconnect" is in response.json()["detail"]; update the test to
snapshot the entire 400 payload so the API contract is locked—use the pytest
snapshot fixture to capture response.json() (or response.text) and assert
against that snapshot (e.g., snapshot.assert_match(response.json())), keeping
the existing status_code and detail substring checks if desired; apply the same
change to the other occurrence that currently checks response.status_code and
response.json()["detail"].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fed9caf6-9c8a-46cd-a978-48b99092ba16
📒 Files selected for processing (6)
autogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/external/v1/routes.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/rest_api.py
🚧 Files skipped from review as they are similar to previous changes (2)
- autogpt_platform/backend/backend/api/external/v1/routes.py
- autogpt_platform/backend/backend/api/features/v1.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). (9)
- GitHub Check: check API types
- GitHub Check: test (3.11)
- GitHub Check: type-check (3.11)
- GitHub Check: type-check (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: type-check (3.12)
- GitHub Check: end-to-end tests
- GitHub Check: Check PR Status
🧰 Additional context used
📓 Path-based instructions (5)
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/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.py
autogpt_platform/backend/**/api/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/api/**/*.py: UseSecurity()instead ofDepends()for authentication dependencies to get proper OpenAPI security specification
Follow SSE (Server-Sent Events) protocol: usedata:lines for frontend-parsed events (must match Zod schema) and: commentlines for heartbeats/status
Files:
autogpt_platform/backend/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.py
autogpt_platform/backend/backend/api/features/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update routes in '/backend/backend/api/features/' and add/update Pydantic models in the same directory for API development
Files:
autogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.py
autogpt_platform/backend/**/*_test.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/*_test.py: Use pytest with snapshot testing for API responses
Colocate test files with source files using*_test.pynaming convention
Mock at boundaries — mock where the symbol is used, not where it's defined; after refactoring, update mock targets to match new module paths
UseAsyncMockfromunittest.mockfor async functions in tests
When writing tests, use Test-Driven Development (TDD): write failing tests marked with@pytest.mark.xfailbefore implementation, then remove the marker once the implementation is complete
When creating snapshots in tests, usepoetry run pytest path/to/test.py --snapshot-update; always review snapshot changes withgit diffbefore committing
Files:
autogpt_platform/backend/backend/api/features/v1_test.py
🧠 Learnings (10)
📚 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/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.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/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.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/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.py
📚 Learning: 2026-03-31T15:37:38.626Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12623
File: autogpt_platform/backend/backend/copilot/tools/agent_generator/fixer.py:37-47
Timestamp: 2026-03-31T15:37:38.626Z
Learning: When validating/constructing Anthropic API model IDs in Significant-Gravitas/AutoGPT, allow the hyphen-separated Claude Opus 4.6 model ID `claude-opus-4-6` (it corresponds to `LlmModel.CLAUDE_4_6_OPUS` in `autogpt_platform/backend/backend/blocks/llm.py`). Do NOT require the dot-separated form in Anthropic contexts. Only OpenRouter routing variants should use the dot separator (e.g., `anthropic/claude-opus-4.6`); `claude-opus-4-6` should be treated as correct when passed to Anthropic, and flagged only if it’s used in the OpenRouter path where the dot form is expected.
Applied to files:
autogpt_platform/backend/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.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/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.py
📚 Learning: 2026-05-23T05:29:43.085Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13200
File: autogpt_platform/backend/backend/executor/scheduler.py:590-593
Timestamp: 2026-05-23T05:29:43.085Z
Learning: When reviewing Python code that uses Pydantic discriminated/tagged unions (e.g., `Annotated[Union[...], Field(discriminator="kind")]`), recognize that using `isinstance(x, SomeVariantInfo)` to narrow the union is an intentional and correct runtime guard and should also enable static type narrowing in tools like Pyright. Do not recommend replacing such `isinstance`-based narrowing with `cast(...)` when the check already proves the variant at runtime.
Applied to files:
autogpt_platform/backend/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.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/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.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/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.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/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.py
📚 Learning: 2026-05-26T14:24:34.866Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 13217
File: autogpt_platform/backend/backend/api/features/search/service.py:137-137
Timestamp: 2026-05-26T14:24:34.866Z
Learning: In the Significant-Gravitas/AutoGPT backend, treat `user_id` (an opaque UUID used only for correlation/tracing) as non-PII. Do not flag direct logging of `user_id` in `logger.warning`/`logger.info` statements as a PII exposure issue, as the established convention is to log `user_id` for tracing while reserving PII for fields like email or display name.
Applied to files:
autogpt_platform/backend/backend/api/rest_api.pyautogpt_platform/backend/backend/api/external/fastapi_app.pyautogpt_platform/backend/backend/api/features/library/routes/agents.pyautogpt_platform/backend/backend/api/features/v1_test.py
🔇 Additional comments (4)
autogpt_platform/backend/backend/api/external/fastapi_app.py (1)
7-7: LGTM!Also applies to: 31-37
autogpt_platform/backend/backend/api/rest_api.py (1)
61-61: LGTM!Also applies to: 309-309
autogpt_platform/backend/backend/api/features/v1_test.py (1)
19-31: LGTM!autogpt_platform/backend/backend/api/features/library/routes/agents.py (1)
217-224: LGTM!
`poetry run format` reorders `handle_internal_http_error` import to its alphabetical slot in v1_test.py and reformats the long handler signature in external/fastapi_app.py to a single line. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`poetry run format` joins the two adjacent string literals in graph_lifecycle_hooks.py's credential_ref onto one line. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 participant
Why / What / How
Why: Saving a graph with a node whose OAuth refresh token had been revoked returned an opaque HTTP 500 from
POST /api/graphswith('invalid_grant: Bad Request', …)in logs. The ticket (SECRT-2370) flagged this as cascading from agent image generation, but image gen is already fire-and-forget — the actual 500 is OAuth refresh failing insideon_graph_activate, which today runs aftercreate_graph+create_library_agent. So by the time activation blew up, the graph and library agent were already committed, the user got no clean error, and anyinput_defaultcleanups activation does (clearing stale optional credentials) were silently dropped.What: Two related fixes in
backend.integrations.webhooks.graph_lifecycle_hooksand every call site that persists a graph.How:
GraphActivationError._on_graph_activatenow wrapsget_credentials(...)in try/except so an OAuth refresh failure is treated the same as a missing credential — optional fields get cleared, required fields raise a clear "credential X for node Y could not be loaded — please reconnect this integration" message. Routes map that to HTTP 400 instead of 500.create_new_graph,update_graph, external APIcreate_graph,create_graph_in_library, andupdate_graph_in_library. A credential failure now means nothing is written; the response is a 400 with the friendly message.set_graph_active_versionalready had no save to roll back, but also mapsGraphActivationError→ 400.input_defaultcleanups (clearing stale optional credentials) are now actually persisted, since the save happens after activation mutates them.Out of scope: rotating the Ideogram dev key that produced the image-gen warning in the original trace — that's a config task, handled separately.
fork_library_agentstill activates afterfork_graph's own DB write; splitting that would mean reaching into the data layer, left as a follow-up.Changes 🏗️
backend/integrations/webhooks/graph_lifecycle_hooks.py: addGraphActivationError; catch credential-load exceptions in_on_graph_activateand treat as missing/unusable; clearer error messages distinguishing "no longer exists" vs "could not be loaded — please reconnect".backend/api/features/v1.py:create_new_graph,update_graph,set_graph_active_versionnow callon_graph_activatebefore persisting (where applicable) and mapGraphActivationErrorto HTTP 400.backend/api/external/v1/routes.py: external APIcreate_graphmatches the new pre-validate-then-save order and 400 mapping.backend/api/features/library/db.py:create_graph_in_libraryandupdate_graph_in_libraryactivate before persisting; they raiseGraphActivationErrorfor callers (copilot agent generator) to surface.graph_lifecycle_hooks_test.pycovering OAuth-refresh-as-clear-error, optional-cred-cleared-on-refresh-failure, missing-required-cred message, happy path. Existingtest_update_graph_in_library_allows_archived_library_agentupdated to setgraph_model.is_active = Falsesince activation is now wired into that path.Checklist 📋
For code changes:
pytest backend/integrations/webhooks/graph_lifecycle_hooks_test.py— 4 passedpytest backend/api/features/library/db_test.py— 20 passed (including the updated test)pytest backend/api/features/v1_test.py— 32 passedpytest backend/api/external/v1/+backend/integrations/webhooks/— 13 passedruff+pyrightclean on touched filesPOST /api/graphsreturns 400 with the friendly message and no graph row in DBFor configuration changes:
.env.defaultis updated or already compatible with my changesdocker-compose.ymlis updated or already compatible with my changes