Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 6958a85ef5a8fb4e280fbd6a07b0264502e000aa against PR base 365cbc242be59305b5a6714b2ad810a713edbb54 and live main@16c405657640abd4536680e80656192f9d20840e (main is one unrelated attribution/mailmap commit ahead).
The normal path is a real improvement: profile_name_from_session_key() derives the request profile, profile config owns its Docker shared key, and the new secondary-profile tests correctly reject fallback to the supervisor's process-wide key.
P1 — fail closed when request-profile / multiplex authority is indeterminate.
There are still two exception paths that restore the exact ambient/default authority this PR is removing:
-
gateway/platforms/base.py::_docker_sandbox_dir_candidates(): if importing or calling the profile helpers raises,profile_nameremainsNone, theexcept Exceptionbranch adds_active_docker_shared_key(), and the followingprofile_name in (None, "", "default")branch adds"default"._default_docker_workspace_host_roots()then materializes every candidate as a real<sandbox>/<candidate>/workspacehost root. A secondary-profile request can therefore search the process/default workspace precisely when profile resolution becomes indeterminate. -
tools/terminal_tool.py::docker_shared_container_key_for_profile(): ifis_multiplex_gateway_enabled()raises, the function setsmultiplex = False;if not multiplex or name == active_profile:then returns process-wideHERMES_DOCKER_SHARED_CONTAINER_KEYfor any requested profile, including a secondarycoderprofile.
That violates the explicit #95470 contract that secondary profiles must not inherit the default environment fallback. Normal-path precedence is not enough here: failure to establish request ownership cannot expand access to process/default state.
Required before merge:
- use process-env /
defaultfallback only when the requested profile is positively established as the active/default process profile; - fail closed for secondary profiles when profile or multiplex resolution throws, rather than converting an indeterminate authority check into ambient ownership;
- narrow/remove the broad catches on this security-critical path where possible;
- add negative regressions for (a)
is_multiplex_gateway_enabled()raising whilecoderis requested and the default env key is populated, and (b) profile/key resolution raising for anagent:coder:...MEDIA lookup; neither case may produce the active/default shared key ordefaultworkspace root. An end-to-end same-name MEDIA fixture (file exists only in default workspace) would pin the actual read boundary especially well.
The graph around this is coherent but needs explicit reconciliation. #94633 by @fangliquanflq is the merged multiplex substrate this corrects, and itself preserved @teknium1's #92419 lineage. #92344 by @fangliquanflq is complementary shared-container profile isolation and overlaps tools/terminal_tool.py; it is currently conflicted against main, so whichever lands first needs an explicit rebase/reconciliation path that preserves its per-profile shared-key persistence/migration semantics and credit. #91326 by @shuowang is the useful other side of the shape: cross-profile proxy sharing is allowed only by explicit opt-in, which reinforces that Docker/MEDIA sharing cannot reappear through an ambient fallback. #93943 is the broader complementary authority law, not a duplicate implementation.
Exact-head evidence: CI run 32959369547 is red in the test job; exact-head Docker 32959368794 and Nix 32959368779 are green. I do not have a reliable failed-job log receipt from the connector, so I am not attributing that CI failure to this patch. Please rerun exact-head CI after the authority-failure repair.
This is close: the successful profile-key path is the right repair. Carry the same isolation rule through the indeterminate/error paths and this becomes a coherent class fix.
andrexibiza
left a comment
There was a problem hiding this comment.
Topology correction from the final graph read-back: the reviewed commit's actual parent / current merge-base is 8e9459c97f707047be5915a5c8b4c503756daa9b, not 365cbc242be59305b5a6714b2ad810a713edbb54. The PR metadata endpoint returned the latter as base_sha, which I repeated in the primary review, but direct commit ancestry / compare is the authoritative topology receipt.
Live main has since advanced to d22e2b9f6ecd4b0efde00a426e4f40896b7598ae; the exact reviewed head remains unchanged and is now 1 ahead / 14 behind main. None of that changes the P1 finding above; the repaired object should be restacked on landing main and rerun exact-head.
6958a85 to
13a7a0b
Compare
|
The exception paths were the leftover hole. Happy path is what I meant. The names in the review are not in this patch ( Closed both on the latest push. Process env / I did not drop Restacked on current main with the fail-closed fix. |
13a7a0b to
90183aa
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking re-review on exact head 90183aa0eeacb3ca22b317f10e7ba571a6dd1bbb.
The cross-profile MEDIA authority boundary is still open. _docker_sandbox_dir_candidates('agent:coder:…') now prioritizes coder but still unconditionally appends default; resolve_outbound_file() returns the first existing candidate. If coder has no foo.png but ~/.hermes/sandboxes/default/workspace/foo.png exists, coder-routed outbound attachment can still resolve the default profile’s file. That is same-name cross-profile leakage, not migration compatibility.
test_sandbox_candidates_key_lookup_error_does_not_use_default_shared_key proves only that the active default shared-container key is not consulted; it does not forbid the local default workspace root. The submitted behavior therefore still widens a known routed profile into an unowned fallback namespace.
Known routed profiles need fail-closed resolution: routed profile/shared scope plus only legacy paths whose ownership is provably the same session/profile — never an unowned default. Add deterministic witnesses: default has relative file + coder does not => reject/None; coder has its own copy => coder copy wins; key-lookup exception remains profile-safe.
Exact-head Docker 32962558054 and Nix 32962558077 are green, but CI 32962560215 is red: JS & TS checks / UI Tests (1/3) job 98158120518 failed and aggregate All required checks pass job 98158810868 failed. This exact head is not ready to close.
…ofiles Under multiplex, os.environ holds the default profile's TERMINAL_DOCKER_SHARED_CONTAINER_KEY. A secondary profile with no key in its own config.yaml was joining that container anyway. MEDIA lookup also used the process profile, so agent:coder:... files missed profile:coder. Read terminal.docker_shared_container_key from that profile's config. If multiplex is on and the file has no key, stay isolated. If the multiplex probe or a MEDIA helper throws, a secondary profile still does not inherit the process env key or collapse onto default.
A routed agent:coder:... session still listed the unowned default workspace as a MEDIA candidate. First-existing-file lookup then returned the default profile's foo.png when coder had no copy. Named profiles now search only their own profile/shared sandbox plus the same-session legacy session: path. default stays for the default profile and CLI.
90183aa to
f1c7382
Compare
|
Dropped the unowned
Tests: default has the file and coder doesn't, so lookup returns nothing. Coder has its own copy, so that copy wins. Key-lookup throw still stays on |
|
Thanks for this PR. Merged via #101242 (4a7f228) on current main — routed multiplex profiles get their own terminal cwd/backend/docker config; container boot honors config multiplex_profiles. #101242 won as the consolidated fix because it covers the whole multiplex-profile bug class in one change (with tests) rather than the single symptom addressed here; this PR is superseded by it. If anything from your original change is still missing on main >= 4a7f228, please open a fresh PR/issue against main and tag it. Thanks again. |
What does this PR do?
#94633 lets trusted profiles share one persistent Docker container via
terminal.docker_shared_container_key. Under multiplex,os.environholds the default profile._resolve_container_task_idread that env for every session, so a secondary profile with no key in its ownconfig.yamlstill joined the default container.MEDIA lookup had the same leak.
_docker_sandbox_dir_candidatesusedget_active_profile_name()(the gateway process) and the same env key. Delivery runs after the turn's session contextvars are gone, soagent:coder:...files were looked up in the default sandbox.This reads
terminal.docker_shared_container_keyfrom the named profile'sconfig.yaml. If multiplex is on and that file has no key, the profile stays isolated. MEDIA candidates take the profile from the session key (agent:coder:...→profile:coderfirst). Single-profile CLI/gateway still uses the process env bridge.Related Issue
Fixes #95470
Type of Change
Changes Made
tools/terminal_tool.py:docker_shared_container_key_for_profilereads the named profile'sconfig.yaml. Multiplex miss does not fall through toos.environ._resolve_container_task_iduses that helper.gateway/platforms/base.py:_docker_sandbox_dir_candidatesparses the profile fromagent:<profile>:...and uses the same helper.tests/tools/test_shared_container_task_id.py: multiplex leak, secondary-profile key, and MEDIA candidate order.How to Test
scripts/run_tests.sh tests/tools/test_shared_container_task_id.py -qTERMINAL_DOCKER_SHARED_CONTAINER_KEY=team/workspacein the process env, profileworkwith an empty key in itsconfig.yaml._resolve_container_task_idmust returnprofile:work, notshared:team/workspace.docker_shared_container_key: work-labin work's config. Result must beshared:work-lab._docker_sandbox_dir_candidates('agent:coder:telegram:dm:1')must start withprofile:coder, notdefault.Ran on Windows 11 with Python 3.11: 26 passed in
tests/tools/test_shared_container_task_id.py.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs