fix(scripts): honor SPECKIT_PYTHON override for preset manifest parsing - #4445
chelsealong wants to merge 10 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.
mnriem
added
the
triage-nice-to-have
There was a problem hiding this comment.
🟡 Changes recommended
Multiple moderate portability and manifest-parsing issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends SPECKIT_PYTHON support to preset manifest parsing across all script variants.
Changes:
- Selects PyYAML-capable override interpreters in Bash and PowerShell.
- Delegates YAML parsing from Python when needed.
- Adds override and fallback parity tests.
File summaries
| File | Review |
|---|---|
tests/test_resolve_template_python_parity.py |
Moderate: both tests use POSIX-only venv paths; baseline also fails to unset SPECKIT_PYTHON. |
scripts/python/common.py |
Moderate: JSON serialization rejects valid YAML values such as timestamps. |
scripts/powershell/common.ps1 |
Override selection reviewed without additional findings. |
scripts/bash/common.sh |
Moderate: caller argument splitting breaks interpreter paths containing spaces. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- 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.
|
Addressed all 5 findings from the Copilot review:
Verified both new tests fail on the pre-fix code ( |
There was a problem hiding this comment.
🟡 Changes recommended
Bash 3.2 incompatibility and delegated YAML transport/validation regressions remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/python/common.py:412
text=Trueuses the process locale for the pipe. Under the repository's tested ASCII-locale configuration, a valid manifest containing non-ASCII metadata raisesUnicodeEncodeErrorbefore the delegated interpreter can parse it. Force UTF-8 for both the parent pipe and the child Python stdio.
scripts/bash/common.sh:520
- This second
mapfilecall also breaks under the supported macOS system Bash 3.2, so preset lookup cannot use the selected interpreter there. Use the Bash-3-compatiblewhile readarray population while retaining each emitted command argument as a single element.
mapfile -t python_cmd < <(_python3_command)
scripts/bash/common.sh:640
mapfilerequires Bash 4+, butshis the default on macOS where/bin/bashis 3.2. Consequently composed preset resolution still fails on the environment reported by #4443 even whenSPECKIT_PYTHONis valid; use the portablewhile readform here as well.
mapfile -t python_cmd < <(_python3_command)
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
|
The re-review and CI both flag a real regression: your |
…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.
|
Addressed both points from the latest review:
Added three regression tests to |
There was a problem hiding this comment.
🟡 Changes recommended
Delegated parsing rejects some valid YAML structures, leaks child tracebacks, and one regression test does not prove override selection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_resolve_template_python_parity.py:766
- This assertion can pass even if the spaced override is ignored, because PATH still contains a working Python with PyYAML and the resolver silently falls back to it. Also, a venv created with
--system-site-packagesfrom the test venv does not necessarily inherit packages installed in that parent venv. Assert that the spaced interpreter imports PyYAML and that_python3_commandactually selects it (or make every PATH fallback PyYAML-less) so the regression test fails when whitespace splitting returns.
scripts/python/common.py:433
- The delegated transport cannot serialize every value that
yaml.safe_loadaccepts. In particular,json.dump(..., default=...)never appliesdefaultto mapping keys, so an otherwise ignored metadata mapping such asmetadata: {2026-09-08: value}raisesTypeErrorhere, while the in-process parser accepts the same valid YAML and resolves the template. Preserve non-JSON-native mapping keys with a tagged recursive encoding (or move manifest extraction/validation into the child) so delegation does not change which manifests resolve.
"json.dump(yaml.safe_load(sys.stdin.read()), sys.stdout, default=_default)",
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
…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.
|
Addressed both points from the latest Copilot review:
Confirmed both new regression tests fail on the pre-fix code and pass after. Full suite: |
|
Thanks @chelsealong — confirmed both are handled: the child now emits the concise |
There was a problem hiding this comment.
🟡 Changes recommended
Delegated parsing can reject valid recursive YAML, and interpreter and test portability guards remain incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/python/common.py:486
- The Python twin accepts any executable that can
import yaml, unlike the Bash/PowerShell twins and the established override contract. IfSPECKIT_PYTHONresolves to a Python 2 environment with PyYAML, the probe succeeds and the later child program fails with syntax/runtime errors instead of treating the override as unusable. Include the major-version check in this probe.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
… 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.
|
Addressed both points from the latest Copilot review, plus the failing CI:
Added |
There was a problem hiding this comment.
🟡 Changes recommended
Delegated parsing incorrectly treats repeated YAML aliases as recursive cycles and can reject valid manifests.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
|
Thanks @chelsealong — the latest changes address the recursive-metadata example and remove the symlink dependency from the spaced-path test. One parsing-parity issue remains in Please preserve non-recursive shared aliases while preventing recursive traversal, and add that case to the parity coverage. Please also complete the disclosure with the specific agent/tool and model(s) used. Autonomous operation and extent are already stated, but “Claude/Anthropic” doesn’t identify the model. Drafted for @mnriem by GitHub Copilot (model: GPT-6 Astra). |
mnriem
added
author-needs-disclosure
… 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.
|
Addressed the shared-alias regression from the latest review:
Also completed the AI disclosure in the PR body with the specific tool/model for this revision. |
mnriem
removed
author-needs-disclosure
There was a problem hiding this comment.
🟡 Changes recommended
The no-PyYAML tests inherit PYTHONPATH, making their isolation and delegated-path coverage unreliable.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
…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.
|
Addressed the PYTHONPATH isolation issue from the latest Copilot review:
Full suite: |
There was a problem hiding this comment.
🔵 Needs a closer look
Delegated parsing still rejects valid !!pairs or !!omap metadata containing nested non-JSON mapping keys.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/python/common.py:436
- PyYAML's safe loader represents
!!pairsand!!omapas lists of tuples. Because tuples are returned unchanged here, a valid ignored metadata value such as an ordered pair containing a mapping with a timestamp key reachesjson.dumpunnormalized and raisesTypeError; the in-process parser accepts and ignores the same metadata. Include tuples in the recursive normalization (and cover this delegated-parsing case).
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
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.
|
Addressed the latest review: Added |
There was a problem hiding this comment.
🔵 Needs a closer look
The shared test environment leaks the new override, and manifest-less presets perform unnecessary interpreter probes.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
scripts/python/common.py:515
resolve_template_content()calls this function once per preset, and this line now launches aSPECKIT_PYTHONprobe whenever the current interpreter lacks PyYAML—even when the preset has no manifest and only the conventional fallback is needed. This adds one unnecessary interpreter startup (or up to the 10-second timeout) per manifest-less preset. Short-circuit those presets before importing/probing YAML.
tests/parity_helpers.py:85clean_env()now supports tests for this override but still inherits an ambientSPECKIT_PYTHON. That makes tests such astest_all_variants_fail_when_yaml_parser_is_unavailableenvironment-dependent: an externally configured interpreter with PyYAML bypasses the blocked/default interpreter and makes the expected failure succeed. Remove the override here; tests that exercise it already set it explicitly.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…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.
|
Addressed both points from the latest Copilot review:
Added |
Summary
Fixes #4443.
Preset composition raises
PyYAML is required to resolve preset template compositionwhenever the resolve/setup scripts are invoked under a barepython3that lacks PyYAML, while auv tool install/pipx-isolatedCLI venv (which does have PyYAML, per
pyproject.toml) is invisible tothat bare interpreter. This happens both when the bash/PowerShell twins
shell out to Python to parse a
preset.ymlmanifest, and when the Pythontwin (
resolve_template.py/setup_plan.py/ etc., viacommon.py)tries to
import yamldirectly in its own process.SPECKIT_PYTHONis already the established override for exactly this classof problem elsewhere in the codebase (see
extensions/agent-context/scripts/bash/update-agent-context.shand itstests). This PR extends that same override to the three
template-resolution twins:
scripts/bash/common.sh:_python3_command()now prefersSPECKIT_PYTHON(verified to be a working Python 3 interpreter that also has PyYAML)
before falling back to
python3/python/py -3on PATH.scripts/powershell/common.ps1:Get-Python3Commanddoes the same.scripts/python/common.py: since this twin runs in-process rather thanshelling out,
_preset_template_layernow triesimport yamlfirst and,if that fails, delegates manifest parsing to
SPECKIT_PYTHONvia a smallsubprocess-based
safe_load/YAMLErrorshim (_import_yaml/_DelegatedYAML) so the rest of the function's logic is unchanged.No dependencies, lockfiles, or CI config were touched —
pyyamlwas alreadya declared dependency; this only fixes which interpreter picks it up.
Update: an earlier revision of this PR let the bash/PowerShell twins
adopt
SPECKIT_PYTHONas soon as it resolved to any Python 3 interpreter,without checking it actually had PyYAML — unlike the precedent this PR
cites, which validates both. That meant a
SPECKIT_PYTHONset to a validbut PyYAML-less interpreter (e.g. pinned for an unrelated reason) would
shadow a working
python3on PATH and turn previously-succeeding presetcomposition into a
PyYAML is requiredfailure. Both_python3_command()and
Get-Python3Commandnow also probeimport yamlbefore acceptingSPECKIT_PYTHON, matchingupdate-agent-context.sh's candidate-validationloop; if the probe fails they fall through to the existing
python3/python/py -3chain exactly as ifSPECKIT_PYTHONwere unset.Test plan
Added
test_all_variants_honor_speckit_python_override_when_yaml_missingtotests/test_resolve_template_python_parity.py. It creates a realPyYAML-less interpreter (
python -m venv --without-pip) and:python3and noSPECKIT_PYTHONset — asserts both fail (reproduces[Bug] Preset composition fails under isolated installs: scripts run on system python3 but PyYAML lives in the CLI venv #4443), then
SPECKIT_PYTHONto the interpreter that has PyYAML — asserts bothsucceed and produce the correct composed template content.
(PowerShell is included in the same assertions when
pwshis available.)Also added
test_all_variants_fall_back_when_speckit_python_lacks_pyyaml,covering the regression an earlier revision introduced: it sets
SPECKIT_PYTHONto a real Python-3 interpreter without PyYAML whileleaving PATH's own
python3(which has PyYAML) untouched, and assertscomposition still succeeds via that PATH fallback across the bash and
PowerShell twins.
Confirmed both tests fail without their respective fixes:
test_all_variants_honor_speckit_python_override_when_yaml_missing: withgit checkout HEAD~2 -- scripts/bash/common.sh scripts/powershell/common.ps1 scripts/python/common.py, fails withassert Falseon the override-succeeds assertion.test_all_variants_fall_back_when_speckit_python_lacks_pyyaml: with onlythe latest commit's two files reverted (
git checkout HEAD~1 -- scripts/bash/common.sh scripts/powershell/common.ps1, i.e. the state theindependent reviewer blocked), fails the same way — reproducing the
reviewer's exact regression scenario.
Restored the fix in both cases and reran; both pass.
AI disclosure
This PR was written by Claude Code, Anthropic's agentic coding CLI, based on
the issue's description and existing
SPECKIT_PYTHONprecedent in thecodebase. An independent reviewer agent blocked the first revision for
letting the bash/PowerShell twins honor
SPECKIT_PYTHONwithout checkingit actually had PyYAML (a regression relative to the cited precedent); that
revision and subsequent review-response revisions fixed that gap and the
issues raised in later rounds, each adding a regression test reproducing
the specific issue and rerunning the full test suite, shellcheck, and ruff
before pushing.
The most recent revision (preserving non-recursive shared YAML aliases in
_stringify_keyswhile still catching true cycles) was generated by ClaudeCode running Claude Sonnet 5 (model ID
claude-sonnet-5). Earlier revisionsin this thread were also produced with Claude Code; the specific underlying
Claude model used for each of those earlier revisions was not recorded at
the time.