fix: convert #subdirectory= fragment to path component for local path…#163
fix: convert #subdirectory= fragment to path component for local path…#163johannao76 wants to merge 2 commits intomicrosoft:mainfrom
Conversation
… overrides When _build_include_source_resolver fires and the override is a local filesystem path (not a git+https:// or other URL), the #subdirectory=foo/bar fragment from the original git URL is meaningless as a file path. The file loader tries to open the literal string including #subdirectory=, fails to find it, and throws 'File not found'. Fix: detect when override is a local path, and convert #subdirectory=behaviors/foo.yaml -> /behaviors/foo.yaml instead of appending it as a URL fragment. Reproducer: amplifier source add superpowers ~/src/amplifier-bundle-superpowers then start a session that includes superpowers-methodology.yaml via git URL.
Adds scripts/smoke-test.sh to test the fix for #subdirectory= fragment handling in _build_include_source_resolver. The script: - Launches a clean Docker container with amplifier installed - Overrides amplifier-app-cli with the local checkout - Runs 4 unit tests for the resolver fix - Runs a session smoke test end-to-end All tests passing.
There was a problem hiding this comment.
Pull request overview
This PR fixes bundle include source overrides so that when an override is a local filesystem path, any #subdirectory=... fragment from the original git URL is converted into a path component (instead of being appended as a literal fragment that breaks file loading).
Changes:
- Update
_build_include_source_resolverto translate#subdirectory=...into/<subdirectory>for local path overrides. - Add a Docker-based smoke test script that installs Amplifier, overrides
amplifier-app-cliwith the local checkout, runs targeted checks, and performs a short session run.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
amplifier_app_cli/lib/bundle_loader/prepare.py |
Adjusts include override resolution to convert #subdirectory= into a local path segment. |
scripts/smoke-test.sh |
Adds a containerized smoke test workflow to validate the fix and run a basic session. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| is_local = not (override.startswith("git+") or "://" in override) | ||
| if is_local and fragment.startswith("subdirectory="): | ||
| subdir = fragment.removeprefix("subdirectory=") | ||
| return f"{override}/{subdir}" | ||
| return f"{override}#{fragment}" |
There was a problem hiding this comment.
This behavior change will break the existing pytest coverage for _build_include_source_resolver (e.g. the local-path fragment preservation assertion). Please update/add unit tests under tests/lib/bundle_loader/test_prepare.py to reflect the new local-path #subdirectory= conversion (the new smoke-test script won’t help CI).
| docker exec "$CONTAINER_NAME" bash -c " | ||
| apt-get update -qq && apt-get install -y -qq git >/dev/null 2>&1 | ||
| pip install -q uv | ||
| echo 'Bootstrap OK' | ||
| " || fail "Bootstrap failed" |
There was a problem hiding this comment.
The commands executed inside the container don’t enable set -e/pipefail, so failures in apt-get/pip install can be masked (the last echo still succeeds). Add strict mode inside the bash -c script (or chain with &&) so the smoke test reliably fails when bootstrap fails.
| export PATH=/root/.local/bin:\$PATH | ||
| uv tool install git+https://github.com/microsoft/amplifier@main 2>&1 | tail -3 |
There was a problem hiding this comment.
uv tool install ... | tail -3 can hide errors (and without pipefail the pipeline can still exit 0). Prefer keeping full output or explicitly checking the install command’s exit status; at minimum enable set -o pipefail in the container command.
| export PATH=/root/.local/bin:\$PATH | |
| uv tool install git+https://github.com/microsoft/amplifier@main 2>&1 | tail -3 | |
| set -euo pipefail | |
| export PATH=/root/.local/bin:\$PATH | |
| uv tool install git+https://github.com/microsoft/amplifier@main |
| SMOKE_OUTPUT=$(docker exec "$CONTAINER_NAME" bash -c " | ||
| export PATH=/root/.local/bin:\$PATH | ||
| timeout 120 amplifier run '$SMOKE_PROMPT' 2>&1 | ||
| ") || SMOKE_EXIT=$? |
There was a problem hiding this comment.
timeout 120 amplifier run '$SMOKE_PROMPT' is vulnerable to quoting/injection issues if SMOKE_PROMPT contains a single quote (or shell metacharacters). Pass the prompt into the container via an env var and use proper shell escaping/"$SMOKE_PROMPT" inside the container, or avoid bash -c and pass args directly to docker exec.
| SMOKE_OUTPUT=$(docker exec "$CONTAINER_NAME" bash -c " | |
| export PATH=/root/.local/bin:\$PATH | |
| timeout 120 amplifier run '$SMOKE_PROMPT' 2>&1 | |
| ") || SMOKE_EXIT=$? | |
| SMOKE_OUTPUT=$(docker exec \ | |
| -e SMOKE_PROMPT="$SMOKE_PROMPT" \ | |
| "$CONTAINER_NAME" \ | |
| bash -c ' | |
| export PATH=/root/.local/bin:$PATH | |
| timeout 120 amplifier run "$SMOKE_PROMPT" 2>&1 | |
| ') || SMOKE_EXIT=$? |
| # -> "/local/path/superpowers" | ||
|
|
||
| # Fragment preservation: | ||
| # Fragment preservation — git URL override keeps the fragment: |
There was a problem hiding this comment.
Docstring example/comment is internally inconsistent: it says “git URL override keeps the fragment” but the example override is a local path and the output shows subdirectory being converted into a path component. Please update the wording/example so it matches the actual behavior for local vs git overrides.
| # Fragment preservation — git URL override keeps the fragment: | |
| # Local path override — git subdirectory fragment becomes a path component: |
| # For local path overrides, #subdirectory=path is a git-only | ||
| # convention and is not meaningful as a file path. Convert it | ||
| # to a path component instead. | ||
| is_local = not (override.startswith("git+") or "://" in override) | ||
| if is_local and fragment.startswith("subdirectory="): |
There was a problem hiding this comment.
is_local detection here differs from the project’s existing is_local_path() helper (and will treat file://... as non-local). It also misclassifies scp-style git URLs (e.g. git@github.com:org/repo) as “local”. Consider reusing a shared local-path detector and ensure file:// overrides get the same subdirectory→path handling when appropriate.
| if is_local and fragment.startswith("subdirectory="): | ||
| subdir = fragment.removeprefix("subdirectory=") | ||
| return f"{override}/{subdir}" |
There was a problem hiding this comment.
Building the local override path via f"{override}/{subdir}" is not OS-safe (Windows paths/UNC) and allows subdir values like ../... or absolute paths to escape the override directory. Use path-aware joining/normalization and reject/contain path traversal before returning the resolved path.
When _build_include_source_resolver fires and the override is a local filesystem path (not a git+https:// or other URL), the #subdirectory=foo/bar fragment from the original git URL is meaningless as a file path. The file loader tries to open the literal string including #subdirectory=, fails to find it, and throws 'File not found'.
Fix: detect when override is a local path, and convert #subdirectory=behaviors/foo.yaml -> /behaviors/foo.yaml instead of appending it as a URL fragment.
Reproducer:
amplifier source add superpowers ~/src/amplifier-bundle-superpowersthen start an amplifier session.Error:
