Repository navigation
fix(agent-server): propagate the selected title profile into Docker runtimes - #5315
mstevens843 wants to merge 6 commits into
Conversation
A Docker-backed conversation carries the title LLM profile only as a name. The inner agent-server resolves that name in the profile store under its own persistence directory, which is the private runtime directory of the conversation and is never seeded. Auto-titling therefore reports that the profile is not found and falls back to the agent LLM, which for ACP agents is an inert sentinel, so the title becomes a truncated message. When a conversation is first established, the outer server now copies the selected profile, and only that profile, into the runtime profile store. It loads the profile from the host store with the host cipher, resolves a linked provider connection into inline credentials, and saves the copy encrypted with the runtime key. A repeated start of an existing conversation keeps that snapshot, whatever the host profile or the new request says. A retry of an incomplete creation replaces what the earlier attempt staged, so the runtime store never holds more than the selected profile. The request contract, the container mounts, and the existing missing-profile fallback are unchanged. Fixes issue 5310.
|
📁 PR Artifacts Notice This PR contains a |
⏳ 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. |
|
Hi, I updated the branch with current main; the new head is 13c8ab2. The contribution files are unchanged. The fork workflows are awaiting maintainer approval. Could someone approve those runs so CI and the automated review can proceed? |
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).
Summary
The core change is well-targeted and matches the issue's smallest coherent scope: stage_title_profile() copies only the selected profile into the conversation runtime's own profiles/ store, resolves a linked provider connection into inline credentials (the runtime has no provider store), encrypts with the runtime key, and leaves the existing title_llm_profile -> agent.llm -> truncation fallback untouched. The established-conversation gate, stale-attempt cleanup under provisioning.lock(), and the persistence_dir()/lock() refactor are sound, and the staging call sits correctly between provisioning.create() and get_or_create(), so the container starts with the profile already present.
Scope check: this belongs in this repository (Agent Server Docker runtime) and has the product direction the issue's triage already specified. No ready-for-dev or architecture gate applies.
Material finding (blocking): a new test in this PR fails on the current head
tests/agent_server/docker_runtime/test_mediation.py::test_runtime_auto_title_uses_the_staged_profile_for_an_acp_style_agent fails on 13c8ab2:
AttributeError: 'types.SimpleNamespace' object has no attribute 'get_llm_call_context'
conversation_service.py:2658 in _generate_title_traced
conversation.get_llm_call_context() if conversation else LLMCallContext()
Reproduced in a clean checkout of the head (uv sync --dev, then uv run pytest tests/agent_server/docker_runtime -q): 1 failed, 57 passed; the 18 new tests otherwise pass. The PR's "58 passed" does not hold on this head.
Cause: the test stubs the inner conversation as service._conversation = SimpleNamespace(agent=SimpleNamespace(llm=...)) (line 355), but _generate_title_traced now calls conversation.get_llm_call_context() on the live conversation object. That accessor was introduced on main by bd33d63 refactor(sdk): centralize LLM call context (#4159) (2026-09-29), which is not an ancestor of this head, so the stub had no such method when the test was written. The merge commit 13c8ab2 merged main as of ~2026-09-27 and therefore does not carry that refactor.
Verification that the failure is only the missing stub method: adding conv.get_llm_call_context = lambda: None to the stub and leaving everything else identical makes the test pass and asserts the intended behavior (calls == [("title-aux", "aux-secret")], title == "Staged title"). So the production staging path is fine; the test just needs to model the current inner LocalConversation interface.
Why this matters for the merge decision: the PR claims a green focused suite as its regression evidence, and that suite is red on the exact reviewed head. It also means the branch is stale against main (two commits behind), so whichever lands first needs a rebase; and because CI on this head is not running (the fork workflows are action_required, see below), this failure would otherwise be invisible until a post-merge run.
Suggested fix
Update the stub to match the current inner conversation surface, e.g.:
conv = SimpleNamespace(agent=SimpleNamespace(llm=LLM(model="acp-managed", usage_id="acp-managed")))
conv.get_llm_call_context = lambda: None
service._conversation = conv(or use a real LocalConversation, which is what _generate_title_traced now requires), then re-run uv run pytest tests/agent_server/docker_runtime -q.
CI / evidence status on this head
Check runs for 13c8ab2: PR Artifacts and PR Description Check are green; every fork pull_request workflow (Run tests, Agent Server, Pre-commit checks, Check Docstrings, REST API breakage checks, TypeScript client *, Version bump guard, Deprecation deadlines, Persisted settings compatibility checks, Review Thread Gate) is completed/action_required — awaiting maintainer approval, so none of them has actually run against this head. The PR's reported focused-run numbers are therefore the only test evidence, and they do not reproduce here.
The live before/after evidence in .pr/docker_title_profile_e2e_output.txt is otherwise strong and does exercise the real container path (base RED, patched GREEN, isolation, repeat-start snapshot stability, reprovision); the author is explicit that the endpoint and credentials are synthetic. That evidence is convincing on its own, and the remaining gap is the red regression test on this head.
Non-blocking observations
stage_title_profile()handlesFileNotFoundError/ValueErrorfrom the host load, which covers a danglingprovider_connection_id(ProviderConnectionNotFoundsubclassesValueError) and a missing provider store, so a dangling link falls back gracefully rather than failing creation. Good.- The profile name is validated by
LLMProfileStore.save()'sPROFILE_NAME_PATTERN, so the../escapecase cannot write outside the store. Good. conversation_diris computed outside the lock whilecreate()writes the manifest inside it; a repeated start still returns early viaprovisioning.create()'s existing-manifest branch before staging, so the race the comment describes is not reachable in practice.
Verdict on the head as reviewed:
🔄 CHANGES REQUESTED
| title_llm_profile="title-aux", | ||
| ) | ||
| service.cipher = identity.cipher | ||
| service._conversation = SimpleNamespace( |
There was a problem hiding this comment.
This new test fails on the current head. _generate_title_traced now calls conversation.get_llm_call_context() on the live conversation object, but this stub is a bare SimpleNamespace:
AttributeError: 'types.SimpleNamespace' object has no attribute 'get_llm_call_context'
conversation_service.py:2658 in _generate_title_traced
Reproduced on 13c8ab2 (uv run pytest tests/agent_server/docker_runtime -q -> 1 failed, 57 passed), so the PR's "58 passed" does not hold on this head. get_llm_call_context was added to LocalConversation on main by bd33d63 refactor(sdk): centralize LLM call context (#4159), which is not an ancestor of this head (the merge 13c8ab2 predates it).
Adding conv.get_llm_call_context = lambda: None (or using a real LocalConversation) makes the test pass and asserts the intended behavior (calls == [("title-aux", "aux-secret")], title == "Staged title"), so only the stub needs updating to model the current inner conversation interface.
…text test(agent-server): align title profile fixture with conversation context Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed in a8ea83a. The test double now supplies a typed accessor returning a real The Docker-runtime suite passes all 58 tests, and applicable pre-commit hooks pass. The broader recorded result remains 2,280 passes and seven macOS Canvas backend failures also reproduced on the unchanged baseline. No new Docker trials were run. Could a maintainer approve the fork workflows on the updated head so CI and review can proceed? |
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).
Summary
This is a re-review after the head moved from 13c8ab2 to a8ea83a. The prior review's single blocking finding was that tests/agent_server/docker_runtime/test_mediation.py::test_runtime_auto_title_uses_the_staged_profile_for_an_acp_style_agent failed because the test's conversation stub was a bare SimpleNamespace lacking get_llm_call_context(). That finding is now resolved: commit a8ea83a adds a typed get_llm_call_context accessor returning LLMCallContext().for_conversation(...), and the docker-runtime suite passes on the reviewed head (uv run --frozen pytest tests/agent_server/docker_runtime -q -> 58 passed).
The production change is byte-identical to the previously reviewed tree: mediation.py (588a7f3), provisioning.py (c3182a5), registry.py (0b6d1e3), and routers.py (ae2de20) are unchanged across b3d261e..a8ea83a; only the test fixture differs. So the earlier technical assessment still applies:
stage_title_profile()copies only the selected profile into the conversation runtime's privateprofiles/store, resolves a linked provider connection into inline credentials (the runtime has no provider store to follow the reference), clearsprovider_connection_id, and encrypts with the runtime key, so the host key never enters the container. Thetitle_llm_profile -> agent.llm -> truncationfallback is untouched, and a missing/invalid/corrupt profile only logs a warning, so conversation creation still succeeds.- The established-conversation gate (
meta.json/base_state.json) plus stale-attempt cleanup underprovisioning.lock()keeps a repeated start from replacing the snapshot or importing a second credential-bearing profile, while a retry of an incomplete creation replaces what the earlier attempt staged. The staging call sits correctly betweenprovisioning.create()andget_or_create(), so the container starts with the profile already present. persistence_dir()/lock()are pure refactors of the existing lock file; no new request field, endpoint, mount, or persisted shape, and the three-mount hardening is unchanged.
Security review: only the selected profile is written, the copy is encrypted at rest with the per-runtime key and does not decrypt with the host key, profile names are validated by LLMProfileStore's PROFILE_NAME_PATTERN (the ../escape case cannot write outside the store), and no plaintext credential appears on disk. The live evidence in .pr/docker_title_profile_e2e_output.txt reproduces the issue on the base revision and shows the fix on the patched tree with an unchanged production code path; the author is explicit that the endpoint and credentials are synthetic.
CI / evidence status
Check runs for a8ea83a: PR Artifacts and PR Description Check are green; every fork pull_request workflow (Run tests, Agent Server, Pre-commit checks, Check Docstrings, REST API breakage checks, TypeScript client *, Version bump guard, Deprecation deadlines, Persisted settings compatibility checks, Review Thread Gate) is completed/action_required and has not run against this head. Focused local runs on the reviewed head are green (docker-runtime 58 passed; title-related conversation-service tests 13 passed; profile-store tests 64 passed; ruff check/format clean on the changed files). The author notes the full agent-server suite has seven macOS-only Canvas backend failures that reproduce on the unchanged baseline, which is a pre-existing environment issue, not a defect of this change.
No material correctness, security, or compatibility finding remains on the current head.
✅ APPROVED
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
The PR’s own description says the Docker live trials were recorded on an earlier tree and that no fresh Docker build/trial was run after the current-head correction. The SDK review guide requires current-head authentic before/after runtime evidence for Docker/container behavior. Please rerun the live Docker E2E script on commit a8ea83a and attach real runtime output or a screenshot/video showing both the base symptom and the corrected behavior. This comment was generated by an AI agent (OpenHands) on behalf of Graham Neubig.
|
Heads-up before you rebase: this is not a merge conflict, it's a superseded base. Gone from
So resolving the 9 conflict hunks by hand would just reinstantiate deleted code. The fix needs reimplementing against the new API — most likely by staging the title profile inside the Worth knowing: the underlying bug is still open (#5310 — the per-runtime profile store is empty), and I don't see This comment was posted by an AI agent (OpenHands) on behalf of @neubig. |
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
🔴 Taste Rating: Needs improvement before merge — The targeted propagation fix may be reasonable, but the current branch cannot be safely evaluated or merged in its conflicted state.
[SECURITY / SUPPLY-CHAIN SCREEN]
- I screened the PR-head repository criteria, full changed-file surface, scripts, secret handling, dependency surface, and workflow changes as untrusted content. I found no prompt injection, hardcoded secret, dependency-source change, malicious script, or unsafe CI expansion in the submitted diff.
[CRITICAL ISSUES]
- [Overall PR] Merge conflict: GitHub reports
mergeable: CONFLICTINGandmergeStateStatus: DIRTYfor heada8ea83a200113577fd55339593b55e2e60097f39. Rebase/merge currentmain, resolve the conflict without dropping title-profile propagation, and rerun all checks.
[TESTING / EVIDENCE GAP]
- [PR description] The described Docker trials are not sufficient live Agent Canvas proof for the current conflicted head. After resolving conflicts, attach a current-head screenshot/video showing title generation through the actual Canvas interface with the selected title profile reaching the Docker runtime and producing the expected title. Include the corresponding server/runtime log or provenance so the recording is tied to this head.
[CI / MERGEABILITY]
- The branch is conflicting. The sparse reported checks cannot validate the eventual conflict resolution and therefore must be rerun afterward.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM. This crosses host-to-container configuration mediation and profile/secret routing; an incorrect resolution can silently select the wrong model or lose credentials/configuration.
VERDICT:
❌ Needs rework: Resolve conflicts, rerun complete CI, and provide current-head live Canvas/Docker evidence.
KEY INSIGHT:
For cross-runtime profile propagation, the resolved merge result—not the pre-conflict patch—is the artifact that must be reviewed and demonstrated.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, add or update
.agents/skills/custom-codereview-guide.mdwith the/codereviewtrigger and re-request review.
This review was generated by OpenHands AI on behalf of neubig.
|
@neubig Thanks for flagging the launch-path changes in #5406. I'll first reproduce #5310 on current main, then adapt the fix to the new launch flow without restoring the removed pre-start implementation. I'll also capture fresh before/after evidence through the actual Agent Canvas and Docker runtime, with build provenance and runtime logs. Any reproduction-environment differences will be stated explicitly. The earlier Docker results will remain labeled as historical. |
Merge upstream main while preserving the published PR history. Replace the superseded mediation-based implementation with the reviewed runtime profile snapshot and lifecycle locking correction. Copy only the selected profile with resolved provider credentials and the runtime cipher. Keep the filesystem worker owned through repeated cancellation before releasing the start/delete lock. Preserve established snapshots and existing fallback, retry, release and reprovision behavior. Retain current launch APIs and add focused route regressions and sanitized Canvas/Docker evidence. No public schema or dependency changes relative to main. Co-authored-by: openhands <openhands@all-hands.dev>
|
@neubig Updated in Local verification passed 79 Docker-runtime tests plus two reviewer probes, 237 Linux-image tests, and the applicable hooks. The broader run retains eight failures also reproduced on the clean base. Canvas before/after evidence and provenance are linked in the updated description. The recordings are retained and tied to the integrated implementation through source/build-input checks, not newly recorded after the merge. Synthetic-provider and scripted-ACP limitations remain explicit. The description check now passes. Could you approve the fork workflows on this head and re-review once CI completes? |
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).
What I verified on head bf3a834
- State: PR open, non-draft,
mergeable: true(the earlierCONFLICTINGstate is resolved), basemain, headbf3a834604013d2346bbbb11fd067f581362cddc(merge ofmain1561ae0into the branch). - Diff vs the live-tested tree:
provisioning.pyandrouters.pyon this head are byte-identical to3cae388(the tree the author live-tested); onlytest_routes.pygained one unrelated main test. So the implementation under review is exactly the implementation that was exercised. - Focused tests:
uv run pytest tests/agent_server/docker_runtime -q-> 117 passed in the workspace. - Linked issue #5310: still
bug,ready-for-dev,priority:normal, open.
Implementation assessment (no material code defect found)
RuntimeProvisioningStore.prepare_title_profile() snapshots only the selected host profile into the runtime's private store (runtime-data/<id>/persistence/profiles), resolves a linked provider connection on the host and clears provider_connection_id, and re-encrypts with the runtime key. I checked the paths that matter:
- Isolation: the runtime mounts are still exactly
conversations_path/<id>,runtime-data/<id>/persistence, and the workspace; the host profile store is not mounted.provider-connectionpayloads and other host profiles do not cross. - Secrets: the staged profile is written with
include_secrets=True, cipher=identity.cipher; the host-store key cannot decrypt it, and the plaintext key is not logged or persisted in plaintext. - Fallback / non-fatal creation: missing, corrupt, invalid-name (
../escape), and dangling-provider profiles are caught (FileNotFoundError/OSError,ValueError, andProviderConnectionNotFoundwhich subclassesValueError) and logged, leaving the agent-LLM/truncation fallback intact. - Snapshot semantics: the established-conversation gate (
meta.json/base_state.json) prevents a repeat start or host edit from replacing the snapshot; a retry of a failed start clears the stale staged profile first. - Lifecycle:
start_lock()serializes the start/delete pair, cancellation of theasyncio.to_threadworker is not forwarded until the filesystem write completes, and the worker error is preserved as the cancellation cause. Lock ordering (.start.lock-> manifest.lock) is consistent, so no deadlock.
The try/except-based cancellation loop in _start_conversation is deliberate and the accompanying regression test (test_delete_waits_for_title_preparation_before_removing_runtime) covers repeated cancellation plus worker failure.
Why I am not approving yet
This is a production-facing fix whose claimed failure occurs only through the container lifecycle, so the repository review guide requires authentic before/after evidence that exercises the real entry point on the current head, and current-head CI is not usable as evidence for a changed head.
-
No CI has run on this head. Every substantive workflow (
Run tests,Agent Server,Pre-commit checks,REST API breakage checks,TypeScript client *,Version bump guard,Persisted settings compatibility checks,Deprecation deadlines,Review Thread Gate) iscompleted/action_requiredonbf3a834; onlyValidate PR descriptionandcheck-pr-artifactssucceeded. Fork workflows need a maintainer to approve them, so nothing has validated this exact commit. -
The live evidence is bound to a pre-merge tree/image, not the head. The author states plainly that the corrected image "was not rebuilt from the integration commit" and that its metadata still names the reviewed base. The equivalence argument (522 copied build inputs, 502 installed Python files, 7 imported modules hashed) is thorough, and I confirmed the title-path files are identical to the tested tree. However, the merge to this head also pulled in main commits
92e1424(reclaimer) and1561ae0(workspace shedding) that insert aReclaimerandDockerRuntimeStorageinto the Docker runtime start/stop path (registry.start->reclaimer.start(),on_stop). That lifecycle is part of the runtime this fix rides on and is not covered by the retained recordings. This is unrelated main-side work, not something this PR must fix, but it is a concrete reason the head is not evidence-equivalent to the recorded run.
Given the guide's rule for container-behavior fixes ("do not APPROVE from constructed test state alone ... repeat the same workflow on the current PR head"), the correct next step is a maintainer decision: approve/run the fork workflows on bf3a834 and either accept the provenance binding or request a head-level rerun of the live workflow. I am not claiming a code defect; the implementation and its regression tests look correct.
Deferring approval to a human maintainer.
🔄 CHANGES REQUESTED
|
@neubig Thanks for the additional review. I agree that the new reclaimer/storage lifecycle means the retained recordings do not exercise the complete Could you approve the fork workflows on I can rebuild that exact head and repeat the actual Canvas/Docker before-and-after comparison against upstream parent |
3 participants
HUMAN:
I use Docker runtimes, and I wanted conversations running in containers to respect the title model I selected the same way local mode does, without giving the container access to my full settings directory. The issue was that the container only received the profile name, not the actual profile, so it could not find it and would fall back to the agent model or a shortened version of the first message for the title. This fix copies only the selected title profile into the conversation's private profile store when it is created and encrypts it with that runtime's own key, without sharing any other host profiles or adding new API fields. I reviewed the Docker runtime changes and the before and after results, and the Docker runtime test suite passes with 58 tests. I built this with help from an AI coding agent and did a second review pass that caught and fixed a restart case where profiles could be replaced or added. The testing used a fake local model server and fake credentials rather than a real provider, and the ACP case used a stand-in instead of a real Codex or Claude agent.
AGENT:
Why
Docker conversations previously received the selected title profile's name without its definition in the inner runtime's private profile store. Auto-titling therefore fell back to the agent model or message truncation. The runtime needs the selected profile and its resolved credentials while preserving per-conversation isolation and the existing title fallback.
Summary
6f8c38d00e0468ac138447be905f07a8a0cd7af8into parenta8ea83a200113577fd55339593b55e2e60097f39. Replace the superseded mediation implementation with the reviewed provisioning/router change; no deleted launch APIs are restored. Only provisioning.py, routers.py and test_routes.py differ from that main revision outside temporary evidence.Implementation patch SHA-256:
4e7147038dbc30d84b085407129f118ecceb1e54d82c8285605357e8778b8a27. Integration commit:3cae388cec3a2d23b8d0d135d7d769426dbad4b1; tree:ce253e10decab81f920b0568992e9354a5dc7176.Issue Number
Fixes #5310 — Docker title profile propagation.
How to Test
From the repository root at the published integration commit:
The recorded repository Docker-runtime result is 79 passed. The recorded total of 81 passed additionally includes two unchanged local reviewer probes, which are not part of the repository suite or the public commands above. The permanent cancellation regression can be selected with:
Local integration verification recorded on October 6, not rerun for this description-only correction: reversing only the cancellation correction caused 3 expected failures, and restoring it returned all 79 repository tests plus both local reviewer probes to passing. The Linux image run passed 237 tests. Compatibility tests passed 28; persisted-settings checks validated 23 fixtures and 8 released payloads; OpenAPI/RuntimeIdentity schemas equal the pinned main revision; applicable hooks including Pyright passed. Counts overlap.
Broader affected suites had 2,593 passed and 8 failed, with the identical eight failures reproduced on the clean pinned base: seven Canvas-extension cases requiring Linux artifacts on macOS and one macOS Unix-socket path-length case. The socket case passes on Linux. The broad suite is not green.
For the live workflow, use isolated settings and a container-reachable auxiliary endpoint, select the title profile in Canvas, create a new Docker-backed conversation, send a harmless message, wait for the primary reply and reload to inspect the persisted title. Corroborate the selected profile/model with runtime/provider observations and encryption/isolation checks. The retained trials and their environment limits are below; no provider access is required by the unit-test commands above.
Video/Screenshots
Canvas before/after captures and verification notes · Sanitized provenance. These records were prepared before publication; the verified source and build-input equivalence below binds them to the published integration.
Screenshots: OpenHands baseline, OpenHands corrected, ACP baseline, ACP corrected, selected title profile.
Actual Canvas baseline recordings from October 5 and corrected recordings from October 6 are retained, not newly recorded during integration. All 1,835 tracked files outside .pr match the reviewed tree. During integration verification, inspection of the retained corrected image matched 522 copied build inputs, 502 installed Python files and seven actual imported modules. Corrected OpenHands conversation
f97ca3e8-379b-43b3-b281-fd06585d7e20and ACP conversation81618155-7e18-4f6e-be15-a3d529c626fbvisibly use “Aux profile title 5310”; provider/model observations, runtime logs and profile isolation/encryption checks agree. Image IDsha256:d91892f2fbc4430a965504bb58e8bfe97bfed8388df3172c64dc8c667a11b8ac; Canvas0ab2137ad529f2f5935edf152d25a3f8f5f59293(1.25.0); SDK/server/client 1.53.0. The image still carries the reviewed base's build metadata; it was not rebuilt from the integration commit.Design Doc
The linked evidence README describes the provisioning ownership, encryption boundary and start/delete locking. No separate design document or public API/schema change is introduced.
Type
Notes
The endpoint/credentials are synthetic. ACPAgent uses an actual stdio process running Canvas's mock backend, not authenticated Codex/Claude or full ACP coverage. No paid provider calls, private credentials or production conversations. Maintainers must decide whether this bounded evidence meets acceptance. Full repository, opt-in stress/acp_live, Windows/amd64, PyInstaller, TypeScript CI and real-provider matrices were not run here. Retained Canvas's Node 22 versus >=24 engine warning and unrelated automation 404s remain disclosed.
The earlier 58-test claim in the HUMAN section and prior September/October 1 Docker results are historical; this AGENT section reports the current integration evidence without editing the HUMAN text. Independent correction re-review separately passed 81 tests and did not independently rerun the broader/live checks. AI coding assistance was used for implementation, verification and this description update.
Local verification, retained recordings and remote CI/review are separate. The results above describe the recorded local integration checks; current-head remote CI outcomes and maintainer review must be assessed independently. No claim is made that remote CI is green or that maintainer review is complete.