Fix/4443 yaml runtime fallback - #4674
Open
EnocFlores wants to merge 12 commits into
Open
EnocFlores wants to merge 12 commits into
EnocFlores wants to merge 12 commits into
Conversation
Preset composition raises "PyYAML is required" whenever the resolve/setup scripts run under a bare `python3` that lacks PyYAML but a `uv tool install`/`pipx`-isolated CLI venv has it. SPECKIT_PYTHON is already the established override for this class of problem (see the agent-context extension's update-agent-context.sh); extend it to the bash, PowerShell, and Python template-resolution twins so a user (or the CLI, in future) can point scripts at an interpreter that actually has PyYAML. Fixes github#4443
…owerShell _python3_command() (bash) and Get-Python3Command (PowerShell) accepted SPECKIT_PYTHON as soon as it resolved to any Python 3 interpreter, unlike their cited precedent (update-agent-context.sh) which also verifies `import yaml` succeeds before trusting it. That let a SPECKIT_PYTHON without PyYAML shadow a PATH python3 that has it, turning previously working preset composition into a "PyYAML is required" failure. Gate the SPECKIT_PYTHON branch on a successful `import yaml` in both twins so it's only preferred when it can actually serve manifest parsing; otherwise the existing python3/python/py fallback chain runs unchanged, matching pre-fix behavior for that case.
- bash: preserve SPECKIT_PYTHON as one argv element instead of splitting it on whitespace, which broke paths containing spaces. - python: stringify non-JSON-native YAML values (e.g. unquoted dates) in the delegated-YAML subprocess instead of crashing json.dump. - tests: select venv paths by os.name so the SPECKIT_PYTHON parity tests work on Windows, and explicitly unset SPECKIT_PYTHON from the baseline env so a pre-set override can't mask the baseline failure.
…types across delegation mapfile is a Bash 4+ builtin unavailable on macOS's system Bash 3.2, so preset resolution failed there even with a valid SPECKIT_PYTHON. The delegated-YAML subprocess also used the process locale for its pipe (raising under an ASCII locale on non-ASCII metadata) and stringified every non-JSON-native YAML value indiscriminately, letting a validated field like `file` silently pass an isinstance(str) check that native parsing would correctly reject.
…backs json.dump's default= hook never applies to dict keys, so a manifest mapping with a non-JSON-native key (e.g. an unquoted date) raised TypeError under SPECKIT_PYTHON delegation even though the in-process parser accepts it; stringify non-native keys before dumping. Also catch yaml.YAMLError in the delegated child so a malformed manifest reports a concise message instead of leaking the child's raw traceback into TemplateResolutionError. Fixes the spaced-path regression test relying on --system-site-packages inheritance and not proving the override was actually selected.
… 3 in override probe
- scripts/python/common.py: _stringify_keys now tracks visited container
ids so a self-referential YAML alias (e.g. metadata: &m {self: *m}, which
yaml.safe_load supports natively) no longer causes RecursionError during
SPECKIT_PYTHON delegation. _import_yaml's probe now also checks
sys.version_info.major == 3, matching the bash/PowerShell twins, so a
SPECKIT_PYTHON pointing at a PyYAML-equipped Python 2 is rejected instead
of being accepted and failing later in the child process.
- tests/test_resolve_template_python_parity.py: fixed
test_bash_honors_speckit_python_path_containing_spaces, which was failing
in CI (PyYAML is required to resolve preset template composition) because
symlinking sys.executable from outside a venv's own directory breaks
Python's pyvenv.cfg discovery, hiding the venv's site-packages. Replaced
the symlink with a shim script that execs sys.executable directly, which
keeps discovery working. Added regression tests for the two fixes above.
… parsing _stringify_keys marked every container id as seen for the rest of the document instead of only while its own subtree was being walked, so a second, unrelated reference to the same anchor (e.g. a templates list anchored under metadata and reused under provides.templates) was wrongly treated as a cycle and replaced with the non-native marker. Track an active recursion stack and discard each id once its subtree finishes so only true self-references are caught.
…lated A --without-pip venv still honors an inherited PYTHONPATH, so the SPECKIT_PYTHON parity tests could silently pass without exercising the delegated-YAML path if PYTHONPATH leaked a directory containing PyYAML. clean_env() now strips it, and a new make_yaml_less_venv() helper asserts the created interpreter actually can't import yaml.
PyYAML's safe loader represents !!omap/!!pairs entries as tuples, which _stringify_keys left untouched, so a nested non-JSON-native mapping key inside one still reached json.dump unstringified and raised TypeError even though the in-process parser accepts and ignores the same metadata.
…KIT_PYTHON in tests _preset_template_layer() now checks for preset.yml before importing/probing PyYAML, avoiding an unnecessary interpreter probe (and potential SPECKIT_PYTHON child-process spawn) per manifest-less preset in resolve_template_content(). clean_env() also strips an ambient SPECKIT_PYTHON so tests that don't set it explicitly get a true baseline.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The Python resolver repeats interpreter probing and isolated uv execution for every manifest-bearing preset.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds resilient PyYAML discovery and fallback across generated template resolvers, addressing isolated CLI installations.
Changes:
- Adds
SPECKIT_PYTHONand isolated uv fallback support. - Preserves cross-runtime argument, encoding, and YAML parsing behavior.
- Adds extensive regression and parity coverage.
Review fully AI-drafted by GitHub Copilot (model: unknown, autonomous) on behalf of the requester.
| File | Description |
|---|---|
scripts/bash/common.sh |
Adds argv-safe YAML runtime selection. |
scripts/powershell/common.ps1 |
Adds YAML fallback and environment isolation. |
scripts/powershell/resolve-template.ps1 |
Produces concise resolver errors. |
scripts/python/common.py |
Implements delegated YAML parsing and uv fallback. |
tests/parity_helpers.py |
Adds isolated runtime test helpers. |
tests/test_resolve_template_python_parity.py |
Covers fallback, parity, and failure paths. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
EnocFlores
force-pushed
the
fix/4443-yaml-runtime-fallback
branch
from
September 22, 2026 15:50
8677fb1 to
4079265
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new !!omap regression test fails on Windows because its assertion does not normalize CRLF line endings.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (1)
Comment on lines
+1072
to
+1076
| assert result.returncode == 0, result.stderr | ||
| assert json_stdout(result) == { | ||
| "TEMPLATE_NAME": TEMPLATE, | ||
| "TEMPLATE_CONTENT": expected, | ||
| } |
mnriem
added
the
triage-nice-to-have
EnocFlores
marked this pull request as ready for review
September 22, 2026 17:53
This branch has not been deployed
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
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.
Description
Fixes #4443.
This draft incorporates and extends the implementation from #4445 by @chelsealong. That PR established the
SPECKIT_PYTHONoverride and delegated YAML parsing behavior; this PR preserves that work and adds an automatic fallback for isolated installations.When no available Python candidate can import PyYAML, generated Bash, PowerShell, and Python resolvers now use the argv-safe, pinned fallback:
uv run --isolated --no-project --with pyyaml==6.0.3 pythonThe implementation:
SPECKIT_PYTHON,python3,python, andpy -3precedence.PYTHONPATHand enablesPYTHONSAFEPATH=1for the isolated uv runtime.shell=True.!!omap, and!!pairs.PowerShell-specific validation also fixed scalar unrolling of the
py -3command and verbose top-level exception formatting.Testing
Validation completed locally:
7720 passed, 16 skipped60 passed39 passed, 1 skippeduv run specify --help: passedThe exact
uv sync && uv run pytestcommand does not install the optional test dependencies and therefore could not spawnpytest. The repository-supported command passed:uv sync --extra test && uv run pytestTemplate SHA-256 values:
spec-template.md:3945437fc35cd30a5b2bf7beea680337c3516826d3efa5a6b92c4a7eca1ba28eplan-template.md:dfba43da2b2a207ff8c177a0fba03af074fd45e261696b4b64172041fad2e992checklist-template.md:c65b7f10aa9a965ea9c7f4e879d4ab0240665473731c5e9e638c4acb3ca9afc4Version:
1.0.5.dev0Current head:
5091adb8e7a475a09bcba566a5d7bb36f5b297fbuv run specify --helpAI Disclosure
AI disclosure: This branch incorporates #4445 by @chelsealong, whose Claude Code assistance is disclosed in that PR. The isolated uv fallback, additional tests, review remediation, and validation were assisted by OpenCode (model: gpt-5.6-sol, human-supervised).