Address Copilot Round-7 review comments on PR #2483:
1. record_existing(recovered=False) and record_file now BOTH discard the
path from _recovered_files. The marker is meant to flag "we observed
this file but cannot vouch it's a managed baseline" — once the same
path is re-recorded as managed (either explicitly or by writing fresh
bytes), the marker is stale and must clear so refresh_managed and
future is_recovered queries return the truthful answer.
2. is_recovered now applies the same canonical-key guard as record_existing
(rejects absolute paths and '..' segments lexically before delegating
to _validate_rel_path). Such paths can never be stored keys, so the
query correctly returns False without depending on _validate_rel_path
semantics that diverged from record_existing's stricter contract.
record_file docstring updated to mention the side-effect on recovered
markers.
Tests: 3 new methods in TestManifestRecoveredFiles covering
record_existing(false) clearing, record_file clearing, and is_recovered
dotdot rejection.
Address Copilot review comment id 4309888722 round-5 (2026-05-21) on PR #2483:
``is_recovered()`` previously checked ``self._recovered_files`` membership
with bare ``Path(rel).as_posix()``, while ``record_existing()`` stores keys
via ``_validate_rel_path(rel, root).relative_to(root).as_posix()``. The two
normalizations disagreed on absolute paths and paths that escape the
project root — ``is_recovered`` would silently return False for inputs that
``record_existing`` would have refused entirely.
The fix routes ``is_recovered`` through the same ``_validate_rel_path``
pipeline; ``ValueError`` from the validator is caught and converted to
False so query semantics stay exception-free (Python ``__contains__``
convention).
Tests: 2 new methods in ``TestManifestRecoveredFiles``:
- ``test_is_recovered_absolute_path_returns_false``
- ``test_is_recovered_escaping_path_returns_false``
🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).
Address Copilot review id 4309888722 (2026-05-18) on PR #2483:
1. Recovery semantics (shared_infra.py:371, 412) — install_shared_infra
now passes ``recovered=True`` when re-recording a skipped existing
file. This flag funnels into a new ``recovered_files`` array in the
manifest JSON, so a future ``refresh_managed`` run can distinguish
"hash I produced" from "hash I observed on a file that may be a user
customization" and avoid silent overwrite without ``--refresh-shared-infra``.
Schema is purely additive: ``files: dict[str, str]`` is unchanged; the
new ``recovered_files: list[str]`` is omitted when empty.
2. Symlinked ancestor (manifest.py:172) — ``record_existing`` now walks
every component of the rel path and rejects any symlinked ancestor,
not just a symlinked leaf. Catches ``linked_dir/file.txt`` where
``linked_dir`` is a symlink, which previously slipped past the leaf-only
``is_symlink()`` check and was resolved through by ``_validate_rel_path``.
Mirrors the component-walk pattern in ``_ensure_safe_manifest_directory``.
3. Misleading "escapes project root" message (manifest.py:168) — paths
like ``dir/../file.txt`` normalize inside the project, so the old
message lied about what was wrong. New message: "Manifest paths must
be canonical; '..' segments are not allowed". Still rejects (canonical
keys are required so ``check_modified``/``uninstall`` cannot key the
same file under two paths).
Tests: 7 new test methods across TestManifestRecoveredFiles and
TestRecordExistingNewGuards covering all 4 Copilot findings. Full suite
passes locally.
🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).
Address Copilot review (2026-05-11, review id 4266902103):
1. `record_existing` was calling `(self.project_root / rel).is_symlink()`
BEFORE validating containment. For absolute paths or paths containing
`..`, this performed a filesystem stat outside the project root before
`_validate_rel_path()` raised. Add a cheap lexical pre-check that
delegates to `_validate_rel_path()` for the canonical error messages,
so the symlink stat only ever runs on paths that are already lexically
inside the project root.
2. Add focused unit tests in `tests/integrations/test_manifest.py` for
the symlink and non-regular-file error paths, including:
- symlink target rejection
- dangling symlink rejection (caught by the symlink guard before
the is_file check)
- directory path rejection (is_file == False)
- missing-path rejection (is_file == False)
- absolute-path lexical pre-check
The Copilot reviewer noted these guards had no focused coverage in
`test_manifest.py`, only via the `test_integration_claude.py`
regression test.
3. The third Copilot finding (repeated `dict(self._files)` copies via
`manifest.files` in the skip branches) is already resolved on this
branch by using `prior_hashes` — the function-scope snapshot taken at
the top of `install_shared_infra` — for the membership check, instead
of `manifest.files`.
AI disclosure: drafted with assistance from Claude (Opus 4.7).
Address Copilot review feedback on PR #2483. The previous fix called
``manifest.record_existing(rel_skip)`` from the skip branch of both
loops in ``install_shared_infra``, which would crash with
``IsADirectoryError`` (or another ``OSError``) if a directory or other
non-regular-file happened to exist at the expected destination path —
since ``record_existing`` opens the file to compute its SHA-256.
Three coordinated fixes:
1. ``IntegrationManifest.record_existing`` now validates its
precondition: it raises ``ValueError`` if the path is a symlink or
is not a regular file. The docstring already promised "an
already-existing file"; this enforces it. The symlink check runs on
the un-resolved path because ``_validate_rel_path`` calls
``resolve()``, which would silently follow the symlink. Mirrors the
existing ``_ensure_safe_manifest_destination`` precedent in the
same module.
2. In ``install_shared_infra``'s scripts and templates skip branches,
guard the ``record_existing`` call with ``dst.is_file()`` and wrap
it in ``try/except (OSError, ValueError)``. A directory collision,
permission error, or TOCTOU race no longer aborts the whole
install — the user gets a per-path warning, the path still
surfaces in ``skipped_files``, and the rest of the install
continues.
3. ``_read_manifest_files`` in the regression test no longer falls
back to ``data.get("_files")`` (Copilot's low-confidence finding):
the silent fallback could mask a schema regression where the
public ``files`` key is renamed. It now asserts ``"files" in data``
and that the value is a dict.
Add two regression tests in ``TestSpeckitManifestRecordsSkippedFiles``
covering the directory-at-destination edge case for both the scripts
loop and the templates loop. Both verify (a) install does not crash,
(b) the non-file path is not recorded in the manifest, and (c) the
path still surfaces in the user-visible warning.
The "shared infrastructure file(s)" warning text is changed to
"path(s)" so it remains accurate when non-file entries appear in the
list.
Refs #2107
`install_shared_infra` skipped files that already existed on disk
when `force=False`, but the skip branches in both the scripts loop
and the templates loop only appended to `skipped_files` without
calling `manifest.record_existing`. So when the function ran with a
fresh manifest against an already-populated `.specify/` tree (e.g.
after the manifest was deleted, corrupted, or extracted out of band),
every file went down the skip path, `planned_copies` /
`planned_templates` stayed empty, and `manifest.save()` wrote an
empty `files` field — leaving the integration believing nothing was
installed.
Record every skipped file in the manifest, but only when it is not
already tracked. This preserves the original hash for files that
were previously recorded so `check_modified()` (used by
`integration use` to decide whether a user has customized a
template) keeps working correctly.
Add `TestSpeckitManifestRecordsSkippedFiles` in
`tests/integrations/test_integration_claude.py` covering both the
fresh-skip path and the recover-after-lost-manifest path.
Fixes#2107
* refactor: extract _assets.py and _utils.py from __init__.py
Move bundle path resolution and version lookup into _assets.py (stdlib only,
zero internal imports), and system utilities (subprocess, tool detection,
file operations) into _utils.py (imports only from ._console). Re-export all
moved symbols from __init__.py for backward compatibility. Update
test_check_tool.py to patch both specify_cli and specify_cli._utils namespaces
since constants are now defined in _utils.
* style: apply PR-1 review patterns to _assets.py and _utils.py
- Add module docstring to _assets.py (stdlib-only, zero internal imports)
- Add blank line after `from __future__ import annotations` in both files
- Replace `Optional[X]` with `X | None` throughout _utils.py (PEP 604)
- Remove unused `Optional` import from _utils.py
- Use explicit re-export form (`X as X`) for public symbols in __init__.py
- Remove unused `subprocess` and `tempfile` imports from __init__.py (moved to _utils.py)
* fix(opencode): use commands/ directory (plural) to match OpenCode docs
OpenCode documentation (https://opencode.ai/docs/commands/) uses
.opencode/commands/ (plural) as the canonical command directory.
The OpenCode runtime supports both .opencode/command/ and
.opencode/commands/ via a {command,commands} glob, but the
singular form was the original convention and is now outdated.
Update the OpenCode integration to write to .opencode/commands/
instead of .opencode/command/, aligning with the documented
standard and the OpenSpec fix (Fission-AI/OpenSpec#748).
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
Assisted-by: OpenCode (claude-opus-4-6)
* feat(registrar): add legacy_dir fallback for backward-compatible directory migration
Add _resolve_agent_dir() to CommandRegistrar that checks a
legacy_dir fallback when the canonical directory does not exist.
When legacy_dir is found, a deprecation warning directs users to
run "specify integration upgrade" to migrate.
The OpenCode integration declares legacy_dir: ".opencode/command"
so that extension and preset registration, as well as command
cleanup, continue working for projects that have not yet migrated
to .opencode/commands/.
The legacy_dir mechanism is opt-in: integrations that do not
declare it get no fallback and no behavioral change.
Add end-to-end test verifying that "specify integration upgrade
opencode" migrates commands from legacy .opencode/command/ to
canonical .opencode/commands/ and removes stale files.
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
Assisted-by: OpenCode (claude-opus-4-6)
* fix(registrar): address PR review feedback on legacy_dir handling
- Fix deprecation warning formatting: quote paths and remove trailing
'/.' that produced confusing '.opencode/commands/.' output
- Eliminate duplicate warnings: pass pre-resolved directory to
register_commands() via _resolved_dir parameter so
_resolve_agent_dir() is only called once per agent
- Fix unregister_commands() to clean both canonical and legacy dirs
when both exist, preventing orphaned command files after upgrade
- Add test_unregister_cleans_legacy_when_both_dirs_exist regression
test and tighten warning count assertion to exactly 1
Assisted-by: OpenCode (claude-opus-4-6)
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
---------
Signed-off-by: Marcus Burghardt <maburgha@redhat.com>
* refactor: extract _console.py from __init__.py
Move Rich UI primitives (BANNER, TAGLINE, StepTracker, get_key,
select_with_arrows, console, BannerGroup, show_banner) into a new
src/specify_cli/_console.py module. Re-export all symbols from
__init__.py to preserve the public API. Add regression guard tests.
* refactor(console): improve type annotations and add guard for empty options
- Add module-level docstring documenting the console layer's purpose and
the dependency-layering rule (no imports from other specify_cli modules)
- Tighten select_with_arrows() signature: options typed as dict[str, str]
and default_key as str | None to align with repo typing style
- Add early ValueError guard when options is empty, preventing downstream
ZeroDivisionError / IndexError inside the Live loop
* refactor(console): improve type safety and code quality in _console.py
- Add Callable import from collections.abc for precise callback typing
- Annotate StepTracker._refresh_cb as Callable[[], None] | None
- Add parameter/return types to attach_refresh()
- Use explicit keyword form typer.Exit(code=1) across all error exits
- Add blank line between StepTracker class and get_key() (PEP 8)
- Add regression test for select_with_arrows() raising ValueError on
empty options dict
* style(cli): add __all__ declaration to fix Ruff F401 lint warnings
- Add explicit __all__ for intentional re-exports (BANNER, TAGLINE, get_key)
- Prevent F401 unused import errors in CI lint checks
- Maintain backward compatibility for external imports
* Preserve public console imports
The CLI package intentionally re-exports console helpers for compatibility, so __all__ must track that public surface instead of narrowing star imports to a partial set.
Constraint: Existing tests import console helpers directly from specify_cli
Rejected: Remove __all__ entirely | keeping an explicit export list documents the intended compatibility surface
Confidence: high
Scope-risk: narrow
Directive: Keep __all__ synchronized when adding or removing specify_cli public re-exports
Tested: uv run pytest tests/test_console_imports.py -q
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* style(cli): use explicit re-export syntax to fix ruff F401 warnings
Use `X as X` form for BANNER, TAGLINE, and get_key imports
to mark them as intentional public re-exports and silence
ruff F401 lint errors.
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* fix(kiro-cli): replace literal $ARGUMENTS with prose fallback
Kiro CLI file-based prompts do not natively substitute any
argument placeholder (kirodotdev/Kiro#4141, kiro.dev/docs/cli
manage-prompts), so the literal "$ARGUMENTS" set in
KiroCliIntegration.registrar_config["args"] reached the model
verbatim and broke the prompt — every parameterized SpecKit
command under Kiro CLI was unusable.
Replace the placeholder with a prose fallback that instructs
the model to take its argument from the user's next message,
mirroring the convention used by other integrations whose
target CLI lacks native argument injection.
Add two regression tests in TestKiroCliIntegration:
- test_rendered_prompts_do_not_contain_raw_arguments
- test_rendered_prompts_contain_kiro_arg_placeholder
and override the inherited test_registrar_config so it does
not require args == "$ARGUMENTS".
Fixes#1926
* test(kiro-cli): tighten args regression guard + document quirk
Address review feedback on PR #2482.
Two changes that bracket the original bug fix from both sides — code AND
documentation:
1. Test layer (Copilot finding at lines 27, 56)
The previous test_registrar_config asserted only that args != "$ARGUMENTS"
and that args is truthy. That would silently pass if a future change
swapped $ARGUMENTS for $INPUT, {{userMessage}}, <args>, or any other
unsubstituted placeholder syntax — defeating the regression guard for
issue #1926.
Replace with a dual-layer guard:
- test_registrar_config_args_is_exact_prose_fallback pins args to the
imported _KIRO_ARG_FALLBACK constant. Wording drift now requires a
deliberate paired commit (production constant + test).
- test_registrar_config_args_does_not_look_like_a_placeholder_token is
an independent regression guard built on a 7-pattern regex set
covering Bash ($X, ${X}, ${X:-default}), Mustache/Handlebars/Jinja
({{X}}, {{{X}}}), Liquid/Jinja control ({% %}), Python str.format /
.NET ({0}, {var}), angle-bracket (<X>), and Windows (%X%). Patterns
are anchored to the full string so legitimate prose mentioning a
placeholder ("the {{magic}} of placeholders") is not flagged.
Also fix the line-56 tautology by importing _KIRO_ARG_FALLBACK directly
into test_rendered_prompts_contain_kiro_arg_placeholder, instead of
reading the constant back from registrar_config["args"]. The test now
verifies the FALLBACK STRING reaches the rendered output, independent
of the integration's own config staying correct.
2. Docs layer (mnriem CHANGES_REQUESTED)
The Kiro CLI row in docs/reference/integrations.md only documented its
alias. Update the notes column to lead with the limitation — Kiro CLI
does not substitute $ARGUMENTS in file-based prompts, so Spec Kit ships
a prose fallback at render time — with inline links to upstream Kiro
"Manage prompts" docs and issue #1926. Style follows the Pi row
("limitation first, alias preserved at end").
Refs #1926
* test(presets): silence expected UserWarnings in self-test composition tests
The self-test preset that ships with the repo provides a wrap-strategy
command (speckit.wrap-test) intentionally without a corresponding core
base layer, exercising the 'no base layer' branch of
_reconcile_composed_commands().
Eighteen tests across TestSelfTestPreset and TestPresetSkills install
this preset and trigger an expected UserWarning. Running the suite with
-W error::UserWarning surfaces them as test noise that could obscure
unrelated warnings.
Add class-level pytest.mark.filterwarnings filters to acknowledge the
two known messages ('Cannot compose command speckit.wrap-test' and
'Post-install reconciliation failed for self-test') so other UserWarning
sources still propagate normally.
Fixes#2363
* test(presets): scope filterwarnings to UserWarning category
Address Copilot review on #2373: the previous filterwarnings entries
omitted the warning category, so any warning class with a matching
message would have been silenced. Append :UserWarning to the four
filters so only the deliberately-emitted UserWarnings from
_reconcile_composed_commands() are ignored.
* test(presets): narrow self-test warning filter to install helper only
Address Copilot feedback: the class-level @pytest.mark.filterwarnings on
TestPresetSkills was too broad. The 'Post-install reconciliation failed'
filter could mask real reconciliation regressions, since that warning is
only emitted when _reconcile_composed_commands/_reconcile_skills raises.
Tests in TestPresetSkills already call install_self_test_preset(), which
scopes a narrow filter to the expected wrap-strategy 'Cannot compose'
warning. The class-level filters are redundant for those calls and unsafe
elsewhere, so they are removed.
* test(presets): align TestSelfTestPreset docstring with helper-based filtering
Address Copilot feedback: docstring referred to 'filters above', but the
fix uses warnings.filterwarnings inside install_self_test_preset rather
than class-level decorators. Updated the docstring to describe the actual
mechanism.
* test(presets): remove extra blank line between helper and class (PEP 8)
Address Copilot feedback: PEP 8 expects two blank lines between top-level
definitions; reduce the three blank lines between install_self_test_preset
and TestSelfTestPreset to two.
* fix(integration): refresh shared infra on integration switch
* fix(integration): address Copilot review on switch shared-infra refresh
- Clarify install_shared_infra docstring: force overwrites regular files
but always preserves symlinks (safe-destination check refuses to follow).
- Print refresh_hint only for preserved_user_files; skipped_files keeps
the generic remediation. Avoids misleading guidance when files were
merely skipped (not detected as customized).
- Catch ValueError from the safe-destination check and bucket the path
under a new symlinked_files warning instead of aborting the switch.
- Restore templates/constitution-template.md to upstream (drop accidental
leading blank lines).
* fix(integration): narrow symlink bucketing to dedicated exception
Address Copilot feedback on shared_infra.py:305 — _safe_dest_or_bucket
caught any ValueError as 'symlinked', which masked genuine safety errors
(path escape, parent-not-a-directory).
- Introduce SymlinkedSharedPathError(ValueError) raised only by the
symlink-specific branches in _ensure_safe_shared_*().
- _safe_dest_or_bucket() now catches only SymlinkedSharedPathError;
other ValueErrors propagate so the operation aborts with the real
cause instead of being silently bucketed.
- Wrap top-level dest_scripts/dest_variant/dest_templates mkdir calls
in the same bucket helper so a symlinked .specify/scripts or
.specify/templates is preserved with a warning rather than aborting
the switch (matches the documented 'preserve customizations' behavior).
- Update tests to expect the new bucket+warn behavior for leaf-level
symlinked destinations.
* fix(integration): tailor shared-infra warnings and rename preflight test
Address Copilot review on PR #2375:
- skipped_files hint now uses refresh_hint when refresh_managed=True
so integration switch suggests --refresh-shared-infra instead of the
generic init/upgrade flags.
- symlinked-files warning header says "path(s)" rather than "file(s)"
since symlinked directories (e.g. .specify/scripts/bash) are also
bucketed there.
- Rename test_shared_infra_install_preflights_before_writing to
test_shared_infra_install_buckets_unsafe_destinations_and_continues
to match the new bucket-and-continue semantics.
* test: rename symlink bucketing tests to reflect bucket-and-continue behavior
The two file-bucketing tests at line 300/320 were named *_refuses_*, but
the new behavior buckets symlinked file destinations with a warning while
safe destinations in the same install still complete. Rename to
*_buckets_* and update docstrings to match.
The remaining *_refuses_* tests (line 342/362/381) genuinely raise on
symlinked dirs/manifests and keep their names.
---------
Co-authored-by: Quratulain-bilal <quratulain.bilal@users.noreply.github.com>
* add lingma support
* fix
* fix context file
* Update CONTEXT_FILE path in test integration
* fix IntegrationOption.default
* fix IntegrationOption.defaultfix
* fix: address Copilot review feedback
- Add blank line after __future__ import (PEP 8)
- Remove trailing whitespace at end of lingma/__init__.py
- Bump integrations/catalog.json updated_at timestamp
- Add Lingma to supported agent list in README.md
* fix: address Copilot review feedback (round 4)
- Reword module docstring: Lingma is a brand-new skills-only integration
with no prior command-mode history, so 'deprecated since v0.5.1'
wording (copied from Trae) was misleading
- Remove Lingma from README CLI-tool check list: Lingma is IDE-based
(requires_cli=False) and is explicitly skipped by specify init /
specify check tool detection
* fix(forge): use hyphen notation for command refs in Forge integration
- Add invoke_separator = "-" class attribute to ForgeIntegration so
effective_invoke_separator() returns "-" for shared-template installs
- Add "invoke_separator": "-" to ForgeIntegration.registrar_config so
agents.py CommandRegistrar can resolve refs with the correct separator
- Pass invoke_separator to process_template() in ForgeIntegration.setup()
so all .forge/commands/*.md bodies use /speckit-foo notation
- Replace literal /speckit.specify with __SPECKIT_COMMAND_SPECIFY__ in
extensions/git/commands/speckit.git.feature.md so every agent resolves
the reference through its own separator
- Apply resolve_command_refs re.sub in agents.py register_commands() after
argument-placeholder substitution so extension commands registered for
Forge get /speckit-foo refs; all other agents continue to get /speckit.foo
Fixes ZSH compatibility: dot-notation command invocations (/speckit.specify)
are misinterpreted by ZSH as file-path operations; hyphen notation
(/speckit-specify) works correctly in all shells.
* fix(agents): propagate invoke_separator from integration class into AGENT_CONFIGS
Skills-based agents (claude, codex, kimi, …) inherit invoke_separator="-"
from SkillsIntegration but do not repeat it in their registrar_config dicts.
_build_agent_configs() was copying registrar_config verbatim, so
register_commands() fell back to "." when resolving __SPECKIT_COMMAND_*__
tokens for those agents — emitting /speckit.specify instead of the correct
/speckit-specify for extension commands like speckit.git.feature.
Fix: after copying registrar_config, inject invoke_separator from the
integration's class attribute when it is not already declared explicitly.
This makes the integration class the single source of truth for all agents,
without requiring each SkillsIntegration subclass to duplicate the field.
Also replace the inline re.sub in register_commands() with a call to
IntegrationBase.resolve_command_refs() (deferred import to avoid the
existing circular dependency) so token-resolution logic is not duplicated.
Adds two tests in test_agent_config_consistency.py:
- test_skills_agents_have_hyphen_invoke_separator_in_agent_configs: asserts
every /SKILL.md agent has invoke_separator="-" in AGENT_CONFIGS.
- test_skills_agent_command_token_resolves_with_hyphen: end-to-end check via
CommandRegistrar that the git extension's speckit.git.feature command is
installed for Claude with /speckit-specify (not /speckit.specify).
Addresses review comment on PR #2462.
* fix: validate URL scheme in build_github_request
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* test: add missing hostname validation test for build_github_request
* fix: update docstring and fix import grouping per Copilot feedback
* fix: sort imports and simplify url validation in build_github_request
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* fix: migrate extension commands on integration switch
When switching integrations (e.g. kimi → opencode), extension commands
were not re-registered for the new agent, leaving the new agent without
extension support and orphaning files in the old agent's directory.
Changes:
- Add ExtensionManager.unregister_agent_artifacts() to clean up old
agent extension files and registry entries during switch
- Add ExtensionManager.register_enabled_extensions_for_agent() to
re-register all enabled extensions for the new agent
- Wire both into integration_switch() after uninstall/install phases
- Handle skills mode (Copilot --skills) correctly
- Add tests for kimi→opencode→claude migration, Copilot skills mode,
and disabled extension handling
Fixes extension commands not appearing after integration switch.
* Update src/specify_cli/extensions.py
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* feat(integrations): add Devin for Terminal skills-based integration
- Register DevinIntegration as a SkillsIntegration with .devin/skills/ layout
- Add catalog entry, docs row, and supported-agents listing
- Display /speckit-<command> hyphen syntax in init "Next Steps" panel
(matches Claude/Cursor/Copilot skills mode, since Devin invokes skills
by directory name)
Closes#2346
* fix(devin): implement -p non-interactive dispatch; clarify skills comment
Addresses Copilot review on PR #2364:
- Override build_exec_args() in DevinIntegration to emit
'devin -p <prompt> [--model X]' for non-interactive text dispatch
(verified Devin CLI supports -p / --print). Returns None when
output_json=True since Devin has no structured-output flag, so
CommandStep workflows that require JSON cleanly raise
NotImplementedError instead of crashing on an unknown CLI flag.
requires_cli=True is retained for tool detection.
- Extend the skills-integrations enumeration comment in
specify_cli/__init__.py to include copilot and devin so the
comment matches the code below it.
* fix(devin): always return exec args; document plain-text stdout
Addresses third Copilot review comment on PR #2364.
Returning None from build_exec_args() when output_json=True
incorrectly used the codebase's IDE-only sentinel: workflow
CommandStep checks 'impl.build_exec_args("test") is None' to
detect non-dispatchable integrations (test_workflows.py exercises
this with WindsurfIntegration). The previous implementation made
Devin appear non-dispatchable to all command steps even though it
runs fine via 'devin -p'.
Always return the args list. When output_json is requested, Devin
is still dispatched and returns plain-text stdout instead of
structured JSON; the docstring documents this explicitly.
* docs(devin): include claude in skills-integrations enumeration comment
Addresses Copilot review on PR #2364: the comment listing skills
integrations omitted Claude, which is also a SkillsIntegration
subclass. Updated to keep the comment accurate for future readers.
* test(devin): add build_exec_args regression tests; bump catalog updated_at
Addresses Copilot review on PR #2364, per @mnriem's request to
'address the Copilot feedback, especially the testing ask':
- tests/integrations/test_integration_devin.py: add TestDevinBuildExecArgs
with three regression assertions:
* build_exec_args returns args (not the None IDE-only sentinel)
* --output-format is never emitted, regardless of output_json
* --model flag is passed through correctly
- integrations/catalog.json: bump top-level updated_at to reflect the
Devin entry addition so downstream catalog consumers can detect the
change reliably.
* fix(extensions): use explicit UTF-8 encoding when reading manifest YAML
On Windows, Python's open() defaults to the system locale encoding
(e.g., GBK on Chinese Windows), which causes UnicodeDecodeError when
extension.yml or preset.yml contains non-ASCII content such as Chinese
characters in description fields.
Add encoding='utf-8' to ExtensionManifest._load_yaml and
PresetManifest._load_yaml so manifests are read consistently across
platforms.
Fixes#2325
* test(extensions,presets): add UTF-8 manifest regression tests for #2325
Positive: extension.yml/preset.yml with non-ASCII (Chinese + emoji)
descriptions load correctly when written as UTF-8 bytes — fails on
Windows without explicit encoding='utf-8'.
Negative: files containing invalid UTF-8 bytes raise a clean error
(ValidationError or UnicodeDecodeError), not a silent crash.
* fix(extensions,presets): wrap I/O and decode errors as ValidationError
Address remaining Copilot concerns on #2370:
- Catch UnicodeDecodeError and OSError in both manifest loaders and
re-raise as ValidationError / PresetValidationError so callers see a
consistent error type, not a bare decode/IO traceback.
- Validate that PresetManifest YAML root is a mapping (extensions.py
already had this; presets.py was missing it). Treat None as {} for
empty-file compatibility.
- Tighten the negative regression tests to assert the specific message,
and add a non-mapping-root test for PresetManifest matching the
existing one for ExtensionManifest.
* feat(extensions,presets): authenticate GitHub-hosted catalog and download requests with GITHUB_TOKEN/GH_TOKEN
Squashed from #2087 (original author: @anasseth).
Adds GitHub-token authentication to extension and preset catalog fetching
and ZIP downloads so private GitHub repos work when GITHUB_TOKEN/GH_TOKEN
is set, while preventing credential leakage to non-GitHub hosts.
- Introduces shared _github_http module with build_github_request() and
open_github_url() helpers
- Routes ExtensionCatalog and PresetCatalog network calls through
GitHub-auth-aware opener
- Adds comprehensive unit/integration tests for auth header behavior
- Updates user docs for both extensions and presets
Co-authored-by: anasseth <16745089+anasseth@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(auth): address review feedback from #2087
- Fix redirect handler to preserve Authorization on GitHub-to-GitHub
redirects (e.g. github.com → codeload.github.com). The previous
implementation relied on super().redirect_request() which strips
auth on cross-host redirects, breaking private repo archive downloads.
- Add codeload.github.com to documented host lists in both
EXTENSION-USER-GUIDE.md and presets/README.md
- Add redirect auth-preservation and auth-stripping tests
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(auth): use Bearer scheme instead of token for consistency
Aligns with the rest of the codebase (e.g. __init__.py:1721) and
GitHub's current API guidance. Updates all test assertions accordingly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix: address second round of Copilot review feedback
- Fix docstring to say Bearer instead of token (matches implementation)
- Remove unused imports/fixtures from redirect tests (GITHUB_HOSTS,
MagicMock, temp_dir, monkeypatch)
- Replace __import__('io').BytesIO() with normal import io pattern
in test_presets.py
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: anasseth <16745089+anasseth@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* feat(init): deprecate --no-git flag, gate deprecations at v0.10.0
- Add deprecation warning when --no-git is used on specify init
- Update --ai deprecation gate from 1.0.0 to 0.10.0
- Update test expectation for the new version gate
Closes#2167
* fix: address PR review feedback
- Update --no-git deprecation message to reference existing 'specify extension'
commands instead of non-existent --extension flag
- Add test_no_git_emits_deprecation_warning CLI test
* fix: strengthen --no-git deprecation test assertions
Add assertions unique to the --no-git message ('will be removed',
'git extension will no longer be enabled by default') to prevent
false positives from the --ai deprecation panel.
* feat(vibe): migrate to SkillsIntegration and inject user-invocable frontmatter
Switches VibeIntegration from the old prompts-based MarkdownIntegration to SkillsIntegration, adopting the .vibe/skills/speckit-<name>/SKILL.md layout required by Mistral Vibe v2.0.0+. Post-processes each generated SKILL.md to inject `user-invocable: true` so skills are directly callable by users, not just by other agents.
* test(vibe): assert user- invocable: true is present in all generated SKILL.md files
* Update tests/integrations/test_integration_vibe.py
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* fix: resolve command references per integration type (dot vs hyphen)
Replace hardcoded /speckit.<cmd> references in templates with
__SPECKIT_COMMAND_<NAME>__ placeholders that are resolved at
setup time based on the integration type:
- Markdown/TOML/YAML agents: separator='.' → /speckit.plan
- Skills agents: separator='-' → /speckit-plan
Changes:
- Add resolve_command_refs() static method to IntegrationBase
- Add invoke_separator class attribute (. for base, - for skills)
- Wire into process_template() as step 8
- Update _install_shared_infra() to process page templates
- Replace /speckit.* in 5 command templates and 3 page templates
- Add unit tests for resolve_command_refs (positive + negative)
- Add integration tests verifying on-disk content for all agents
- Add end-to-end CLI tests for Claude (skills) and Copilot (markdown)
Fixes#2347
* review: use effective_invoke_separator() for Copilot skills mode
Address PR review feedback: instead of bleeding _skills_mode
knowledge into the CLI layer, add effective_invoke_separator()
method to IntegrationBase that accepts parsed_options.
CopilotIntegration overrides it to return "-" when skills
mode is requested. The CLI layer simply asks the integration
for its separator — no hasattr or _skills_mode coupling.
Also adds tests for the new method on both base and Copilot,
plus an end-to-end test for 'specify init --integration copilot
--integration-options --skills' verifying page templates get
hyphen refs.
* fix: build_command_invocation preserves full suffix for extension commands
Previously rsplit('.', 1)[-1] on 'speckit.git.commit' yielded
just 'commit', producing /speckit.commit instead of
/speckit.git.commit (or /speckit-git-commit for skills).
Fix: strip only the 'speckit.' prefix when present, then join
remaining segments with the appropriate separator.
Updated in IntegrationBase, SkillsIntegration, and
CopilotIntegration. Added tests for extension commands in
build_command_invocation across all three.
* fix: Copilot dispatch_command() preserves full extension command suffix
dispatch_command() had the same rsplit('.', 1)[-1] bug as
build_command_invocation() — speckit.git.commit would dispatch
as /speckit-commit instead of /speckit-git-commit in skills
mode, or --agent speckit.commit instead of speckit.git.commit
in default mode.
xargs re-parses stdin as shell tokens, causing 'unterminated quote'
errors when feature descriptions contain apostrophes, double quotes,
or backslashes. Replace with sed-based whitespace trim that preserves
input verbatim.
Add regression tests for special characters in descriptions (core and
extension scripts), plus a negative test for whitespace-only input.
Fixes#2339
* fix: rebase onto upstream/main, resolve conflicts with PR #2189
upstream/main merged PR #2189 (wrap-only strategy) which overlaps with
our comprehensive composition strategies (prepend/append/wrap). Resolved
conflicts keeping our implementation as source of truth:
- README: keep our future considerations (composition is now fully
implemented, not a future item)
- presets.py: keep our composition architecture (_reconcile_composed_commands,
collect_all_layers, resolve_content) while preserving #2189's
_substitute_core_template which is used by agents.py for skill
generation
- tests: keep both test sets (our composition tests + #2189's wrap
tests), removed TestReplayWrapsForCommand and
TestInstallRemoveWrapLifecycle which test the superseded
_replay_wraps_for_command API; our composition tests cover equivalent
scenarios
- Restored missing _unregister_commands call in remove() that was lost
during #2189 merge
* fix: re-create skill directory in _reconcile_skills after removal
After _unregister_skills removes a skill directory, _register_skills
skips writing because the dir no longer passes the is_dir() check.
Fix by ensuring the skill subdirectory exists before calling
_register_skills so the next winning preset's content gets registered.
Fixes the Claude E2E failure where removing a top-priority override
preset left skill-based agents without any SKILL.md file.
* fix: address twenty-third round of Copilot PR review feedback
- Protect reconciliation in remove(): wrap _reconcile_composed_commands
and _reconcile_skills in try/except so failures emit a warning instead
of leaving the project in an inconsistent state
- Protect reconciliation in install(): same pattern for post-install
reconciliation so partial installs don't lack cleanup
- Inherit scripts/agent_scripts from base frontmatter: when composing
commands, merge scripts and agent_scripts keys from the base command's
frontmatter into the top layer's frontmatter if missing, preventing
composed commands from losing required script references
- Add tier-5 bundled core fallback to collect_all_layers(): check the
bundled core_pack (wheel) or repo-root templates (source checkout) when
.specify/templates/ doesn't contain the core file, matching resolve()'s
tier-5 fallback so composition can always find a base layer
* fix: address twenty-fourth round of Copilot PR review feedback
- Use yaml.safe_load for frontmatter parsing in resolve_content instead
of CommandRegistrar.parse_frontmatter which uses naive find('---',3);
strip strategy key from final frontmatter to prevent leaking internal
composition directives into rendered agent command files
- Filter _reconcile_skills to specific commands: use _FilteredManifest
wrapper so only the commands being reconciled get their skills updated,
preventing accidental overwrites of other commands' skills that may be
owned by higher-priority presets
* fix: address twenty-fifth round of Copilot PR review feedback
- Support legacy command-frontmatter strategy: when preset.yml doesn't
declare a strategy, check the command file's YAML frontmatter for
strategy: wrap as a fallback so legacy wrap presets participate in
composition and multi-preset chaining
- Guard skill dir creation in _reconcile_skills: only re-create the
skill directory if the skill was previously managed (listed in some
preset's registered_skills), avoiding creation of new skill dirs
that _register_skills would normally skip
* fix: add explanatory comment to empty except in legacy frontmatter parsing
* fix: address twenty-sixth round of Copilot PR review feedback
- Unregister stale commands when composition fails: when resolve_content
returns None during reconciliation (base layer removed), unregister
the command from non-skill agents and emit a warning
- Load extension aliases during reconciliation: _register_command_from_path
now checks extension.yml for aliases when the winning layer is an
extension, so alias files are restored after preset removal
- Use line-based fence detection for legacy frontmatter strategy fallback:
scan for --- on its own line instead of split('---',2) to avoid
mis-parsing YAML values containing ---
* fix: address twenty-seventh round of Copilot PR review feedback
- Handle non-preset winners in _reconcile_skills: when the winning
layer is core/extension/project-override, restore skills via
_unregister_skills so skill-based agents stay consistent with the
priority stack
- Update base_frontmatter_text on replace layers: when a higher-priority
replace layer occurs during composition, update both top and base
frontmatter so scripts/agent_scripts inheritance reflects the
effective base beneath the top composed layer
* fix: address twenty-eighth round of Copilot PR review feedback
- Parse only interior lines in _parse_fm_yaml: use lines[1:-1] instead
of filtering all --- lines, preventing corruption when YAML values
contain a line that is exactly ---
- Omit empty frontmatter: skip re-rendering when top_fm is empty dict
to avoid emitting ---/{}/--- for intentionally empty frontmatter
- Update scaffold wrap comment: mention both {CORE_TEMPLATE} and
$CORE_SCRIPT placeholders for templates/commands vs scripts
- Clarify shell composition scope in ARCHITECTURE.md: note that bash/PS1
resolve_template_content only handles templates; command/script
composition is handled by the Python resolver
* fix: address twenty-ninth round of Copilot PR review feedback
- Fix TestCollectAllLayers docstring: reference collect_all_layers()
- Add default/unknown strategy handling in bash/PS1 composition: error
on unrecognized strategy values instead of silently skipping
- Fix comment: .composed/ is a persistent dir, not temporary
- Fix comment: legacy fallback checks all valid strategies, not just wrap
- Cache PresetRegistry in _reconcile_skills: build presets_by_priority
once instead of constructing registry per-command
* fix: address thirtieth round of Copilot PR review feedback
- Guard legacy frontmatter fallback: only check command file frontmatter
for strategy when the manifest entry doesn't explicitly include the
strategy key, preventing override of manifest-declared strategies
- Document rollback limitation: note that mid-registration failures may
leave orphaned agent command files since partial progress isn't
captured by the local vars
* fix: handle project override skills and extension context in reconciliation
* fix: add comment to empty except in extension registration fallback
* fix: filter extension commands in reconciliation and fix type annotation
* fix: filter extension commands from post-install reconciliation
Apply the same extension-installed check used in _register_commands to
the reconciliation command list, preventing reconciliation from
registering commands for extensions that are not installed.
* fix: skip convention fallback for explicit file paths and add stem fallback to tier-5
When a preset manifest provides an explicit file path that does not
exist, skip the convention-based fallback to avoid masking typos.
Also add speckit.<stem> to <stem>.md fallback in tier-5 bundled/source
core lookup for consistency with tier-4.
* fix: scan past non-replace layers to find base in resolve_content
The base-finding scan now skips non-replace layers below a replace
layer instead of stopping at the first non-replace. This fixes the
case where a low-priority append/prepend layer sits below a replace
that should serve as the base for composition.
* fix: add context_note to non-skill agent registration for extensions
Add context_note parameter to register_commands_for_non_skill_agents
and pass extension name/id during reconciliation so rendered command
files preserve the extension-specific context markers.
* fix: Optional type, rollback safety, and override skill restoration
- Fix context_note type to Optional[str]
- Wrap shutil.rmtree in try/except during install rollback
- Separate override-backed skills from core/extension in _reconcile_skills
* fix: align bash/PS1 base-finding with Python resolver
Rewrite bash and PowerShell composition loops to find the effective
base replace layer first (scanning bottom-up, skipping non-replace
layers below it), then compose only from the base upward. This
prevents evaluation of irrelevant lower layers (e.g. a wrap with
no placeholder below a replace) and matches resolve_content behavior.
* fix: PS1 no-python warning, integration hook for override skills, alias cleanup
- Warn when no Python 3 found in PS1 and presets use composition strategies
- Apply post_process_skill_content integration hook when restoring
override-backed skills so agent-specific flags are preserved
- Unregister command aliases alongside primary name when composition
fails to prevent orphaned alias files
* fix: include aliases in removed_cmd_names during preset removal
Read aliases from preset manifest before deleting pack_dir so alias
command files are included in unregistration and reconciliation.
* fix: add comment to empty except in alias extraction during removal
* fix: scan top-down for effective base in all resolvers
Change base-finding to scan from highest priority downward to find the
nearest replace layer, then compose only layers above it. Prevents
evaluation of irrelevant lower layers (e.g. a wrap without placeholder
below a higher-priority replace) across Python, bash, and PowerShell.
* fix: align CLI composition chain display with top-down base-finding
Show only contributing layers (base and above) in preset resolve
output, matching resolve_content top-down semantics. Layers below
the effective base are omitted since they do not contribute.
* fix: guard corrupted registry entries and make manifest authoritative
- Add isinstance(meta, dict) guard in bash registry parsing so corrupted
entries are skipped instead of breaking priority ordering
- Only use convention-based file lookup when the manifest does not list
the requested template, making preset.yml authoritative and preventing
stray on-disk files from creating unintended layers
* fix: align resolve() with manifest file paths and match extension context_note
- Update resolve() preset tier to consult manifest file paths before
convention-based lookup, matching collect_all_layers behavior
- Use exact extension context_note format matching extensions.CommandRegistrar
- Update test to declare template in manifest (authoritative manifest)
* revert: restore resolve() convention-based behavior for backwards compatibility
resolve() is the existing public API used by shell scripts and other
callers. Changing it to manifest-authoritative breaks backward compat
for presets that rely on convention-based file lookup. Only the new
collect_all_layers/resolve_content path uses manifest-authoritative
logic.
* fix: only pre-compose when this preset is the top composing layer
Skip composition in _register_commands when a higher-priority replace
layer already wins for the command. Register the raw file instead and
let reconciliation write the correct final content.
* fix: deduplicate PyYAML warnings and use self.registry in reconciliation
- Emit PyYAML-missing warning once per function call in bash/PS1 instead
of per-preset to avoid spamming stderr
- Use self.registry.list_by_priority() in reconciliation methods instead
of constructing new PresetRegistry instances to avoid redundant I/O
and potential consistency issues
* fix: document strategy handling consistency between layers and registrar
Composed output already strips strategy from frontmatter (resolve_content
pops it). Raw file registration preserves legacy frontmatter strategy
for backward compat; reconciliation corrects the final state.
* fix: correct stale comments for alias tracking and base-finding algorithm
* security: validate manifest file paths in bash/PowerShell resolvers
Reject absolute paths and parent directory traversal (..) in the
manifest-declared file field before joining with the preset directory.
Matches the Python-side validation in PresetManifest._validate().
---------
Co-authored-by: Manfred Riem <15701806+mnriem@users.noreply.github.com>
* Initial plan
* feat(copilot): add --skills flag for skills-based scaffolding
Add --skills integration option to CopilotIntegration that scaffolds
commands as speckit-<name>/SKILL.md under .github/skills/ instead of
the default .agent.md + .prompt.md layout.
- Add options() with --skills flag (default=False)
- Branch setup() between default and skills modes
- Add post_process_skill_content() for Copilot-specific mode: field
- Adjust build_command_invocation() for skills mode (/speckit-<stem>)
- Update dispatch_command() with skills mode detection
- Parse --integration-options during init command
- Add 22 new skills-mode tests
- All 15 existing default-mode tests continue to pass
Agent-Logs-Url: https://github.com/github/spec-kit/sessions/a4903fab-64ff-46c3-8eb8-a47f495a70c0
Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>
* docs(AGENTS.md): document Copilot --skills option
Agent-Logs-Url: https://github.com/github/spec-kit/sessions/a4903fab-64ff-46c3-8eb8-a47f495a70c0
Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>
* Potential fix for pull request finding 'Unused local variable'
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
* fix: address PR #2324 review feedback
- Reset _skills_mode at start of setup() to prevent singleton state leak
- Tighten skills auto-detection to require speckit-*/SKILL.md (not any
non-empty .github/skills/ directory)
- Add copilot_skill_mode to init next-steps so skills mode renders
/speckit-plan instead of /speckit.plan
- Fix docstring quoting to match actual unquoted output
- Add 4 tests covering singleton reset, auto-detection false positive,
speckit layout detection, and next-steps skill syntax
- Fix skipped test_invalid_metadata_error_returns_unknown by simulating
InvalidMetadataError on Python versions that lack it
* fix: inline skills prompt in dispatch_command auto-detection path
build_command_invocation() reads self._skills_mode which stays False
when skills mode is only auto-detected from the project layout. Inline
the /speckit-<stem> prompt construction so dispatch_command() sends the
correct prompt regardless of how skills mode was detected.
Also strengthen test_dispatch_detects_speckit_skills_layout to assert
the -p prompt contains /speckit-plan and the user args.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: mnriem <15701806+mnriem@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
* fix: --force now overwrites shared infra files during init and upgrade
_install_shared_infra() previously skipped all existing files under
.specify/scripts/ and .specify/templates/, regardless of --force.
This meant users could never receive upstream fixes to shared scripts
or templates after initial project setup.
Changes:
- Add force parameter to _install_shared_infra(); when True, existing
files are overwritten with the latest bundled versions
- Wire force=True through specify init --here --force and
specify integration upgrade --force call sites
- Replace hidden logging.warning with visible console output listing
skipped files and suggesting --force
- Fix contradictory upgrade docs that claimed --force updated shared
infra (it didn't) and warned about overwrites (they didn't happen)
- Add 6 tests: unit tests for skip/overwrite/warning behavior, plus
end-to-end CLI tests for both --force and non-force paths
Fixes#2319
* fix: improve skip warning to suggest specific commands
Address review feedback: the generic '--force' suggestion was
misleading when _install_shared_infra is called from integration
install/switch (which don't have a --force for shared infra).
Now points users to the specific commands that can refresh shared
infra: 'specify init --here --force' or 'specify integration
upgrade --force'.
* feat(cli): add specify self check and self upgrade stub (#2282)
Introduce a new `specify self` Typer sub-app with two subcommands.
`specify self check` performs a read-only lookup against the GitHub Releases
API, compares the installed version to the latest tag with PEP 440 semantics,
and prints one of four verdicts (newer-available, up-to-date, indeterminate,
graceful-failure). When a newer stable release is available, the output
includes a copy-pasteable `uv tool install --force --from git+...@<tag>`
reinstall command. `GH_TOKEN` / `GITHUB_TOKEN` is attached as a bearer
credential when set so users behind shared IPs escape the anonymous 60/hour
rate limit.
`specify self upgrade` is a documented non-destructive stub in this release:
three-line guidance output, exit 0, no outbound call, no install-method
detection. The real destructive implementation is planned as follow-up work.
Failure categorization is a fixed three-entry enum (offline or timeout,
rate limited, HTTP <code>). Anything outside those three categories
propagates as a non-zero exit so bugs surface instead of being silently
swallowed. No machine-readable output, no retries, no caching in this
release — see issue #2282 discussion.
Tests mock `urllib.request.urlopen`; the suite performs zero real network
calls. Full regression suite: 1586 passed.
* fix(cli): disable Rich highlight for deterministic output
Rich's default `highlight=True` applies ANSI color to detected patterns
(integers, version strings, paths) whenever stdout is deemed a TTY.
This caused intermittent failures in existing pytest assertions in
tests/test_cli_version.py and tests/test_extensions.py::TestExtensionRemoveCLI
that compare plain-text output without passing through `strip_ansi()`.
Setting `Console(highlight=False)` globally makes all CLI output
deterministic and fixes the flake without modifying the affected tests.
The numeric cyan highlighting was not a documented part of the CLI
visual contract.
* fix: address copilot review feedback
* fix: tighten self-check token handling
* fix: align self-check helpers and script metadata
* fix: harden self-check version handling
* fix: guard self-check failure rendering
* feat: implement strategy: wrap
* fix: resolve merge conflict for strategy wrap correctness
* feat: multi-preset composable wrapping with priority ordering
Implements comment #4 from PR review: multiple installed wrap presets
now compose in priority order rather than overwriting each other.
Key changes:
- PresetResolver.resolve() gains skip_presets flag; resolve_core() wraps
it to skip tier 2, preventing accidental nesting during replay
- _replay_wraps_for_command() recomposed all enabled wrap presets for a
command in ascending priority order (innermost-first) after any
install or remove
- _replay_skill_override() keeps SKILL.md in sync with the recomposed
command body for ai-skills-enabled projects
- install_from_directory() detects strategy: wrap commands, stores
wrap_commands in the registry entry, and calls replay after install
- remove() reads wrap_commands before deletion, removes registry entry
before rmtree so replay sees post-removal state, then replays
remaining wraps or unregisters when none remain
Tests: TestResolveCore (5), TestReplayWrapsForCommand (5),
TestInstallRemoveWrapLifecycle (5), plus 2 skill/alias regression tests
* fix: resolve extension commands via manifest file mapping
PresetResolver.resolve_extension_command_via_manifest() consults each
installed extension.yml to find the actual file declared for a command
name, rather than assuming the file is named <cmd_name>.md. This fixes
_substitute_core_template for extensions like selftest where the manifest
maps speckit.selftest.extension → commands/selftest.md.
Resolution order in _substitute_core_template is now:
1. resolve_core(cmd_name) — project overrides win, then name-based lookup
2. resolve_extension_command_via_manifest(cmd_name) — manifest fallback
3. resolve_core(short_name) — core template short-name fallback
Path traversal guard mirrors the containment check already present in
ExtensionManager to reject absolute paths or paths escaping the extension
root.
* fix: add bundled core_pack as Priority 5 in PresetResolver.resolve()
resolve_core() was returning None for built-in commands (implement,
specify, etc.) because PresetResolver only checked .specify/templates/
commands/ (Priority 4), which is never populated for commands in a
normal project. strategy:wrap presets rely on resolve_core() to fetch
the {CORE_TEMPLATE} body, so the wrap was silently skipped and SKILL.md
was never updated.
Priority 5 now checks core_pack/commands/ (wheel install) or
repo_root/templates/commands/ (source checkout), mirroring the pattern
used by _locate_core_pack() elsewhere.
Updated two tests whose assertions assumed resolve_core() always
returned None when .specify/templates/commands/ was absent.
* fix: harden preset wrap replay removal
* fix: stabilize existing directory error output
* fix: track outermost_pack_id from contributing preset; use Path.parts in tests
- outermost_pack_id now updates alongside outermost_frontmatter inside
the wrap loop, so it reflects the actual last contributing preset
rather than always taking wrap_presets[0] (which may have been skipped)
- Replace str(path) substring checks in TestResolveCore with Path.parts
tuple comparisons for correct behaviour on Windows (CI runs windows-latest)
* fix: guard against non-mapping YAML manifests; apply integration post-processing in replay
- ExtensionManifest._load raises ValidationError for non-dict YAML roots instead of TypeError
- PresetManager._replay_wraps_for_command calls integration.post_process_skill_content,
matching _register_skills behaviour
- PresetResolver skips extensions that raise OSError/TypeError/AttributeError on manifest load
- Tests: non-mapping YAML, OSError manifest skip, and replay integration post-processing
Extend the alias containment guard from b67b285 to the two remaining
write paths that derive filenames from free-form command/alias names:
- Primary command write in CommandRegistrar.register_commands()
- CommandRegistrar.write_copilot_prompt()
Consolidate the check into a shared _ensure_inside() helper. Per
maintainer guidance on #2229, use a lexical
(os.path.normpath + Path.is_relative_to) containment check rather than
resolve() so `..` / absolute-path traversal is rejected while
intentionally symlinked sub-directories under an agent's commands
directory (e.g. .claude/skills/shared -> /team/shared-skills) keep
working for existing extension setups.
Add 22 parametrised regression cases covering traversal payloads on
primary commands, aliases, and the Copilot companion prompt, plus a
positive case that confirms symlinked sub-directories remain supported.
* fix(copilot): use --yolo to grant all permissions in non-interactive mode
The Copilot CLI's --allow-all-tools flag only covers tool execution
permissions but does not grant path or URL access. When the Copilot
agent autonomously runs shell commands (e.g. npm run build) during
workflow execution, the CLI blocks path access and cannot prompt for
approval in non-interactive mode, producing:
Permission denied and could not request permission from user
Replace --allow-all-tools with --yolo (equivalent to --allow-all-tools
--allow-all-paths --allow-all-urls) to grant all three permission types.
Rename the opt-out env var from SPECKIT_ALLOW_ALL_TOOLS to
SPECKIT_COPILOT_ALLOW_ALL to match the formal --allow-all alias and
scope it to the Copilot integration.
Fixes#2294
* review: deprecate SPECKIT_ALLOW_ALL_TOOLS, rename to SPECKIT_COPILOT_ALLOW_ALL_TOOLS
Address Copilot review feedback:
- Honour the old SPECKIT_ALLOW_ALL_TOOLS env var as a fallback with a
DeprecationWarning so existing opt-outs are not silently ignored.
- Rename the new canonical env var to SPECKIT_COPILOT_ALLOW_ALL_TOOLS.
- New var takes precedence when both are set.
- Use monkeypatch in tests to avoid flakiness from ambient env vars.
- Add tests for deprecation warning, precedence, and opt-out paths.
* review: use UserWarning instead of DeprecationWarning
DeprecationWarning is suppressed by default in Python, so users relying
on the old SPECKIT_ALLOW_ALL_TOOLS env var would never see the
deprecation notice during normal CLI runs. Switch to UserWarning which
is always shown. Update test to also assert the warning category.
