Repository navigation
Conversation
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
9063d70 to
1d15a06
Compare
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Verdict: ✅ APPROVE
Taste Rating: 🟢 Good taste
Clean, additive feature that separates archive state from runtime availability. The archived_at field is persisted independently in meta.json, the search filter is a simple boolean partition, and Docker runtime access is blocked via _is_archived guards in _workspace_or_404 and reprovision_conversation_runtime.
Key observations:
- Backward compatible:
archived_at: datetime | None = Field(default=None)onStoredConversation(which inheritsextra="forbid"fromConversationConfig) is purely additive — oldmeta.jsonfiles without the field load fine via the default. - Idempotent archiving:
archived_at = (record.stored.archived_at or now) if archived else Nonepreserves the original timestamp on re-archive. Good. - Runtime isolation:
_get_or_load_event_service_lockedreturnsNonefor archived conversations, blocking lazy recovery. The archive/unarchive Docker routes bypass_workspace_or_404so unarchive always works. The catch-all proxy inherits the archived guard via_workspace_or_404. - Thread safety:
set_conversation_archivedruns underasync with self._conversation_lifecycle(conversation_id). Correct. - Tests: Cover archive→search filtering, runtime stop on archive, 409 on reprovision/proxy for archived conversations, and the router-level 404 path. The service test exercises the disk-load path for
_conversation_infoafter the event service is popped.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
- No agent behavior, prompt, or tool changes — purely agent-server conversation lifecycle infrastructure. No eval-risk.
- No dependency changes, no version bumps.
- Additive REST endpoints and TypeScript APIs with no breaking changes to existing contracts.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it is merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Verdict: ✅ APPROVE
Taste Rating: 🟢 Good taste
Clean, additive feature that separates archive state from runtime availability. The archived_at field is persisted independently in meta.json, the search filter is a simple boolean partition, and Docker runtime access is blocked via _is_archived guards in _workspace_or_404 and reprovision_conversation_runtime.
Key observations:
- Backward compatible:
archived_at: datetime | None = Field(default=None)onStoredConversationis purely additive — oldmeta.jsonfiles without the field load fine via the default. - Idempotent archiving:
archived_at = (record.stored.archived_at or now) if archived else Nonepreserves the original timestamp on re-archive. - Runtime isolation:
_get_or_load_event_service_lockedreturnsNonefor archived conversations (line 1215), blocking lazy recovery. The Docker archive/unarchive routes bypass_workspace_or_404so unarchive always works. The catch-all proxy inherits the archived guard via_workspace_or_404. - Thread safety:
set_conversation_archivedruns underasync with self._conversation_lifecycle(conversation_id)with disk writes viaasyncio.to_thread. Correct. - Docker ordering:
_set_docker_archive_statepersists metadata before callingregistry.stop(), so the conversation is marked archived even if container reaping fails. - Tests: Cover archive→search filtering with a real service persistence round-trip, runtime stop on archive, 409 on reprovision/proxy for archived conversations, and router-level 404 paths.
[RISK ASSESSMENT]
- Overall PR: 🟢 LOW
- No agent behavior, prompt, or tool changes — purely agent-server conversation lifecycle infrastructure. No eval-risk.
- No dependency changes, no version bumps.
- Additive REST endpoints and TypeScript APIs with no breaking changes to existing contracts.
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
The review was posted successfully (state: GITHUB_REVIEW_POSTED This comment was posted by an AI agent (OpenHands). |
a96bfe3 to
c7b11a6
Compare
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
🛠️ REQUEST_CHANGES · Risk: HIGH · 066231f17481
OpenHands-Astra, helping Engel Nyst (@enyst). AI review.
Four lifecycle fixes needed:
- [P1] Shutdown: A loaded inner service can overwrite the archive timestamp during graceful shutdown. Preserve it across close/reopen.
- [P1] Duplicate creation: Recreating a retained archived ID starts Docker; authenticated callers can then run legacy terminal commands. Enforce archive checks first.
- [P2] Deletion: Archived records return 400/409 before cleanup. Delete retained data without requiring runtime admission.
- [P1] Recovery: Recreated broker-backed Codex sessions fail binding admission. Restore credentials before checking activation.
Limits: Source review; no runtime/tests/CI validated. Issue #4994 and stacked PR #4998 bodies unavailable.
🤖 Both bot reviews: major misses. Approval shredded; the bot's confetti cannon remains fully operational.
Receipts and review scores
Code: 0 major miss · 1 minor miss · 2 defensible (50–50) · 3 correct. Policy and reporting are separate.
| Review @ commit | Code | Policy | Report | Evidence |
|---|---|---|---|---|
5186965860 @ 1d15a06627ee |
0 | UNKNOWN | UNKNOWN | Missed shutdown archive erasure and blocked deletion at its own commit. Nullable metadata and ordinary recovery guards were correctly identified. Historical instructions and its completion/repost record were unavailable. Shutdown path. |
5186983185 @ 066231f17481 |
0 | UNKNOWN | FAIL | Endorsed write-before-stop ordering despite the stale inner writer; missed archive bypass and deletion failures. Reported APPROVED one second after self-dismissal. Historical instructions unavailable. Completion claim. |
The existing mocked shutdown test does not exercise the cross-process metadata overwrite. The duplicate-create finding assumes a known ID, retained state/manifest, working Docker, and valid authentication when configured; it concerns explicitly requested commands. No unauthenticated access is established. Broker recovery assumes no separate external credential activation. Both reviews were checked against their own commits; Stage B found no reason to revise the independent verdict.
Generated by OpenHands AI using openai/gpt-6-astra; reviewed #4999 at 066231f174815735d9accfdd3d87891d3f842c83.
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: this change belongs in this repository. openhands-agent-server owns conversation lifecycle, the conversation catalog, and the canonical /api/** REST contract. The linked issue #4994 ("[Feature]: Add explicit conversation archive metadata and control-plane APIs") is open with the ready-for-dev label, and the OpenAPI/TypeScript additions are additive. Head is unchanged at 511a277714d1df9c8962982432b7c8fbd52a5c9e, mergeable, with all current-head checks green (52 success, 9 intentionally skipped, no failures).
Findings
No material (blocking) findings on this head. The previously blocking defect — _get_or_load_event_service_locked's unconditional archive short-circuit made archived conversations undeletable in local mode (DELETE -> False -> HTTP 400) — is fixed via the allow_archived parameter that delete_conversation() passes as True, covered by test_archived_conversation_is_still_deletable. Archive/unarchive, the archived/unarchived search partition, the Docker _is_archived runtime guards (start/reprovision/proxy), and the stop-before-persist ordering all behave as documented. The sole review thread on this PR is resolved.
Re-verified on the exact head: pytest tests/agent_server/test_conversation_service.py tests/agent_server/test_conversation_router.py tests/agent_server/docker_runtime/test_routes.py tests/agent_server/test_credential_binding.py -> 260 passed.
Non-blocking observation (deferring to the maintainer): count_conversations() still counts every catalog record while search_conversations() excludes archived rows by default, so a client reading /api/conversations/count alongside /api/conversations/search can disagree once anything is archived. The count route is untouched by this PR and the issue's acceptance criteria only require search filtering, so this does not block.
No security issues: the new routes are additive, reuse the existing get_conversation_service/get_registry dependencies, and do not expose secrets or bypass authentication. No agent-behavior, prompt, or dependency changes; eval risk is low.
✅ APPROVED
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Reviewed head 511a2777 (matches the requested SHA; PR open, no labels, linked issue #4994 still ready-for-dev). CI for this head is green (21 success, 9 skipped), and the focused suites pass locally: tests/agent_server/test_conversation_service.py tests/agent_server/test_conversation_router.py tests/agent_server/docker_runtime/test_routes.py -> 226 passed.
The tests/agent_server/test_credential_binding.py failures I observed are environmental and pre-existing, not introduced here: the file fails 12 tests on base a8a9fc6e and 13 on head (the extra is this PR's new test_local_codex_binding_is_restored_before_cold_admission). Root cause is this sandbox exporting OH_RUNTIME_LAUNCHED_PROFILE with secret_refs=["GITHUB_TOKEN_REVIEW"], so _profile_allows_secret(stored, "CODEX_AUTH_JSON") is False and no binding is created. I am not treating this as a blocker.
Two behavior gaps remain on the current head, both about the archive contract the PR documents:
1. GET /api/conversations/count ignores archive state (body-level, no changed line). _count_conversations returns len(self._conversation_records) when no status filter is given, so archived conversations are still counted, while search_conversations now defaults to archived=False. Demonstrated on head: after archiving one conversation, /api/conversations/count -> 1 but /api/conversations/search returns 0 items. Search and count previously agreed; the new default silently diverges them, so any client using count for totals will over-report. Consider adding an archived filter to count (or excluding archived by default) to match search.
2. Local runtime mode does not enforce the archive contract (see inline). The PR summary claims archived runtime creation, recovery, and reprovision are blocked. That holds in Docker mode (409 from _is_archived), but the default local runtime (conversation_runtime="local") silently returns HTTP 200 for a resume of an archived conversation and returns 404 for its event history, contradicting the README's "preserves history" / "DELETE permanently removes" semantics.
🔄 CHANGES REQUESTED
…eadable Local-runtime archiving diverged from the documented contract in two ways; both now match the Docker runtime. 1. Resume silently succeeded. start_conversation() fell through to _conversation_info() for an archived id and returned 200 while leaving _event_services empty, so a client got success for an operation that did nothing. It now raises ConversationArchivedError, mapped to 409 to match Docker's rejection of the same request. 2. Archived history became unreadable. Read routes depended on get_event_service, whose archived guard returned None and surfaced 404. A new get_read_event_service seam falls back to the persisted event log for archived conversations, mirroring Docker's serves_persisted_event_reads=True. The four read endpoints (search/count/get/batch-get) use it; runtime and write routes keep the strict seam, so archived-is-not-runnable is preserved. Tests: service-level regression tests for the 409 guard, the readable persisted history, and is_conversation_archived for unknown ids; router-level 409 test; dependency-seam tests for archived/live/unknown resolution; read endpoint tests retargeted to the new seam. Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: this change belongs in this repository. openhands-agent-server owns conversation lifecycle, the conversation catalog, and the canonical /api/** REST contract; the linked issue #4994 is open with ready-for-dev, and the OpenAPI/TypeScript additions are additive. Reviewed head c88e9c96b02c6f62fd68058d241618bfe56a1fcd (matches the requested SHA; PR open, mergeable, no labels).
Findings
No material (blocking) findings on this head. The two divergences raised on the previous head are fixed and I re-reproduced each against c88e9c96:
- Archived resume now fails loudly.
start_conversation()raisesConversationArchivedError, whichconversation_router.start_conversationmaps to 409 (matching Docker) andapi.pyhandles globally. Verified: resume of an archived id raisesConversationArchivedError, and_event_servicesis left empty afterwards — no runtime is created for a conversation that never started. - Archived history stays readable. The new
get_read_event_serviceseam resolves an archived conversation throughget_persisted_event_servicewhile runtime/write routes keep the strictget_event_service. Verified: strictget_event_service(archived)isNone, the persisted read service resolves andsearch_events()returns anEventPage, and unknown ids still 404.
Adjacent contract behavior also holds on this head: delete_conversation(archived) returns True; unarchive clears archived_at and a subsequent resume succeeds; fork of an archived conversation returns None rather than creating a runtime.
Validation on the exact head (with the sandbox's OH_RUNTIME_LAUNCHED_PROFILE/OH_CONVERSATIONS_PATH/OH_PERSISTENCE_DIR/OH_SECRET_KEY cleared, which otherwise perturbs credential-binding tests): tests/agent_server/test_conversation_service.py test_conversation_router.py test_event_router.py test_dependencies.py test_api.py test_event_service.py docker_runtime/test_routes.py test_search_limit_validation.py test_credential_binding.py -> 406 passed. CI on this head is green apart from one check noted below.
Non-blocking notes
unresolved-review-threadsis a stale race, not a defect. That check failed at14:56:05reporting "Found 1 unresolved review thread(s)", while the author resolved the thread at14:56:02— the gate sampled before the resolution landed. All review threads are now resolved (unresolved=0), so a rerun of the check should clear it. No code change is implied.count_conversations()still ignores archive state (re-verified on this head): after archiving one conversation,search_conversations()returns 0 items whilecount_conversations()returns 1. The count route is unchanged by this PR and #4994's acceptance criteria only require search filtering, so this does not block; adding the samearchivedfilter to count would keep the two consistent. Deferring to the maintainer.
No security issues: the new endpoints and read seam are additive, reuse the existing get_conversation_service/get_registry dependencies, and do not expose secrets or bypass authentication. No agent-behavior, prompt, or dependency changes.
✅ APPROVED
… by archive Archiving persisted the outer catalog's copy of StoredConversation. In Docker mode that copy can predate what the inner runtime wrote (for example a title set on the runtime, or its final shutdown write), and in local mode an open EventService may have replaced its stored metadata without syncing the catalog record. Either way the archive marker overwrote newer metadata. Reload persisted metadata after stopping the Docker runtime, and base the local update on the live service's copy. GET /api/conversations/count now accepts the same archived filter and default as search, so a count shown next to the default listing no longer includes archived conversations. Co-authored-by: openhands <openhands@all-hands.dev>
|
This comment was written by an AI agent (OpenHands) on behalf of @neubig. I pushed @enyst's review (5227608995)
all-hands-bot's non-blocking count note (5348102803 / 5354726079). Tests: All review threads were already resolved. The HUMAN section of the PR description was not changed. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: this belongs in this repository. openhands-agent-server owns conversation lifecycle, the conversation catalog, and the canonical /api/** REST contract; linked issue #4994 is open with ready-for-dev, and the contract additions are additive. Reviewed head 768b6e7e827fdd47e893ea55f43f497f356d441f (matches the requested SHA; PR open, mergeable, no labels).
Findings
No material (blocking) findings on this head. This commit closes the two remaining gaps from the previous head, and I re-verified both against 768b6e7e:
- Archive no longer loses metadata written by the runtime.
docker_runtime/routers.py:_set_archive_statenow callsrefresh_persisted_conversation()afterregistry.stop()and before writing the archive marker, andset_conversation_archived()builds on the openEventService's stored copy (authoritative while open) instead of the catalog record. Reproduced locally with a live service whose stored title differed from the catalog: the archive response andmeta.jsonboth retain"Live title"witharchived_atset, the runtime is closed and evicted, and a fresh Docker-runtime test (test_archive_keeps_metadata_written_by_stopping_runtime) covers the stop-then-persist ordering. - Count now agrees with search on archive state.
count_conversations(status, archived=False)applies the same(archived_at is not None) == archivedpredicate as search, and the router/TypeScript client expose the parameter. Verified: with one archived conversation,count()= 0,count(archived=True)= 1, andsearch()returns 0 items; after unarchive,count()returns to 1.
Prior contract behavior still holds on this head: archived resume and proxied writes return 409, archived history stays readable via the get_read_event_service persisted-log seam, archived conversations remain deletable, and unarchive clears archived_at without provisioning a runtime.
Validation on the exact head (with the sandbox's OH_RUNTIME_LAUNCHED_PROFILE/OH_CONVERSATIONS_PATH/OH_PERSISTENCE_DIR/OH_SECRET_KEY cleared, which otherwise perturbs credential-binding tests): tests/agent_server/test_conversation_service.py test_conversation_router.py docker_runtime/test_routes.py -> 233 passed; test_event_router.py test_dependencies.py test_api.py test_credential_binding.py test_search_limit_validation.py test_openapi_contract.py test_conversation_eviction.py -> 129 passed; archive-focused selection -> 15 passed. GitHub Actions for this head is fully green (52 success, 2 skipped), with no failing or in-progress checks and no unresolved review threads.
No security concerns: the new endpoints reuse the existing get_conversation_service/registry dependencies under the standard session-key auth, the read seam is read-only, and no secrets are newly exposed. The count endpoint's archived parameter is additive and defaults to the active set, matching search.
✅ APPROVED
Resolve conflicts: keep archiving 409/ConversationArchivedError behavior from the PR, adopt main's launch_http_exception mapping and _reuse_existing_conversation_locked helper. Co-authored-by: openhands <openhands@all-hands.dev>
|
|
The merge of main into this branch placed the archived-conversation check ahead of main's conversation_dir() guard, so a symlinked runtime mount raised ValueError out of the request handler and returned 500 instead of the 422 that test_a_symlinked_conversation_dir_is_rejected asserts. Resolve the conversation directory (mapping the guard violation to 422) first, and make _is_archived treat an unresolvable directory as not archived. Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: this belongs in this repository. openhands-agent-server owns conversation lifecycle, the conversation catalog, and the canonical /api/** REST contract; linked issue #4994 is open with ready-for-dev, and the contract additions remain additive. Reviewed head 753dfa96115107bb1f1213f74c2daac91488cea9 (matches the requested SHA; PR open, mergeable, no labels).
Findings
No material (blocking) findings on this head. The only change since the previously reviewed 768b6e7e is commit 753dfa96 ("resolve symlinked mounts before the archive check"), which is a correct rebase repair rather than new behavior:
- In
docker_runtime/routers.py,start_conversationnow resolvesconversation_dir(conversation_id)(mapping the symlink-guardValueErrorto 422) before the_is_archivedcheck. Previously the archive check ran first, so a symlinked conversation directory raisedValueErrorout of the metadata read and returned 500 instead of the 422 thattest_a_symlinked_conversation_dir_is_rejectedasserts._is_archivednow also treats an unresolvable directory as "not archived" so the caller reports the guard violation itself. I confirmed on this head thatPOST /api/conversationsfor a symlinked conversation dir returns 422 (the base behavior), and the baseconversation_dir(...).joinpath("meta.json").is_file()guard is preserved on theruntime_info/credentialsroutes (their 500-on-symlink behavior is unchanged from base). - The archive feature itself is unchanged and still intact after the main merge. I re-verified: archive stops and evicts an open local service and preserves metadata the live service replaced (
test_archive_keeps_metadata_replaced_by_live_service), the Docker router reloads persisted metadata afterregistry.stop()before writing the marker (test_archive_keeps_metadata_written_by_stopping_runtime), start/resume on an archived id raisesConversationArchivedError→ 409 (test_start_on_archived_conversation_raises_instead_of_succeeding), archived history stays readable via theget_read_event_servicepersisted-log seam, archived conversations remain deletable, and count/search agree on archive state (test_archive_filter_is_independent_from_runtime,test_count_conversations_with_archived_filter).
Tests run on the exact head (with the sandbox's OH_RUNTIME_LAUNCHED_PROFILE/OH_CONVERSATIONS_PATH/OH_PERSISTENCE_DIR/OH_SECRET_KEY cleared, which otherwise perturbs credential-binding tests): docker_runtime/test_routes.py + test_conversation_router.py → 119 passed; test_conversation_service.py + test_credential_binding.py + test_event_router.py + test_search_limit_validation.py → 203 passed; test_openapi_contract.py + test_openapi_discriminator.py + docker_runtime/ → 77 passed; archive/symlink-focused selection → 10 passed. GitHub Actions for this head is fully green (50 success, 2 skipped), with no failing or in-progress checks and no unresolved review threads.
No security concerns: the archive/unarchive endpoints reuse the existing session-key-auth dependencies, the read seam is read-only, and _is_archived reading meta.json is the same trust surface the surrounding routes already use.
✅ APPROVED
Generated by OpenHands AI agent on behalf of Graham Neubig. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Scope: this belongs in this repository. openhands-agent-server owns conversation lifecycle, the conversation catalog, and the canonical /api/** contract, and clients/typescript is the in-repo client for it; linked issue #4994 is open with ready-for-dev. All contract additions remain additive. Reviewed head d52774e52fa46e90282782c82df55e7804fc68dd (matches the requested SHA; PR open, no labels). The PR diff against base 30e90260 is the 23 files reported, with no unrelated main-merge content included (the _cleanup_stale_tmux_sessions rewrite that appears in the branch history is already present at the base commit and is not part of this PR's diff).
Findings
No material (blocking) findings on this head.
The archive feature reviewed at earlier heads is unchanged and still correct. Re-verified on this head: _is_archived guards _container (all proxied runtime routes), start_conversation, and reprovision with 409; the Docker _set_archive_state stops the owned runtime and reloads persisted metadata via refresh_persisted_conversation before writing the archive marker; the local service raises ConversationArchivedError, mapped to 409 in api.py; and archived history stays readable through the get_read_event_service persisted-log seam. The symlink-ordering repair from 753dfa96 is intact (start_conversation resolves conversation_dir before the archive check, so a symlinked mount still yields 422 rather than 500).
The only code new since the last head is d52774e5 ("feat(ts-client): normalize conversation lifecycle"), which is additive TypeScript: a conversation-lifecycle model with normalizeConversationLifecycle / normalizeCloudConversationLifecycle, exported from clients.ts and index.ts, plus archived_at/runtime_status/can_resume on CloudAppConversation. I checked the two ways this could be wrong and both hold:
normalizeConversationLifecyclethrows whenarchived_atisundefined. That is safe for current servers:archived_atis a declaredConversationInfofield with aNonedefault, the conversation routes set noresponse_model_exclude_none, and the model has noexclude_noneserializer config, so Pydantic emitsarchived_at: nullon everyConversationInforesponse (start, get, search, fork). The throw therefore only fires for a genuinely pre-contract server, which is the documented intent.- The bare-
ConversationInfofallback (runtimeStatus: 'available',canResume: truewhenruntime_infois absent) is reachable only from create/fork/navigate, and none of those can return an archived conversation: fork/navigate resolve through_get_or_load_event_service, which returnsNonefor an archived record and surfaces as 404. So the fallback cannot mislabel an archived conversation as resumable.
Tests run on the exact head (with the sandbox's OH_RUNTIME_LAUNCHED_PROFILE/OH_CONVERSATIONS_PATH/OH_PERSISTENCE_DIR/OH_SECRET_KEY cleared, which otherwise perturbs credential-binding tests): test_conversation_service.py + test_conversation_router.py + docker_runtime/test_routes.py + test_api.py → 299 passed; test_event_router.py + test_credential_binding.py + test_search_limit_validation.py + test_openapi_contract.py + docker_runtime/ → 138 passed. TypeScript: npm run build succeeded and vitest for conversation-lifecycle.test.ts + api-clients.test.ts → 111 passed. GitHub Actions for this head is fully green (50 success, 2 skipped), with no failing or in-progress checks and no unresolved review threads.
No security concerns: the archive/unarchive endpoints reuse the existing session-key-auth dependencies, the read seam is read-only, and _is_archived reading meta.json is the same trust surface the surrounding routes already use.
✅ APPROVED
This request was from an old pass pushing Astra to the limits to detect things, its issues have been solved!
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
This PR looks very clean to me and a nice addition to the REST API, when seen in isolation. I left a small question on Slack, about the semantics overall, throughout the ecosystem: since there are or have been multiple behaviors like “archived”: Slack discussion
4 participants
HUMAN:
Rerun formatting/lint and focused tests after replacing undefined
BASE_STATEwithbase_state.json.AGENT:
Why
Conversation archive state must be explicit and independent from runtime availability; missing infrastructure is not equivalent to a user-controlled archive decision. This PR now targets current
main; previously stacked Docker/runtime history was removed after that foundation merged.Summary
archived_atindependently from runtime stateREST API contract changes
Compared with base OpenAPI
30e90260c914for public/api/**paths.How to Test
uv run pre-commit run --files ...uv run pytest -q tests/agent_server/test_conversation_service.py tests/agent_server/test_conversation_router.py tests/agent_server/docker_runtime/test_routes.py(225 passed)uv run pytest -q tests/agent_server/docker_runtime/test_routes.py tests/agent_server/test_credential_binding.py(44 passed)npm run format:checknpm run buildnpm test -- src/__tests__/api-clients.test.ts(86 passed)Evidence (768b6e7)
I tested live against real
python -m openhands.agent_serverprocesses, using the local runtime and the Docker runtime (OH_CONVERSATION_RUNTIME=docker, inner imageghcr.io/openhands/agent-server:c88e9c9-python, Docker 29.8.1). Every request went through HTTP withX-Session-API-Key. I used the same scripts before the fix (c88e9c9) and after it (768b6e7).Docker runtime: archiving lost metadata written by the runtime. Flow: create, send an inner
PATCHwith{"title": "Title set by runtime"}through the proxy, archive from the outer server, then readmeta.jsonfrom disk.Before (c88e9c9):
After (768b6e7):
Local runtime:
GET /api/conversations/countcounted archived conversations. Default search hid them, but count still included them.Before (c88e9c9), with the conversation archived:
After (768b6e7), full lifecycle:
Automated checks on 768b6e7:
uv run pre-commit run --files <10 changed files>: all hooks passed (ruff format, ruff lint, pycodestyle, pyright, import rules).uv run pytest -q tests/agent_server --ignore=tests/agent_server/test_agent_server_wsproto.py: 2238 passed, 13 deselected (stress).tests/agent_server/test_agent_server_wsproto.pywas run separately: 11 passed.test_centralized_lease_renewal_invokes_renew. That test does not touch the changed code, it passed 3 of 3 times when run alone, and it passed in the full runs above.test_archive_keeps_metadata_written_by_stopping_runtimeuses a realConversationServicebehind the Docker router. On c88e9c9 it failed withassert None == 'Title set by runtime'.test_archive_keeps_metadata_replaced_by_live_servicecovers the local case where an openEventServicehas changed its stored metadata but the catalog record has not. On c88e9c9 the title came back asNone.test_archive_filter_is_independent_from_runtimenow also checks count.test_count_conversations_with_archived_filtercovers the router.tests/agent_server/test_openapi_contract.pyandtests/agent_server/test_openapi_discriminator.py: 13 passed.check_agent_server_openapi_quality.py: passed. The count endpoint gained an optionalarchivedquery parameter (default: false). This is an additive contract change.clients/typescript):npm run format:checkandnpm run buildpassed, andnpm run lintreported 0 errors.npm test -- src/__tests__/api-clients.test.ts: 86 passed.countConversationsnow acceptsarchived, and the test asserts that the request goes tocount?status=idle&archived=true.This evidence was collected by an AI agent (OpenHands) on behalf of the user.
Closes #4994
This pull request was created by an AI agent (OpenHands) on behalf of the user.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:d52774e-pythonRun
All tags pushed for this build
About Multi-Architecture Support
d52774e-python) is a multi-arch manifest supporting both amd64 and arm64d52774e-python-amd64) are also available if needed