Named dockview layouts: blueprint + Docker cross-env name-collision fix#2435
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joshalbrecht
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
joshalbrecht
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
joshalbrecht
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
… prefix _find_container_by_name matched containers by the host-name/provider labels alone, which carry no environment discriminator: two envs that differ only in MNGR_PREFIX (e.g. two minds envs) label their containers identically, so creating a host reusing a name from another env failed with a misleading 'container already exists' error naming a container that does not exist. Require the container's actual name to match <prefix><host-name> -- the same uniqueness scope Docker enforces and the same prefix filter _list_containers applies to discovery. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joshalbrecht
changed the title
joshalbrecht
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
Problem: libs/mngr/changelog/mngr-add-dockview-profiles.md described the _find_container_by_name fix only as a create-time collision-check fix, but the same helper is get_host's by-name fast path, which previously could resolve a host name to a running same-named container from a different MNGR_PREFIX environment. Fix: add a sentence noting that host resolution by name is also scoped to the current environment now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joshalbrecht
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
joshalbrecht
had a problem deploying
to
minds-ci-test
GitHub Actions
Failure
Neither is related to this branch's changes; both bit after main was merged in. test_extras_claude_plugin_subcommand invokes the real 'extras claude-plugin' subcommand, which shells out to the 'claude' Node CLI to read plugin status. That process's startup dominates the runtime and can exceed the global 10s pytest-timeout on a contended offload sandbox (observed: 'Failed: Timeout (>10.0s)'), though it runs in well under a second locally. Exercising the real subcommand is the point of the test, so the shell-out cannot be removed; give it the same per-test headroom that test_extras_no_args_shows_status already carries for this exact cause. test_snapshot_create_then_list_on_modal boots a real Modal host and sporadically fails reading the SSH banner while sshd comes up on the fresh container -- Modal boot timing, not a defect here. Marked known-flaky so offload retries it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: the branch marks test_snapshot_create_then_list_on_modal flaky in libs/mngr_modal, which makes mngr_modal a touched project, but no libs/mngr_modal/changelog/<branch>.md entry existed -- the CI changelog gate (scripts/check_changelog_entries) fails with exit 1. The change was instead described in libs/mngr's entry, which would have consolidated a mngr_modal test change into libs/mngr/CHANGELOG.md. Fix: add libs/mngr_modal/changelog/mngr-add-dockview-profiles.md describing the flaky marking, and scope libs/mngr's entry to the mngr-only test change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joshalbrecht
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
Problem: the host container name format `<prefix><host-name>` was re-derived inline at three sites in providers/docker/instance.py -- the new name-scoped lookup in _find_container_by_name, the create path, and the snapshot-restore path. That format is now load-bearing (it is what scopes a host name to one mngr environment for both the create-time collision check and host resolution by name), so drift at any creation site would silently break lookups with no single place pinning the contract. Fix: add host_container_name(prefix, host_name) to providers/docker/volume.py next to the existing state_container_name helper, document the invariant there, and use it at all three sites plus the docker_sdk test that mirrors production naming. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: the new host_container_name helper in
libs/mngr/imbue/mngr/providers/docker/volume.py declared its host-name
parameter as a bare str, even though every call site passes a HostName and
the lookup it backs (_find_container_by_name) is already domain-typed.
Fix: annotate the parameter as HostName (a validated SafeName subclass, which
is what makes the derived container name legal for Docker) and pass
HostName("discoverable") at the one test call site that used a raw literal.
joshalbrecht
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
Problem: _create_test_container in test_docker_integration.py gained a container_name parameter, but its docstring still claimed unconditionally that the container name carries the MNGR prefix "so that _list_containers can find it during cleanup". A container_name outside the prefix is invisible to the docker_provider fixture's prefix-based teardown, so a future caller could silently leak a container into the shared CI Docker daemon. Fix: scope the prefix/cleanup claim to the default and state that a caller passing a foreign-prefix container_name must remove the container itself (as the cross-environment regression test already does). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem: the comment above the _find_container_by_name call in DockerProviderInstance.get_host still called it a "container label lookup", but the lookup is no longer label-only -- it now also requires the container's name to be this environment's <prefix><host-name>, which is the whole point of the cross-environment fix on this branch. Fix: reword the comment to say the fast path finds this environment's container for that host name. Comment-only change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
joshalbrecht
temporarily deployed
to
minds-ci-test
GitHub Actions
Inactive
Contributor
Author
|
lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two things on this branch:
Blueprint for named dockview layouts at
blueprint/dockview-named-layouts/plan-dockview-named-layouts.md. The implementation lives in Add named dockview layouts with per-client selection and agent targeting default-workspace-template#257 (same branch name there).Bug fix (libs/mngr): the Docker provider's create-time host-name collision check (
_find_container_by_name) matched containers by thehost-name/providerlabels alone. Labels carry no environment discriminator, so two mngr environments that differ only inMNGR_PREFIX(e.g. two minds envs) label their containers identically — creating a host that reused a name from another env failed with a misleading "container already exists" error naming a container that doesn't exist (found while testing the layouts feature: a staging-env workspace nameddocker-1was blocked by a dev-env containerminds-dev-josh-1-docker-1). The lookup now also requires the container's actual name to equal<prefix><host-name>— the same uniqueness scope Docker itself enforces and the same prefix filter_list_containersalready applies to discovery. Containers are never renamed after creation (mngr renameonly updates the host record), so name and label stay in lockstep.Includes a regression test (
test_find_container_by_name_ignores_other_environments) and verification against the real colliding containers: with the staging prefix the lookup now returns None while the dev-env prefix still finds its own container.🤖 Generated with Claude Code