fix(docker): expand ~ and $VAR in extra_mounts destinations - #128
Open
bai-uipath wants to merge 4 commits into
Open
fix(docker): expand ~ and $VAR in extra_mounts destinations#128bai-uipath wants to merge 4 commits into
bai-uipath wants to merge 4 commits into
Conversation
The source side of an extra_mounts spec is normalized with expandvars(expanduser(...)) so authors can write portable specs; the destination was only checked for a leading "/" and never expanded. That asymmetry makes one common mount impossible to write portably. env_passthrough forwards HOME with the HOST value on purpose, so any container-side path that must line up with $HOME -- $HOME/.uipath for the uip CLI's saved login state, for instance -- has a different literal value on every host. The only way to express it was to hardcode one host's home directory, which then mounts to the wrong place everywhere else. A login state the CLI cannot see fails tasks as a capability problem rather than a config one, so the misconfiguration is close to invisible: it cost 26% of the rows in an ad-hoc Maestro run before it was spotted. Expand the destination the same way, before the absolute-path check, so `~/.uipath:$HOME/.uipath:rw` resolves. Two details worth keeping: - expandvars leaves an unset variable verbatim, so a typo'd name still fails the absolute-path check. The message now shows the raw and the expanded form, otherwise it reads as a puzzle. - the framework-owned-mount check runs on the expanded destination, since a variable could itself expand to /work or / and the raw form would sail past the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bai-uipath
added a commit
to UiPath/skills
that referenced
this pull request
Aug 22, 2026
The preview experiment shipped its login mount commented out with a `$HOME/.uipath` destination, blocked on destination-side `$VAR` expansion (UiPath/coder_eval#128). It doesn't need it: nightly.yaml and smoke.yaml already authenticate the in-container `uip` from `~/.uipath:/.uipath:rw`, a literal destination that validates on the pinned coder_eval 0.10.2. Adopt that mount verbatim rather than deriving a "more correct" one. It is the arrangement with a 500-task/night track record, and a login the CLI cannot see fails tasks on their tenant calls, which scores as a capability problem rather than a config error. flow-v2-preflight.sh mounts the same destination, so its "uip reports a live login" check is now the empirical test of the unified path. same-ground-headtohead.yaml gets the same treatment, dropping both `/home/tmatup/...` hardcodes and the comment deferring them to UiPath/coder_eval#100. One `$VAR` destination survives, on the repo-root mount, and it is unrelated to auth: DockerDriverConfig can forward host env vars but cannot set container ones, so criteria see `$SKILLS_REPO_PATH` with its forwarded host value and the mount has to land there. That single line is what still wants coder_eval#128. Renames make the Flow v2 scope legible: tests/experiments/preview-maestro-sdk.yaml -> flow-v2-preview.yaml tests/docker/preflight.sh -> flow-v2-preflight.sh same-ground-headtohead.yaml keeps its name: per tests/README.md it is a general campaign comparison arm (pinned skills-image:sg1, SG_EMPTY_SKILLS blanking installed skills), not the v1 half of a Flow v2 head-to-head. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The added docstring and test docstrings justified destination expansion with `$HOME`/`.uipath`, which is not a case this serves: skills experiments mount login state at a literal destination. Restate it against the one real consumer, a container path that has to match a host-valued var. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems. `ruff format` wanted the destination error on one line, which failed the Quality Gate and the Windows Smoke Test. More importantly, a variable whose value carries a ':' added fields to the spec rebuilt at the bottom of the validator. `SNEAKY=/mnt/x:rw` in `$real:$SNEAKY:ro` produced `/real:/mnt/x:rw:ro`, moving the destination and widening a declared read-only mount. Guard both sides after expansion, excluding the Windows drive prefix whose colon is legitimate and already split off. Two tests cover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bai-uipath
marked this pull request as ready for review
August 22, 2026 04:03
bai-uipath
requested review from
akshaylive,
tmatup and
uipreliga
as code owners
August 22, 2026 04:03
|
Claude finished @bai-uipath's task in 1m 45s —— View job Code Review in Progress
|
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.
What
Expand
~and$VARon the destination side of anextra_mountsspec, the way the source side already is. The destination was only checked for a leading/.Closes #100.
Why
An experiment that points
plugins.pathbelow the repo root loses the automatic repo-root bind mount that came with it, and has to re-add the root by hand. That mount's destination has to be the host repo path: criteria shell out to$SKILLS_REPO_PATH/tests/tasks/**/_shared/*.py, andSKILLS_REPO_PATHis forwarded into the container with its host value.DockerDriverConfigforwards host env vars but cannot set container ones, so it cannot be pointed at a fixed path instead. Without this, the only way to write that mount is to hardcode one machine's path.Sole consumer today is one line in
flow-v2-preview.yaml(UiPath/skills#2728).Notes
expandvarsleaves an unset variable verbatim, so a typo'd name still fails the absolute-path check. The message shows the raw and the expanded form./workor/.:is rejected.SNEAKY=/mnt/x:rwin$src:$SNEAKY:rowould otherwise rebuild as/src:/mnt/x:rw:roand silently widen a declared read-only mount.Six tests cover destination
$VAR, destination~, the unset-variable rejection, a variable expanding to a reserved destination, and colon injection on each side.Verified with UiPath/skills#2728
Same spec, same shell: released 0.10.2 →
ValueError: destination must be an absolute path; this branch →/Users/bai.li/uipath/skills:/Users/bai.li/uipath/skills:ro. The expanded mount reachesdocker runand the criteria tree is readable at the host path inside the container. End to end underflow-v2-preview.yaml, taskuipath-maestro-flow/connector_features/path_params.yamlon codex /gpt-5.6-luna: SUCCESS, 6/6 criteria at 1.0. Without this the experiment aborts on load, before any container starts.One correction to #100, which changes its reasoning but not the outcome. Its problem statement says a
/.exampledestination is portable "only when the forwardedHOMEis/". In that container the forwardedHOMEwas/Users/bai.li, anduip login statusstill returned"Status": "Logged in"from a mount at/.uipath, with.authpresent only there and not under$HOME/.uipath. Souipdoes not resolve its login throughHOME, and #100's worked example is not a case this change is needed for. Its acceptance criteria are met regardless, which is why the keyword stands: a host-valued destination without an embedded username, resolved absolute, reserved destinations still rejected after expansion, expansion unable to inject fields or modes, and tests for non-root forwarded homes and unknown placeholders.