fix(backend): bound CountdownTimerBlock inputs and surface field-bound errors inline - #13237
Conversation
Add upper/lower bounds to CountdownTimerBlock so the configured duration stays within a sensible range (max 7 days, repeat 1-1000) and reject negative durations. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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:
WalkthroughCountdownTimerBlock now enforces ChangesCountdownTimerBlock Duration Constraints
Executor input validation mapping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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. 🟢 Low Risk — File Overlap OnlyThese PRs touch the same files but different sections (click to expand)
Summary: 0 conflict(s), 0 medium risk, 3 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 #13237 +/- ##
==========================================
- Coverage 72.55% 72.51% -0.04%
==========================================
Files 2349 2340 -9
Lines 174823 174759 -64
Branches 17726 17646 -80
==========================================
- Hits 126836 126727 -109
- Misses 44220 44276 +56
+ Partials 3767 3756 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…9-fixblocks-cap-countdowntimerblock-duration-closes-5-ghsas
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
autogpt_platform/backend/backend/blocks/time_blocks.py (1)
482-497:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCap the full repeated countdown, not just one iteration.
total_secondsis validated before the loop, butrepeatis applied afterward. A payload likedays=7, repeat=1000still passes and can keep the worker busy for ~19 years total, which misses the stated goal of preventing extremely long timers from tying up execution. Please validate the aggregate runtime (total_seconds * input_data.repeat) againstMAX_TOTAL_SECONDSand add a regression for a multi-repeat over-cap case.Suggested fix
total_seconds = seconds + minutes * 60 + hours * 3600 + days * 86400 if total_seconds < 0: raise ValueError( f"Countdown duration must be non-negative, got {total_seconds}s" ) - if total_seconds > self.MAX_TOTAL_SECONDS: + total_runtime_seconds = total_seconds * input_data.repeat + if total_runtime_seconds > self.MAX_TOTAL_SECONDS: raise ValueError( - f"Countdown duration {total_seconds}s exceeds max " + f"Countdown duration {total_runtime_seconds}s exceeds max " f"({self.MAX_TOTAL_SECONDS}s = 7 days)" )🤖 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/blocks/time_blocks.py` around lines 482 - 497, The code currently validates total_seconds but not the aggregate runtime when repeats are applied; update the logic in the countdown generator (the block using total_seconds, input_data.repeat, and self.MAX_TOTAL_SECONDS) to compute aggregate_seconds = total_seconds * input_data.repeat and raise ValueError if aggregate_seconds > self.MAX_TOTAL_SECONDS (or if aggregate_seconds < 0), ensuring the per-iteration sleep loop remains unchanged; also add a regression test that constructs a payload with valid per-iteration duration but excessive repeat (e.g., days=7, repeat>1) to assert the new validation rejects it.
🤖 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.
Outside diff comments:
In `@autogpt_platform/backend/backend/blocks/time_blocks.py`:
- Around line 482-497: The code currently validates total_seconds but not the
aggregate runtime when repeats are applied; update the logic in the countdown
generator (the block using total_seconds, input_data.repeat, and
self.MAX_TOTAL_SECONDS) to compute aggregate_seconds = total_seconds *
input_data.repeat and raise ValueError if aggregate_seconds >
self.MAX_TOTAL_SECONDS (or if aggregate_seconds < 0), ensuring the per-iteration
sleep loop remains unchanged; also add a regression test that constructs a
payload with valid per-iteration duration but excessive repeat (e.g., days=7,
repeat>1) to assert the new validation rejects it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fc49a2f-b02b-424f-a317-90823ffb8a7c
📒 Files selected for processing (2)
autogpt_platform/backend/backend/blocks/time_blocks.pyautogpt_platform/backend/backend/blocks/time_blocks_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). (1)
- GitHub Check: Seer Code Review
🧰 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/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.py
autogpt_platform/backend/backend/blocks/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/backend/blocks/**/*.py: Inherit from 'Block' base class with input/output schemas when adding new blocks in backend
Implement 'run' method with proper error handling in backend blocks
Generate block UUID using 'uuid.uuid4()' when creating new blocks in backend
Write tests alongside block implementation when adding new blocks in backend
autogpt_platform/backend/backend/blocks/**/*.py: For blocks handling files, usestore_media_file()withreturn_format="for_local_processing"when processing with local tools (ffmpeg, MoviePy, PIL)
For blocks handling files, usestore_media_file()withreturn_format="for_external_api"when sending content to external APIs (Replicate, OpenAI)
For blocks returning files, usestore_media_file()withreturn_format="for_block_output"to enable auto-adaptation to execution context (workspace:// in CoPilot, data URI in graphs)
When creating new blocks, inherit fromBlockbase class, define input/output schemas usingBlockSchema, implement asyncrunmethod, and generate unique block ID usinguuid.uuid4()
Files:
autogpt_platform/backend/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.py
🧠 Learnings (16)
📚 Learning: 2026-02-05T04:11:00.596Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 11796
File: autogpt_platform/backend/backend/blocks/video/concat.py:3-4
Timestamp: 2026-02-05T04:11:00.596Z
Learning: In autogpt_platform/backend/backend/blocks/**/*.py, when creating a new block, generate a UUID once with uuid.uuid4() and hard-code the resulting string as the block's id parameter. Do not call uuid.uuid4() at runtime; IDs must be constant across all imports and runs to ensure stability.
Applied to files:
autogpt_platform/backend/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.py
📚 Learning: 2026-03-16T16:32:21.686Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 12417
File: autogpt_platform/backend/backend/blocks/agent_mail/pods.py:62-74
Timestamp: 2026-03-16T16:32:21.686Z
Learning: In autogpt_platform/backend/backend/blocks/, the Block base class execute() already wraps run() in a try/except to convert uncaught exceptions into BlockExecutionError/BlockUnknownError. Do not add per-block try/except in individual block run() methods, as this is not the established pattern (e.g., Gmail, Slack, Todoist blocks omit it). Only use explicit try/except within blocks that need to distinguish between success and error yield paths inside a generator (e.g., attachment blocks). This guidance applies to all Python files under autogpt_platform/backend/backend/blocks/ and similar block implementations; avoid duplicating error handling in run() unless a block requires generator-based branching.
Applied to files:
autogpt_platform/backend/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.py
📚 Learning: 2026-04-23T12:55:26.122Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12893
File: autogpt_platform/backend/backend/blocks/ayrshare/post_to_tiktok.py:24-24
Timestamp: 2026-04-23T12:55:26.122Z
Learning: Cost billing via the cost(*costs) decorator is applied at input-evaluation time (before a block’s run() executes). Therefore, mutating input_data inside run() will not change billing. When a block’s billing depends on a field plus URL/sniff-derived signals, treat the explicitly declared billing field (e.g., is_video) as the only billing source—set it correctly before run() (or in the code path that occurs before the decorator evaluates input_data). This should be checked for all blocks under autogpt_platform/backend/backend/blocks/ so billing signals are not mistakenly assumed to update during run().
Applied to files:
autogpt_platform/backend/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.py
📚 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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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: In autogpt_platform/backend/backend/blocks/ (and related blocks under autogpt_platform/backend/backend/blocks/), do not add try/except blocks around a block's run() method for standard error propagation. The block executor framework (backend/executor/manager.py) catches uncaught exceptions from run() and emits them on the 'error' output. Only add explicit try/except blocks when you need to control partial outputs in failure cases (e.g., certain outputs must not be yielded on error, as in attachment blocks). This is the standard pattern across the codebase; apply it broadly to blocks' run() implementations.
Applied to files:
autogpt_platform/backend/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.py
📚 Learning: 2026-03-16T16:30:23.196Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 12417
File: autogpt_platform/backend/backend/blocks/agent_mail/pods.py:62-74
Timestamp: 2026-03-16T16:30:23.196Z
Learning: In any Python file under autogpt_platform/backend/backend/blocks, do not add a try/except around run() solely for standard error handling. The block framework’s _execute() in _base.py already catches unhandled exceptions and re-raises as BlockExecutionError or BlockUnknownError. If you yield ("error", message), _execute() raises BlockExecutionError immediately, so the error port will not propagate downstream. Reserve explicit try/except for scenarios where you must control partial output (e.g., attachment blocks that must skip yielding content_base64 on failure).
Applied to files:
autogpt_platform/backend/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/backend/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.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/blocks/time_blocks_test.pyautogpt_platform/backend/backend/blocks/time_blocks.py
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
documentation
- Relax repeat lower bound to ge=0 to preserve backward compatibility with existing workflows that may have repeat=0 (no-op loop). - Override execution_timeout_seconds so the configured 7-day cap is actually reachable; the inherited 30-minute default would have capped any timer above that. - Validate the cumulative duration (per-iteration delay × repeat) instead of just the per-iteration delay, so the 7-day cap can't be bypassed by e.g. hours=1, repeat=1000. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai 🤖 Addressed in ab357fcdd: the cap now validates the cumulative |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Switch the repeat bound back to ge=1 so repeat=0 errors loudly instead of silently running zero iterations and emitting nothing. Also enforce the [1, 1000] range inside run() as defense-in-depth in case a caller constructs the input via model_construct() and bypasses Pydantic field validation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
917ba01 to
d519d37
Compare
kcze
changed the title
|
/review |
|
Queued a review for PR #13237 at d519d37. |
The graph-validation path (data/graph.py:_validate_graph_get_errors) already returns a per-field node_errors map that both builders display inline on the offending block. It covered structural issues (required fields, dependencies, etc.) but not field-level JSON-schema constraints like ge/le/pattern/enum. Add BlockSchemaInput.get_field_errors() (per-top-level-field jsonschema errors keyed by message) and call it inside the run-time branch of _validate_graph_get_errors. Fields whose value comes from an upstream link are skipped because the runtime value isn't known at validation time. Result: a value like CountdownTimerBlock.repeat=1200 (or VideoClipBlock.start_time=-1) now lights up on the block field with a clear message instead of being raised as a bare ValueError at execute time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d519d37 to
8126679
Compare
![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 #13237
PR #13237 — fix(backend): bound CountdownTimerBlock inputs and surface field-bound errors inline
Author: kcze | Files: 6
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — PR clearly explains the motivation (unbounded timer inputs, no inline errors), the solution (Pydantic constraints + generic get_field_errors mechanism), and includes documentation updates.
What This PR Does
The CountdownTimerBlock previously had no bounds on its duration or repeat inputs — a user could set a 365-day timer or repeat=0 (which silently produced nothing). This PR adds Pydantic-level constraints (repeat 1–1000, cumulative duration capped at 7 days), runtime defense-in-depth checks in run(), and a new generic get_field_errors() method on BlockSchemaInput that surfaces constraint violations as inline per-field errors in the graph editor UI. The inline error mechanism benefits all blocks, not just CountdownTimerBlock.
Specialist Findings
🛡️ Security
- 🟠 Bare
int()onUnion[int, str]duration fields (time_blocks.py:482-485) raises unhandledValueErroron non-numeric strings like"abc", producing a raw traceback instead of a structured field error. (Flagged by: security, architect — 2) - 🟡
execution_timeout_secondsset to ~7 days (time_blocks.py:479) could allow executor pool exhaustion if multiple users configure max-duration timers. This is an existing architectural concern, not a regression. (Flagged by: security, architect, performance — 3) - 🟡 Individual duration fields lack
ge=0Pydantic constraints (time_blocks.py:433-444), so negative values aren't caught inline — only at runtime. (Flagged by: security, product — 2)
🏗️ Architecture ✅ — Clean design. get_field_errors is placed on BlockSchemaInput (the right abstraction level), integrates with existing _validate_graph_get_errors flow following the same pattern as get_missing_input and get_mismatch_error. The jsonschema dependency is already in use elsewhere. Minimal tech debt added; significant UX debt resolved.
⚡ Performance ✅ — Adds O(L + F) work per node during graph validation (L = links, F = fields). Negligible for typical graph sizes. cls.jsonschema() is cached. Validator instantiation per call is redundant but non-blocking for current usage.
- 🟡
validator_cls(schema)creates a new jsonschema validator perget_field_errorscall (_base.py:291). Schema is deterministic per class — could be cached. (Flagged by: architect, performance — 2)
🧪 Testing
- 🔴 Graph-level integration of
get_field_errorsin_validate_graph_get_errors(graph.py:849) has zero test coverage. This is the feature's primary delivery path — inline errors in the graph editor. (Flagged by: testing, discussion — 2) - 🔴 Linked-field exclusion logic (
graph.py:840-848) is untested. If this regresses, users see spurious errors on fields whose values come from upstream links. (Flagged by: testing — 1) - 🟠 No happy-path test with
repeat > 1(time_blocks_test.py). The refactored iteration loop isn't verified to produce the correct number of outputs. (Flagged by: testing — 1) - 🟡 Error-path tests don't mock
asyncio.sleep(time_blocks_test.py:17). They work becauseValueErroris raised before reaching sleep, but if validation order changes, tests could hang. (Flagged by: testing — 1)
📖 Quality ✅ — Clean, well-named code. Good docstrings. Two minor issues:
- 🟠 Comment at
graph.py:834says "ge/le/pattern/enum/..." but implementation explicitly excludes pattern and enum. Misleading. (Flagged by: quality — 1) - 🔵
MIN_REPEAT/MAX_REPEATconstants duplicated between Pydantic field constraints and class attributes (time_blocks.py:448vs475-476). Could drift.
📦 Product ✅ — Delivers stated goals. Inline field errors work correctly per QA validation. Bounds are sensible.
- 🟡 Raw jsonschema messages shown to users (
_base.py:297) — e.g., "1200 is greater than the maximum of 1000" rather than "Repeat must be between 1 and 1000". Acceptable but not polished. - 🟡 Duration fields don't get inline validation since their constraint is cumulative (in
run()), creating inconsistency whererepeatshows inline errors butdays=365does not.
📬 Discussion ab357fc. One low-severity Sentry finding unacknowledged by author. Zero human reviews submitted. Branch is behind base and needs rebase.
🔎 QA ✅ — All 7 validation scenarios pass. Schema reflects minimum: 1, maximum: 1000 on repeat. Out-of-bounds values (repeat=0, repeat=1200) produce inline node_errors. Valid graphs execute normally. All 12 tests pass. No regressions.
🔴 Blockers
-
Missing integration test for
get_field_errorsin graph validation (graph.py:849) — The core feature of this PR (surfacing inline field errors during graph validation) has no test coverage. A test should construct a graph with a CountdownTimerBlock node whoseinput_defaulthasrepeat=0, call_validate_graph_get_errors, and assert the field error appears innode_errors. (Flagged by: testing, discussion — 2) -
Missing test for linked-field exclusion (
graph.py:840-848) — The logic that skips validation on linked fields is correctness-critical (wrong behavior shows spurious errors) and has no test. Add a test where a node'srepeatfield is linked from upstream with an out-of-range placeholder ininput_default, and verify no field error is produced. (Flagged by: testing — 1)
🟠 Should Fix
-
Wrap
int()casts in try/except for non-numeric string inputs (time_blocks.py:482-485) — Duration fields areUnion[int, str], soint("abc")raises an unhandledValueErrorwith an unhelpful raw message. Add a try/except producing a user-friendly error like"seconds must be a valid integer". Also add a test for this case. (Flagged by: security, architect — 2) -
Fix inaccurate comment (
graph.py:834) — Comment says "ge/le/pattern/enum/..." but the code only checksminimum/maximum/minLength/maxLength/minItems/maxItems. Update to match reality. (Flagged by: quality — 1) -
Add
repeat > 1happy-path test (time_blocks_test.py) — The refactored loop should be verified with e.g.seconds=1, repeat=3(mocking sleep) asserting 3 output messages are yielded. (Flagged by: testing — 1)
🟡 Nice to Have
- Cache jsonschema validator instance (
_base.py:291) —validator_cls(schema)could be cached per class since the schema is deterministic. Minor perf win for large graphs. (security, architect, performance) - Add
ge=0to individual duration fields (time_blocks.py:433-444) — Would catch negative values inline instead of at runtime. (security, product) - Human-readable error messages (
_base.py:297,time_blocks.py:504) — Map jsonschema messages to friendlier text; format runtime errors with human-readable durations instead of raw seconds. (product) - Document executor capacity impact of 7-day timeout (
time_blocks.py:479) — A code comment noting the capacity implications and any rate-limiting expectations. (security, architect, performance)
🔵 Nits
- Duplicated constants (
time_blocks.py:448vs475) —ge=1, le=1000andMIN_REPEAT=1, MAX_REPEAT=1000could drift. Reference constants in the Pydantic field or add a sync comment. setdefaultdrops subsequent errors (_base.py:297) — Intentional UX choice but undocumented. A one-line comment would clarify.- Constants defined after
__init__(time_blocks.py:474) — Per codebase convention, constants should appear before the methods that use them.
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Authenticated build page loads correctly ✅ |
Human Review Needed
YES — This PR modifies core validation logic in _base.py (shared by all blocks) and graph.py (graph validation pipeline). The get_field_errors mechanism is a platform-wide addition. Zero human reviews have been submitted so far. Needs at least one human approval before merge.
Risk Assessment
Merge risk: LOW-MEDIUM | Rollback: EASY
The change is additive (new validation, not modifying existing behavior) and well-isolated. The CountdownTimerBlock bounds only affect that block. The get_field_errors mechanism is generic but only activates when blocks have schema constraints — existing blocks without ge/le are unaffected. Rollback is a simple revert.
CI Status
❌ 3/6 local checks failed — frontend typecheck, backend tests (environment issue), and frontend tests had failures. Frontend issues appear pre-existing (not caused by this PR's backend-only changes). Backend lint passes. Frontend lint passes.
UI Testing — Variant Results
✅ local: All CountdownTimerBlock bounds and inline field-error validation work correctly - schema enforces min/max, graph validation surfaces per-field errors, and all 12 tests pass.
✅ hosted: Backend validation logic is correct with 11/11 tests passing, schema bounds properly exposed via API, and defense-in-depth checks in place; minor concern that field errors only surface at execution time, not at save time.
github-project-automation
Bot
moved this from 🆕 Needs initial review
to 🚧 Needs work
in AutoGPT development kanban
…lock PR
- Wrap int() coercion of `seconds`/`minutes`/`hours`/`days` in a helper
that raises a clear ValueError on non-numeric strings instead of a
raw Python traceback.
- Move CountdownTimerBlock's class-level constants
(`MAX_TOTAL_SECONDS`, `MIN_REPEAT`, `MAX_REPEAT`,
`execution_timeout_seconds`) above `__init__` to match the
top-down-ordering convention.
- Fix the misleading comment in `_validate_graph_get_errors` so it
accurately lists the JSON-schema keywords actually surfaced
(minimum/maximum/length bounds), not pattern/enum which are skipped.
- Add tests:
- `repeat > 1` happy path (3 outputs, 3 sleeps).
- non-numeric string duration rejected with the new error.
- graph-level integration: `_validate_graph_get_errors` surfaces a
bound violation inline as `node_errors[node][field]`.
- linked-field exclusion: a graph where `repeat` is fed by an upstream
link doesn't produce a spurious field error even when the saved
placeholder is out of range.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🤖 @autogpt-pr-reviewer addressed in 30ae327eb8: 🔴 Blockers
🟠 Should Fix
🔵 Nits
Intentionally deferred
|
github-project-automation
Bot
moved this from 🚧 Needs work
to 👍🏼 Mergeable
in AutoGPT development kanban
github-project-automation
Bot
moved this from 👍🏼 Mergeable
to ✅ Done
in AutoGPT development kanban
2 participants
Why
CountdownTimerBlockaccepted unboundedseconds/minutes/hours/days/repeatvalues. A misconfigured graph could schedule a sleep far longer than any realistic workflow needs (e.g. "1 year"), tying up an execution for an unreasonable amount of time. There were also no guards against negative durations.Separately, field-bound violations (typing an out-of-range value into a block input) had no inline UX. They were either silent (e.g.
repeat=0running zero iterations and emitting nothing) or surfaced as a generic uncaught backend error, rather than being highlighted on the offending field the way other block input errors (required fields, etc.) already are.What
CountdownTimerBlockinputs:repeatconstrained to1–1000.per-iteration × repeat) capped at 7 days.execution_timeout_secondson the block so the 7-day cap is actually reachable (the inherited 30-minute default would have cut it off).How
ge=1, le=1000torepeat. After summingtotal_seconds,run()raisesValueErrorif the cumulative duration is negative or exceedsMAX_TOTAL_SECONDS(7 days). The duration fields areUnion[int, str], so the cumulative check is enforced inrun()rather than at the Pydantic level. Also re-enforces the[1, 1000]repeat range insiderun()as defense-in-depth.execution_timeout_seconds = MAX_TOTAL_SECONDS + 60onCountdownTimerBlock.BlockSchemaInput.get_field_errors(data)which returns per-top-level-field errors for the JSON-schema keywords users can actually bypass by typing/pasting:minimum,maximum,exclusiveMinimum,exclusiveMaximum,minLength,maxLength,minItems,maxItems. Skipsenum/const/pattern/multipleOf/typesince those are already enforced by widget rendering (dropdowns / custom rules).if for_run:branch of_validate_graph_get_errors, so violations land in the samenode_errors[node_id][field_name]map both builders consume to highlight the offending field. Linked fields are skipped — runtime value isn't known at validation time.time_blocks_test.pycovering per-iteration over-cap, cumulative over-cap, negative duration, repeat-zero/over-max (Pydantic), defense-in-depth checks, duration at cap, execution-timeout sanity, andget_field_errorsfor cap-violation and clean cases.Checklist 📋
poetry run pytest backend/blocks/time_blocks_test.pypoetry run pytest 'backend/blocks/test/test_block.py::test_available_blocks[CountdownTimerBlock]'poetry run format && poetry run lint