[ak684]

Prompt and plugin preset automations keep their repositories in the preset package (repos_config.json) and in preset_metadata.repos, but PATCH /v1/{automation_id} could not change them, so they were fixed when the automation was created. Agent Canvas's editor therefore could not save repository changes (OpenHands/OpenHands#18775).

PATCH now accepts repos, with the same entries as preset creation, for prompt and plugin presets:

• The list replaces the saved one; an empty list or null removes them.
• The preset package is rebuilt with the new repos_config.json, in one rebuild with any prompt change in the same request, and preset_metadata.repos is updated. An unchanged list rebuilds nothing.
• Other automations (custom code, templates and plugin specs) answer 422, as does a preset whose package cannot be rebuilt.
• Capabilities advertise a new repoUpdate feature, so clients offer repository edits only where the service supports them.

Tests: uv run pytest on 77dd60e: 2122 passed, 7 skipped. The GCS storage integration test was excluded because its emulator image could not be pulled here. 10 of the 12 new PATCH tests fail on main.

Recording: Agent Canvas running this branch's service together with the matching Canvas change. An automation created with OpenHands/docs gains OpenHands/extensions in Edit, and both are still there after Save, Close and reopening. The automation's downloaded package then lists both repositories in repos_config.json.

On an OpenHands Enterprise test install running this branch's image (automation:sha-9bdb37e) with Agent Canvas from OpenHands/OpenHands#18776, capabilities list repoUpdate. In Edit, adding a second repository and saving, then removing the first and saving again, updated the automation's saved repositories each time, and reopening Edit showed the same list.

[ak684]

@OpenHands /codereview-roasted

Review exactly commit 9bdb37e. If the PR head changes, report that and do not approve another commit. Review only: do not push code, merge, deploy, or change attribution. Check correctness, regression coverage, and whether the PR validation claims match the evidence. Keep code comments short. Post the substantive review and link it in your completion message. End the substantive review with these plain lines (no code fence), choosing one verdict:

REVIEWED_HEAD: 9bdb37e
REVIEW_REQUEST: 2a71432d221e4615bbb51239e3d6aef1
VERDICT: APPROVED

Use VERDICT: CHANGES_REQUESTED instead when findings require changes. If unable to review, explain the blocker without an approval verdict.

[openhands-ai]

I'm on it! ak684 can track my progress at all-hands.dev

[github-actions]

Warning

Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it.

[ak684]

Reviewed commit 9bdb37e7158636bc34f4817be2141ecc1ddba509 (PR head; base 77dd60e). PR head is unchanged from the review request.

Taste: good. Focused, minimal change that reuses the existing preset-tarball regeneration machinery instead of inventing new plumbing.

Correctness — traced end to end:

• PATCH accepts repos on UpdateAutomationRequest with the same RepoSource entries and single-item normalization as creation. A preset bakes repos into repos_config.json and preset_metadata.repos; both are now updated together in update_automation, with setattr only touching real columns (repos is popped first).
• One rebuild covers prompt+repo changes in one request (files dict drives a single regenerate_preset_tarball call), and an unchanged list rebuilds nothing (the dump_repos(body.repos) vs stored preset_metadata.repos comparison is shape-consistent with creation, since both use model_dump(exclude_none=True)).
• An empty list / null drops the metadata key and removes the file from the tarball (_replace_files_in_tarball handles None and preserves sibling bytes).
• Non-prompt/plugin automations 422, and a non-rebuildable tarball during a repo change 422s with nothing persisted (rollback on the exception) — correct, since recording repos the tarball won't clone would claim a checkout the run never makes. Prompt-only changes keep their existing graceful no-op when not rebuildable.
• Auth is enforced: _assert_can_manage + _assert_can_update_fields reject non-creators (403) before the repos path is reached.
• Capabilities advertise the new repoUpdate feature; it is on the ready-path response only.

Regression coverage — solid. The new PATCH tests exercise real tarfile manipulation and a stateful in-memory store (byte-for-byte assertions on the rebuilt tarballs, write_stream.await_count == 1, unchanged-path rebuild skip, plugin preset preserving plugins_config.json, empty-list removal, 422 paths, non-creator 403). The 10-of-12-on-main claim checks out: the two that pass on main are the 403 and invalid-repo cases, which are validated before the new code path.

Validation vs evidence — locally I ran the full suite (uv run pytest tests/): 2130 passed, 7 skipped, 1 failed (test_git_sync.py::TestTarballUploadLifecycle::test_profile_import_uses_api_validation[None]). That failure is not attributable to this PR: it passes in isolation, the only git_sync change here is a docstring, and the CI unit-tests job is green on this head. The GCS-integration storage tests also ran here (the emulator image was pullable), accounting for the 8-test delta vs the PR's reported 2122. Ruff on all changed files: clean.

No material findings.

[RISK ASSESSMENT] LOW — server-side, additive API surface; no new credentials, paths, or lifecycle transitions; the destructive tarball rewrite reuses the pre-existing, transaction-scoped regeneration path.

REVIEWED_HEAD: 9bdb37e
REVIEW_REQUEST: 2a71432d221e4615bbb51239e3d6aef1
VERDICT: APPROVED

[openhands-ai]

The PR head is unchanged from the reviewed commit, and the review has been posted.

Review summary:

I reviewed PR #625 ("feat: let PATCH change a preset automation's repositories") at commit 9bdb37e7158636bc34f4817be2141ecc1ddba509 (head unchanged from the request). The substantive review is posted at: #625 (comment)

Key findings from my review (verdict: APPROVED):

Correctness — The change is focused and reuses existing machinery:

• PATCH accepts repos (same RepoSource entries and single-item normalization as preset creation) and updates both the tarball's repos_config.json and preset_metadata.repos together; prompt+repo changes rebuild once.
• Empty list/null removes repos; unchanged list rebuilds nothing (shape-consistent comparison via model_dump(exclude_none=True)).
• Non-presets and unrecoverable tarballs 422 with a rollback; prompt-only changes keep the existing graceful no-op. Auth is enforced (403 for non-creators).

Regression coverage — solid: the new PATCH tests exercise real tarfile manipulation and byte-for-byte rebuilt-tarball assertions. The "10 of 12 fail on main" claim checks out (the two passing on main are the 403 and invalid-repo cases validated before the new path).

Validation vs. evidence — I ran the full suite locally: 2130 passed, 7 skipped, 1 failed (test_git_sync.py git-sync test). That failure is a pre-existing environment/ordering flake, not a PR regression: it passes in isolation, the only git_sync change in the PR is a docstring, and CI's unit-tests job is green on the exact head. The GCS integration tests ran here (the emulator image was pullable), accounting for the 8-test delta vs. the PR's reported 2122. Ruff is clean.

The review ends with the required verification lines:

REVIEWED_HEAD: 9bdb37e7158636bc34f4817be2141ecc1ddba509
REVIEW_REQUEST: 2a71432d221e4615bbb51239e3d6aef1
VERDICT: APPROVED

No code changes were made — the task was review-only, as requested.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Reviewed head 9bdb37e7158636bc34f4817be2141ecc1ddba509 (base main @ 77dd60e). The PR head matches the review request; the PR is open, labelled type: feat, and the linked issue OpenHands/OpenHands#18775 is open with bug + ready-for-dev.

Scope gate — passes. This changes the automation service's own PATCH/preset contract (tarball regeneration, preset_metadata, capabilities). That is this repository's ownership per AGENTS.md and .agents/skills/custom-codereview-guide.md; the Canvas-side read/edit of repositories correctly lives in OpenHands/OpenHands#18776.

Correctness — no material findings. I traced the changed path end to end:

• repos is accepted on UpdateAutomationRequest with the same RepoSource entries and the same single-item normalization as preset creation. I compared validation of create vs update for http:// (upgraded to https://), short owner/repo without provider (422), ftp:// (422), git@… and list/object forms — they agree.
• repos is popped from update_data before the setattr loop, so no non-column attribute is written; the preset-type gate (prompt/plugin only) raises 422 before anything is applied.
• Prompt and repo edits in one request produce one regenerate_preset_tarball call (files drives a single rebuild). repos_config.json is added when absent and removed (not written empty) when the list is emptied; sibling members are copied through.
• Unchanged list rebuilds nothing, and the "changed" comparison is shape-consistent with creation (dump_repos / model_dump(exclude_none=True) on both sides).
• A repo change whose tarball cannot be rebuilt raises 422 with the metadata untouched, so preset_metadata.repos can never claim a checkout the run will not make. Prompt-only edits keep their existing graceful no-op.
• Authorization is unchanged and correct: _assert_can_manage plus _assert_can_update_fields reject a non-creator's repo edit with 403 before the new path is reached.
• The old upload is soft-deleted in the transaction and its object removed only via background_tasks after commit, matching the pre-existing regeneration contract (no new partial-write or stranded-object window).

Verification. Docker is not available in this environment, so I could not run the container-backed PostgreSQL fixture. I ran the suite against a SQLite-backed copy of the harness (tracked files untouched):

• The 12 new PATCH tests: 12 passed.
• The same 12 tests against main @ 77dd60e: 10 failed, 2 passed — the PR's "10 of 12 fail on main" claim checks out (the two passing are the 403 and invalid-repo cases, rejected before the new path).
• tests/test_router.py, test_capabilities_router.py, test_schemas.py, test_openapi.py, test_route_permissions.py, test_preset_router.py, test_draft_router.py, test_tarball_validation.py, test_uploads.py, test_disable_automation.py, test_git_sync_serializer.py, test_verify_automation_skill.py: all passed. Note the new tests exercise router/file-store logic, not PostgreSQL-specific behavior.
• ruff check and ruff format --check on the changed files: clean. map check and map coverage for the verify-automation skill: clean.
• Current CI for this head is green (unit-tests, backend, Build and Push Automation Image, check-pr-artifacts, pr-title); cleanup-after-merge is skipped as expected.

Non-blocking. Adding repoUpdate to _STATIC_FEATURES leaves the verify-automation feature map's exact-list assertion in F03-capabilities-and-validate.md stale (it does not include the new feature). Per references/maintenance.md, map corrections ship in their own maintenance PR rather than with the product change, so this does not block; worth folding into the next maintenance pass.

No material correctness, security, compatibility, or acceptance-criterion defect found on this head. Issue #18775's service-side acceptance criteria (repository updates persist; unsupported automations are refused rather than silently discarding the change; an unchanged list is left alone) are met.

✅ APPROVED

[openhands-release-bot]

🚀 Released in 1.24.0.