Repository navigation
feat: add automation descriptions - #527
Rohithmatham12 wants to merge 5 commits into
Conversation
⏳ Review waiting on checksThe current head
No action is needed. The scheduled scan retries once every check on the head reports a conclusion. This is an automated check - no AI was used to generate this comment. |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
In scope. AGENTS.md gives this repository ownership of automation definitions, the API/schema contract, and Alembic migrations, and OpenHands/OpenHands#16925 (open, ready-for-dev) names OpenHands/automation as step 1 of this feature. Proceeding with the technical review.
Assessment
The core change is correct and consistent with repository guidance:
- Migration 029 is a single Alembic head on 028 (verified: no competing head), uses the cross-database
Texttype, and matches the pattern of 020/021. AutomationResponse.descriptiondefaults toNone, so rows written before the migration serialize without error.- The git-sync serializer omits a null description, matching the existing
tarball_executablesprecedent, so enabling the field does not rewrite every already-syncedautomation.yaml. descriptionis threaded through the raw, prompt-preset, and plugin-preset create paths, and PATCH picks it up viamodel_dump(exclude_unset=True).
Focused tests pass locally on the head: tests/test_schemas.py tests/test_git_sync_serializer.py tests/test_preset_router.py (179 passed, 45 skipped), tests/test_db.py -k Sqlite (24 passed), and tests/test_git_sync.py tests/test_git_sync_state.py tests/test_git_sync_client.py (86 passed). The testcontainers-backed tests/test_capabilities_router.py suite cannot run here (no Docker), consistent with the PR description. Note that the head's ci, Run tests, and Docker workflow runs currently report action_required, so those paths have not actually been exercised yet.
Material finding
description is added to the create/update/response schemas but not to the draft schemas in openhands/automation/draft_schemas.py. _BaseDraftBody (used by both POST /v1/validate and the /v1/drafts CRUD) still sets extra="forbid" and has no description field, while its own docstring says the draft shape mirrors schemas.py. The validate endpoint is documented as validating "the request body that would be sent to that endpoint", so a preflight body that carries description now comes back valid: false with an extra_forbidden error on description — even though the corresponding create endpoint accepts it. This is a demonstrated inconsistency on the head: CreatePromptAutomationRequest.model_validate({...,"description":"d"}) succeeds, but normalize_draft_body("/v1/preset/prompt", {...,"description":"d"}) raises extra_forbidden.
Why it matters to the merge decision: the consumer this PR unblocks seeds description from entry.description into buildCreatePayload, the same function buildPreflightBody calls, and manifest-setup-dialog.tsx blocks Continue on any preflight error. So the #16925 criterion "an automation launched from a template arrives with the catalog description already set" cannot be satisfied against this service — the automationDescription capability advertises support that a supported code path then rejects. Add description: str | None = Field(default=None, max_length=2000) to _BaseDraftBody so preflight and the drafts CRUD accept what the create endpoints accept. (Note this is the same class of gap as the pre-existing absence of agent_profile_id from RawAutomationDraftBody; that one is out of scope here, but it is a reason to keep the two schema families in step.)
Non-blocking note
A PATCH carrying description: null does not clear the field: model_dump(exclude_unset=True) includes the key with value None, but SQLAlchemy does not flag an unchanged None as dirty, so no UPDATE is issued. Git-sync import, by contrast, can clear it. This only matters once a consumer expects "clear the description" to persist through PATCH; it is not required by #16925 (which uses an empty value on create).
🔄 CHANGES REQUESTED
| # Features every deployment has: they come from the SDK code the service | ||
| # packages into a run, not from configuration. | ||
| _STATIC_FEATURES = ( | ||
| "automationDescription", |
There was a problem hiding this comment.
The automationDescription capability advertises support for a field the service only accepts on the create/update/response schemas. The draft schemas in openhands/automation/draft_schemas.py (_BaseDraftBody, shared by POST /v1/validate and the /v1/drafts CRUD) still use extra="forbid" without a description field, even though that schema family is documented as mirroring schemas.py. A preflight body built from the same buildCreatePayload that now carries description therefore comes back valid: false with an extra_forbidden error on description, and manifest-setup-dialog.tsx blocks Continue on any preflight error — so a template-launched automation cannot carry the catalog description this feature is meant to deliver. Add description: str | None = Field(default=None, max_length=2000) to _BaseDraftBody so preflight and the drafts CRUD accept what the create endpoints accept.
|
Addressed the review finding in e2fba3b: |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
In scope. AGENTS.md gives this repository ownership of the automation definitions, the API/schema contract, and Alembic migrations, and OpenHands/OpenHands#16925 (open, ready-for-dev) names OpenHands/automation as step 1 of this feature. Proceeding with the technical review.
Assessment
The feature itself is well built, and the earlier draft-schema gap is fixed: _BaseDraftBody now carries the same optional 2,000-character description, so /v1/validate and the /v1/drafts CRUD accept what the create endpoints accept (verified: all three draft endpoints now normalize a body containing description, and an over-length value is rejected). The rest of the change is consistent with repository guidance — Text/nullable column, AutomationResponse.description defaulting to None for pre-migration rows, git-sync serializing only a non-null value so existing automation.yaml files are not rewritten, and description threaded through the raw, prompt-preset, and plugin-preset create paths.
Focused tests pass on this head in isolation: tests/test_schemas.py tests/test_git_sync_serializer.py tests/test_preset_router.py tests/test_db.py tests/test_migration_history.py tests/test_git_sync*.py → 295 passed, 45 skipped. (The testcontainers-backed suites cannot run here; no Docker. The head's ci, Run tests, and Docker runs still report action_required.)
Material finding
The migration is numbered 029, which now collides with main. Detail and fix are on the inline comment below. This is the reason for the verdict: on the actual PR merge ref the schema does not build, so the branch cannot merge as-is.
Non-blocking note
The draft materialization path does not carry the new field. _materialize_raw_draft / _materialize_prompt_draft / _materialize_plugin_draft in openhands/automation/draft_router.py build the kwargs for Automation(**values) and none of them include description (verified: the returned dict has no description key). The new test_prompt_draft_accepts_description only asserts CRUD persistence of the draft body, so a draft created with a description and then dispatched materializes an automation with description = NULL, unlike the create endpoints which now pass it through. Canvas's create path POSTs straight to /v1/preset/*, so this does not block the feature, but for contract coherence the materializers should pass body.description through as well.
🔄 CHANGES REQUESTED
| from sqlalchemy import Column, Text | ||
|
|
||
|
|
||
| revision: str = "029" |
There was a problem hiding this comment.
This migration is numbered 029 (revision="029", down_revision="028"), but main has moved since the branch was cut. origin/main now contains 029_add_git_sync_failure_backoff.py — also revision="029", down_revision="028" — plus 030_kv_per_key_rows.py. On the actual PR merge ref (872e3d4, merge of this head into main), both 029 files are present: Alembic emits UserWarning: Revision 029 is present more than once and aborts alembic upgrade head with Multiple head revisions are present for given argument 'head'. tests/test_db.py::TestSqliteMigrations::test_migrations_run_on_sqlite fails on that merge ref for the same reason, and the duplicate head violates the AGENTS.md rule that a schema change ships "one coherent migration with one Alembic head and a complete upgrade path from the released schema". Please rebase onto current main and renumber this to 031_add_automation_description.py with revision = "031" and down_revision = "030".
e2fba3b to
d73d648
Compare
|
Addressed the latest review findings in d73d648. The branch is rebased on current main; the description migration is now revision 031 after main revisions 029 and 030, restoring one Alembic head. Raw, prompt, and plugin draft materialization now carry description into the projected Automation, with prompt-draft dispatch coverage. Ruff, formatting, Pyright, migration-history tests, and schema tests pass locally (30 tests). Draft/capabilities integration tests could not start because Docker is unavailable on this machine. |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope
In scope. AGENTS.md gives this repository ownership of the automation definitions, the API/schema contract, and Alembic migrations, and OpenHands/OpenHands#16925 (open, ready-for-dev) names OpenHands/automation as step 1 of this feature.
Assessment
This revision resolves both earlier blocking findings, and I found no new material issues.
Migration collision fixed. The rebase onto current main (which now carries 029_add_git_sync_failure_backoff.py and 030_kv_per_key_rows.py) renumbers this change to 031_add_automation_description.py (revision = "031", down_revision = "030"). I confirmed on the branch that Alembic reports a single head (031), and on the actual PR merge ref (c61dd9b) that alembic upgrade head succeeds and creates the nullable description TEXT column, with tests/test_db.py::TestSqliteMigrations::test_migrations_run_on_sqlite passing. This satisfies the AGENTS.md requirement of one coherent migration with a single Alembic head and a complete upgrade path.
Draft materialization fixed. _materialize_raw_draft, _materialize_prompt_draft, and _materialize_plugin_draft now pass description=body.description into the projected Automation, and the prompt-draft dispatch test asserts the materialized automation carries it.
Draft/preflight contract. _BaseDraftBody carries the same optional 2,000-character description, so /v1/validate and the /v1/drafts CRUD accept what the create endpoints accept. I verified the three draft endpoints normalize a body containing description, and that an over-length value is rejected.
Rest of the change is consistent with repo guidance. Text/nullable column; AutomationResponse.description defaults to None so pre-migration rows still serialize; git-sync serializes the field only when non-null (so existing automation.yaml files are not rewritten) and reads it back through _validate_and_resolve_fields; and description is threaded through the raw, prompt-preset, and plugin-preset create paths, with PATCH picking it up via model_dump(exclude_unset=True) and the existing setattr loop. automationDescription is added to _STATIC_FEATURES, matching the linked issue's checklist.
Tests. Focused suites pass on this head: tests/test_schemas.py tests/test_git_sync_serializer.py tests/test_preset_router.py tests/test_db.py tests/test_migration_history.py tests/test_git_sync.py tests/test_git_sync_state.py tests/test_git_sync_client.py → 316 passed, 45 skipped. The testcontainers-backed capabilities/draft suites still cannot run here (no Docker socket), and the head's ci, Run tests, and Docker runs currently report action_required, so those paths remain unexercised in CI. That is a CI-availability gap rather than a defect in the change; the new assertions in tests/test_capabilities_router.py and tests/test_draft_router.py will run there once CI is enabled.
✅ APPROVED
enyst
left a comment
There was a problem hiding this comment.
I'm an AI agent (Claude, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.
One mechanical change before this can merge: #517 merged today with its own 031_add_webhook_event_id_header.py, so this migration needs to become 032 (revising 031). Otherwise I'd approve. all-hands-bot's latest review (2026-10-03, on this same head d73d648) covers the migration renumbering and the draft materialization fix, and I agree with both.
What else I checked:
- Clearing with
null. The bot's earlier note (2026-09-30) said that a PATCH withdescription: nullwould not clear the field. On this head it does:update_automationsets every key frommodel_dump(exclude_unset=True)(openhands/automation/router.py:455), and changing a loaded value toNoneis an ordinary ORM change, so the UPDATE writes NULL. OpenHands/OpenHands#16925 has an acceptance criterion for clearing withnull, so a router test for that would lock it in. I don't consider the test blocking. - CI. The test workflows (
ci,Run tests,Docker) on this head areaction_required, so the Postgres-backed capabilities and draft tests have not run anywhere yet. A maintainer needs to approve the fork workflows before merge. - PR description. It still says "migration 029"; the migration is now
031.
d73d648 to
20ef0c5
Compare
|
Resolved the new migration collision in 20ef0c5 after rebasing onto current main. The description migration is now revision 032 and revises main revision 031, restoring a single Alembic head. Migration-history and schema suites pass locally (30 tests); Ruff and Pyright are clean. |
Signed-off-by: Rohithmatham12 <rohithmatham@gmail.com>
Co-authored-by: openhands <openhands@all-hands.dev>
20ef0c5 to
e8c1e4f
Compare
|
Rebased onto current |
Prerequisite for OpenHands/OpenHands#16925.
Problem
Agent Canvas needs a short human-readable description for each automation, but the automation service currently stores and returns only the name. The Canvas work cannot safely depend on this field until it is part of the service contract.
Changes
Verification
The testcontainers-backed capability endpoint suite could not run locally because Docker is unavailable; repository CI should exercise that integration path.