main
100 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Manfred Riem
|
fa3a5c5ce7 |
Clarify extension catalog trust model in docs, help, and messaging (#4177)
* Clarify extension catalog trust model in docs, help, and messaging (#4176) Extension catalog management gave no explanation of why the community catalog is discovery-only, and the install-error text nudged users to flip a discovery catalog to install_allowed — exactly the wrong move. - Docs: add a "discovery-only vs. install sources" trust-model section, document `add --from <url>` as the lightweight vetted-install path, and stop implying you should make community installable. - Help: expand the `catalog` app and `--install-allowed` help to state the vetting intent instead of bare mechanics. - Messaging: rewrite the not-installable errors in `add`, `search`, and `info` to point at `--from` and self-curated catalogs, and to say explicitly not to flip a discovery-only catalog to install_allowed. - `catalog list` now prints trust-model guidance when a discovery-only catalog is active. - Tests cover the new list guidance (present/absent). Deliberately does not add a verb to toggle install_allowed on an existing catalog: discovery-only is a security boundary, not an inconvenience. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d * Address PR review: copy-pasteable install hint and accurate --from warning (#4176) - The discovery-only "install directly" hint used the user-typed argument, which can be a display name with spaces (resolved via search) and would break when copied as a shell command. Emit the resolved catalog ID (ext_info['id']) instead. Added a regression test. - The `--from` untrusted-source warning claimed the URL was "not listed in any of your configured extension catalogs", which is false for a URL copied from a discovery-only catalog — the exact flow this PR documents. Reword it to state the install is bypassing trusted (install-allowed) catalogs, which is accurate regardless of discovery-catalog membership. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d * Harden install hints against catalog-controlled IDs; expose archive URL (#4176) Second review round on #4177. Shell-safety: catalog entry IDs (especially from discovery-only catalogs) are not validated during catalog merge, and rich.markup.escape only neutralizes Rich markup, not shell metacharacters. A malicious ID like `foo; rm -rf ~` was interpolated into the `specify extension add ... --from` command we encourage the user to copy. Add `_command_safe_id`, which only emits an ID matching the manifest rule `^[a-z0-9-]+$` (via VALID_EXTENSION_ARTIFACT_NAME_PATTERN) and otherwise falls back to a literal `<extension-id>` placeholder. Applied to every suggested command in `add`, `search`, and `info`. Discoverability: the documented `--from <archive-url>` flow gave no CLI path to obtain the URL. `extension info` now prints the candidate `download_url` for a discovery-only entry (clearly flagged as needing vetting), and the docs show `extension info <name>` as the way to get the archive URL. Tests cover the resolved-ID hint, the unsafe-ID neutralization, and pass the full extensions + CLI suites (635). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d * Reject leading-hyphen catalog IDs; test info archive-URL branch (#4176) Third review round on #4177. _command_safe_id: an ID like `--force` satisfies the manifest character rule `^[a-z0-9-]+$` but Typer parses a leading hyphen as an option rather than the positional extension argument, so an untrusted catalog could still yield a non-copyable or option-altering suggested command. Reject a leading hyphen and fall back to the `<extension-id>` placeholder. Tests: cover the new `extension info` discovery-only branch that surfaces the candidate `download_url` (plus the no-URL fallback), and the leading-hyphen rejection. Full extensions suite green (528). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, supervised) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86c498e-f129-4422-9983-d1a33513fd4d |
||
|
chelsealong
|
11e3176fd1 |
fix(extensions): reject duplicate provides.templates/scripts names (#4016)
The resolver returns the first entry matching a declared name, so a later duplicate within provides.templates or provides.scripts was silently unreachable while still counted by ExtensionManifest properties. Reject duplicates at manifest-validation time instead. Also clarify EXTENSION-DEVELOPMENT-GUIDE.md's provides section: hooks and events are top-level manifest fields, not provides sub-fields, so the "at least one of ..." wording doesn't imply they can be nested under provides. |
||
|
chelsealong
|
684b3d8e05 |
feat(extensions): accept provides.templates and provides.scripts in manifest (#4012)
* feat(extensions): accept provides.templates and provides.scripts in manifest Extensions could only formally declare commands under `provides` (plus config/hooks/events); templates and scripts shipped by an extension were picked up purely by filename convention, with no id, description, or metadata. Add optional `provides.templates` and `provides.scripts` sections to the extension manifest schema, mirroring the preset template shape minus an authorable `strategy` (extension artifacts always resolve as replace, so a present `strategy` key is now a validation error rather than a silently accepted no-op). ExtensionManifest gains `templates`/`scripts` properties so tooling can enumerate an extension's declared artifacts directly from the manifest. An extension may now satisfy the "must provide something" rule with only a template or script, not just a command/hook/event. Addresses the manifest-schema portion of #4010; resolver authoritative-vs-convention precedence for these new sections is left for a follow-up. * fix(presets): wire extension-declared templates/scripts into resolver collect_all_layers only consulted ExtensionManifest for command resolution, leaving provides.templates/.scripts purely decorative -- a declared entry whose file didn't sit at the conventional path was validated but never resolved. Extend the existing manifest-fallback branch to cover template_type "template" and "script" the same way it already does "command": convention lookup first, manifest lookup as fallback so undeclared on-disk files keep resolving unchanged. * fix(presets): make extension manifest lookup authoritative over convention Copilot review on #4012 found the manifest-declared template/script lookup was gated on convention lookup missing first, so a stale conventional file could shadow a declared entry at a non-conventional path, and resolve() never consulted the manifest at all (only collect_all_layers() did). Add a shared _extension_manifest_declared_template() helper and check it before convention-based lookup in both resolve() and collect_all_layers(), mirroring the preset manifest precedence. Also update EXTENSION-DEVELOPMENT-GUIDE.md, which still claimed provides only supports commands and required a command or hook. * fix(presets): stop resolving symlinks in extension manifest candidate path _extension_manifest_declared_template() resolved ext_dir/rel_path before returning it, which follows symlinks in ext_dir's ancestors (e.g. macOS's symlinked tmp dir) and diverges from the unresolved paths convention-based lookup returns for the same directory. Resolve only for the traversal containment check; return the unresolved candidate. Fixes the 4 CI test failures across all OS/Python matrix jobs on #4012. |
||
|
Noor ul ain
|
3d4f71c90e |
fix(extensions): start fresh on a non-UTF-8 extension registry (#3998)
ExtensionRegistry._load() catches json.JSONDecodeError and FileNotFoundError to start fresh on a corrupted or missing registry, but a .registry file with invalid UTF-8 bytes raised UnicodeDecodeError from the text-mode read before JSON parsing began. Because the registry is loaded in __init__, that bare traceback broke every extension command -- `specify extension list` on such a project exits with a raw UnicodeDecodeError instead of the module's clean path. Catch UnicodeDecodeError in the same clause: undecodable bytes are the same corruption class as unparseable JSON, only the exception type differs. OSError stays uncaught on purpose -- the data may be intact on disk, and starting fresh would let a later _save() wipe it. This is the exact twin of the PresetRegistry._load() fix in #3955; the two registries are parallel implementations and only the preset side was corrected. _get_installed_sibling_ids() already worked around this gap locally by catching UnicodeError at its own call site; its comment is updated to reflect that _load() now handles the case itself, with the local catch kept as belt-and-braces against regression. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
deborre
|
4d5458c883 |
fix: keep long frontmatter values on a single line (#3989)
`CommandRegistrar.render_frontmatter` calls `yaml.dump()` without `width=`,
so PyYAML applies its default ~80-column wrap and folds any long scalar onto
a continuation line.
A `description` longer than roughly 80 characters is therefore rendered as:
---
name: speckit-implement
description: Execute the implementation plan by processing and executing all tasks
defined in tasks.md
---
The YAML remains valid and round-trips faithfully through `yaml.safe_load`,
so this is not data loss. It is a shape inconsistency with real consequences:
- Hand-written core command templates always keep `description` on one line,
so preset- and extension-rendered commands do not match the files they sit
beside in the same directory.
- Consumers that read frontmatter line-wise rather than with a YAML parser
see the description truncated at the fold, followed by a stray line. Spec
Kit itself hand-builds SKILL.md frontmatter in the skills path (see #3391),
so this is not a hypothetical class of consumer.
- `speckit.implement`'s own description is 89 characters, so a preset that
overrides it hits this immediately.
`width=float("inf")` disables the line-wrapping only; escaping, quoting and
the handling of genuinely multi-line values are unchanged, since PyYAML
selects the scalar style before applying width.
Adds a regression test that fails without the change.
Verified against the repo's own suite: 6354 passed. Four failures in
tests/integrations/test_integration_subcommand.py are present on a clean
checkout too (ANSI escapes in captured output) and are unrelated.
|
||
|
Marsel Safin
|
f245c6cd1a |
fix(extensions): treat an unreadable staged backup as a conflict (#3962)
The rescue-retry loop in install_from_directory() reads each staged backup with bare stat()/read_bytes() calls, so a staged config that cannot be read crashed the reinstall with a raw OSError. Every sibling read in this path — the live twin four lines below, the packaged baseline check, the mode sidecar — already catches OSError. Treat an unreadable staged file like an uncomparable live config: add it to the conflict set so both copies are preserved and the retry aborts with the existing resolution guidance while dest_dir is still untouched. Assisted-by: GitHub Copilot (model: claude-fable-5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Noor ul ain
|
cd996f74eb |
fix(manifests): reject non-string requires.speckit_version (#3980)
`requires.speckit_version` was presence-checked but never type-checked in both the extension and preset manifest validators, so an unquoted YAML `speckit_version: 1.0` (a float) passed validation and reached `SpecifierSet(required)` in `check_compatibility()`. That call is guarded by `except InvalidSpecifier` alone, which a non-string escapes two different ways: - a float/int/bool/None raises `TypeError: 'float' object is not iterable` from the `SpecifierSet` constructor; - a list or dict is an *iterable*, so `SpecifierSet` accepts it and the failure surfaces much later as `AttributeError: 'str' object has no attribute 'filter'` from inside `.contains()`. Neither is a `CompatibilityError`/`PresetCompatibilityError`, so both bypass the CLI's "Compatibility Error" handler in `_commands.py` and exit 1 with a raw traceback that names no field, leaving the author with no hint which manifest key is wrong. Type-check the field in both validators, requiring a non-empty string, and additionally guard `check_compatibility()` in both managers since each is public and reachable with a hand-built or mutated manifest. This mirrors the sibling `IntegrationDescriptor`, which already requires a non-empty string for the same key, and completes the type-checking pass started in #3943 for the neighbouring `extension`/`preset` fields. Adds 33 regression tests across both modules covering every escape path; 26 of them fail without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-by: Claude Code (model: Claude Opus 5, supervised) |
||
|
Marsel Safin
|
0fa86e8e9c |
fix(extensions): reject reinstall when a kept config cannot be read (#3960)
The keep-config rescue branch of install_from_directory() reads each preserved config with bare read_bytes()/stat() calls, so a kept config that cannot be read (permission or I/O error) crashed the reinstall with a raw OSError. The sibling symlink guard four lines above already rejects with a ValidationError and resolution guidance for the same reason: bytes that cannot be safely rescued must not reach the rmtree below. Wrap the read and raise ValidationError with guidance, while dest_dir is still untouched so the preserved bytes are never lost. Assisted-by: GitHub Copilot (model: claude-fable-5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Noor ul ain
|
2d8904a21e |
fix(manifests): reject non-string metadata instead of crashing on it (#3943)
* fix(manifests): reject non-string metadata instead of crashing on it `ExtensionManifest` and `PresetManifest` checked only key PRESENCE for `id`/`name`/`version`/`description`, then fed the values straight to `re.match()` and `packaging.Version()`. Both raise a bare `TypeError` on a non-string, which is neither `ValidationError` nor `PresetValidationError`, so it escaped every caller that already handles a malformed manifest. YAML makes this an easy authoring slip rather than a contrived one: an unquoted `version: 1.0` parses as a float and `id: 2` as an int. The user-visible symptom is the one the in-tree comment above the section guards was written to prevent (#3898 for presets, and its extension twin): `list_installed()` degrades a bad manifest to "⚠️ Corrupted extension" but catches only the domain error, so a single bad manifest made `specify extension list` / `specify preset list` exit 1 with a raw traceback and *no output at all* — hiding every healthy extension/preset too, not just the broken one. Also unguarded on the same path: - extension `provides.commands[].name` → `TypeError` from the command-name pattern match. The sibling `file` field was already safe, since `relative_extension_path_violation()` rejects a non-string. - preset `provides.templates[].name`/`.file` → `TypeError` from `re.match` and `os.path.normpath` respectively. The third manifest twin, `IntegrationDescriptor`, is already hardened: it type-checks the same four fields and catches `TypeError` alongside `InvalidVersion`. This brings the other two in line with it. Tests: 68 added across both suites, covering each field against float, int, None, list, dict, and bool, plus an end-to-end guard per manifest type asserting a healthy entry still lists while the bad one degrades. All 68 fail with the source change reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Matt Van Horn
|
15cb7d9a66 |
feat(extensions): scaffold config templates on extension add/enable (#2000)
* feat(extensions): scaffold config templates on extension add/enable Deploy an extension's provides.config templates into .specify/ when the extension is added or enabled. Existing files are never overwritten, so user customizations are preserved. Addresses the review on #2000: - ExtensionManifest.config returns [] unless provides.config is a list of dicts, so a malformed manifest cannot crash callers. - scaffold_config returns a consistent (deployed, skipped_existing, failed) tuple on every path, including a missing manifest. - Template paths must resolve inside the extension dir and targets inside .specify/; symlinks and non-regular files are rejected. - Callers distinguish "already exists (preserved)" from "not scaffolded", and extension_enable no longer crashes on a corrupt manifest. - Tests cover traversal, absolute paths, symlinks, directory templates, malformed provides.config, and the missing-manifest tuple shape. Ported onto the extensions package introduced by #3014: the manager and manifest changes land in extensions/__init__.py and the CLI wiring in extensions/_commands.py. * fix(extensions): deploy config where it is read, and contain the write Addresses @Copilot's review. Config now lands in .specify/extensions/<id>/ rather than the .specify/ root. ConfigManager._get_project_config() reads .specify/extensions/<id>/<id>-config.yml, and the bundled scripts and READMEs use the same path, so a scaffolded git-config.yml was being written somewhere the git extension never looks. Containment is checked component by component before .specify is used as the root. Resolving it first and trusting the result let a symlinked component point outside the project, after which every target satisfied relative_to and copy2 wrote externally. This matches the project safe-write path in shared_infra. mkdir moved inside the OSError handler. A nested target like foo/config.yml raised out of scaffolding when its parent could not be created, and on extension add that happened after the extension was already installed. The 'Configuration may be required' warning is now conditional. It ran unconditionally after the scaffolding block, so it contradicted the success output directly above it and fired for extensions with no provides.config at all. Tests cover the corrected location, a symlinked config root, and an uncreatable nested target. * fix(extensions): only scaffold config targets that removal preserves remove(keep_config=True) rmtree's every subdirectory and keeps only top-level -config.yml / -config.local.yml files; the backup path globs the same top-level pattern. Scaffolding a nested or differently-named target therefore handed the user a file that 'extension add --force' silently deleted and replaced with the template default, losing customization. Constrain scaffold targets to that convention rather than widening four removal paths. --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> |
||
|
Quratulain-bilal
|
36cb7e3c11 |
fix: bound response reads in extension catalog and download (#3775)
* fix: bound response reads in extension catalog and download Replace unbounded esponse.read() calls with ead_response_limited() from _download_security in extensions/__init__.py to prevent denial- of-service via oversized catalog or extension archive responses. Three call sites fixed: - _fetch_single_catalog JSON read (catalog metadata) - _fetch_catalog JSON read (legacy path) - download_extension ZIP read (binary download) All existing mock tests updated to use side_effect with BytesIO.read instead of eturn_value, ensuring compatibility with the chunked read loop in ead_response_limited. Two regression tests added: - test_oversized_catalog_response_rejected - test_oversized_extension_download_rejected * fix: remove .decode utf-8 to preserve bytes for json.loads json.loads accepts bytes directly. Removing .decode maintains compatibility with BOM-bearing or UTF-16/32 catalogs. |
||
|
Manfred Riem
|
a3e183d069 |
feat: support tar archives for installs (#3874)
* feat: support tar archives for installs Add secure .tar.gz and .tgz parity with ZIP installation for extensions, presets, and workflows, including full workflow package preservation. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * chore: clean rebased archive imports Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * fix: preserve hardened archive install behavior Keep malformed ZIP diagnostics, filesystem-independent manifest selection, and reserved workflow overlays consistent after adding generic archive support. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * fix: extract staged workflow archives by descriptor Avoid reopening a held staging path on Windows while retaining authoritative-inode archive validation. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * fix: extract catalog archives from verified bytes Use the already bounded and SHA-verified response bytes directly so Windows file-sharing semantics cannot affect archive detection. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd * fix: address archive install review feedback Preserve forced preset reinstalls, sniff suffixless workflow archives without weakening YAML limits, and restore prior workflow packages before failed-install cleanup. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd --------- Copilot-Session: bd07c6b3-f1f9-484c-869a-94d8fef970dd |
||
|
Manfred Riem
|
6577ffc92b |
Harden extension URL download cache against symlink and junction races (#3869)
* fix(extensions): harden URL download cache Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * fix(extensions): retain secure archive descriptor Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Harden extension URL cache anchor opens Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Use descriptor-safe mkdir for cache components Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Harden extension URL download cache Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Align extension manifest regression expectation Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Make download cache leaf anonymous to remove cleanup TOCTOU Address review: the best-effort cleanup walk re-derived the downloads directory by path, so a cache ancestor swapped after the archive was opened could redirect os.unlink to a replacement leaf, and it silently no-op'd (failing open) on platforms without descriptor-relative unlink. _safe_open_download_zip now unlinks the exclusively-created leaf immediately via the same directory descriptor, returning an fd backed by an anonymous inode. Installation already consumes that descriptor through archive_file, so the on-disk pathname is never reopened and no cleanup walk is needed. The capability gate additionally requires os.unlink in os.supports_dir_fd, so unsupported platforms fail closed. Removed the now-unused _safe_unlink_download_zip helper and its cleanup finally, and updated the tests accordingly. Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Fix Windows test matrix for cache hardening tests The hardened cache primitives fail closed on platforms without dir_fd/ O_NOFOLLOW, so on the windows-latest matrix several tests errored instead of exercising POSIX behavior: - test_symlinked_cache_ancestor_is_refused and test_cache_ancestor_resolving_outside_project_is_refused called _validate_safe_cache_dir directly and expected typer.Exit, but on Windows it raises NotImplementedError first. Guard both with _require_secure_dir_fd() so they skip where the primitive is unavailable. - test_safe_open_fails_closed_without_atomic_platform_support built its download dir via _validate_safe_cache_dir, which itself fails closed on Windows; construct the directory directly so the assertion targets _safe_open_download_zip's platform gate in isolation. - The _open_test_download_zip stand-in unlinked a still-open file, which raises PermissionError on Windows. Use O_TEMPORARY there (auto-delete on close) and keep immediate unlink on POSIX. Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Support Windows in extension URL download-cache hardening Replace the fail-closed NotImplementedError on platforms lacking dir_fd with a portable, still-hardened download path so `specify extension add --from <url>` works on Windows instead of rejecting the install. - `_validate_safe_cache_dir` now dispatches to a POSIX dir_fd + O_NOFOLLOW walk when available, and otherwise a portable path-wise walk that rejects symlink/junction components before and after each mkdir and requires every component to resolve back under the project root. - `_safe_open_download_zip` keeps the POSIX anonymous-inode create/unlink and adds a portable leaf create using O_EXCL + O_TEMPORARY (auto-delete on close) plus a post-open fstat/lstat inode-identity check to detect a leaf swapped underneath us. Installation still consumes only the open descriptor, so the cache pathname is never reopened. - Detect the symlink-refusal case via errno (ELOOP/ENOTDIR/EMLINK) instead of FileExistsError, and add O_CLOEXEC to the descriptor-walk opens. - Drop the now-unreachable NotImplementedError handling in the --from branch. - Tests: cover the portable path (success, symlinked-leaf refusal, symlinked ancestor refusal, full --from install) and keep the POSIX-only cases guarded. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 |
||
|
Manfred Riem
|
623466dc42 |
test(extensions): update stale manifest validation message assertion (#3859)
The extensions `events` feature changed the "nothing provided" validation error from "Extension must provide at least one command or hook" to "Extension must provide at least one command, hook, or event", but test_empty_provides_and_no_hooks_keeps_its_own_message still asserted the old wording, so it failed on main. Update the regex and also pop `events` from the fixture so the test truly exercises the empty-provides path. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 189d67d7-2028-4319-a459-b22919d43a3e |
||
|
Ali jawwad
|
2ef96532d2 |
fix(agents): coerce a non-string description in TOML command rendering (#3799)
CommandRegistrar.render_toml_command passes the raw frontmatter `description`
straight into `_render_basic_toml_string`, which iterates the value and calls
ord() on each character. Frontmatter comes from yaml.safe_load, so description
can be any YAML type:
description='ok string' -> description = "ok string"
description=None -> TypeError: 'NoneType' object is not iterable
description=42 -> TypeError: 'int' object is not iterable
description=True -> TypeError: 'bool' object is not iterable
description=['a','b'] -> description = "ab" <- silently WRONG value
This is a format-branch asymmetry: it is the only renderer reached from
register_commands' format branches that does not normalise description.
render_yaml_command (same class, ~70 lines below) already does exactly
`if not isinstance(description, str): description = str(description) if
description is not None else ""`, render_markdown_command goes through
yaml.dump which handles any type, and TomlIntegration._extract_description
returns "" for a non-str. So only extension/preset commands rendered for the two
TOML agents were affected.
Apply the same coercion the sibling uses. After: None -> "", 42 -> "42",
True -> "True", ['a','b'] -> "['a', 'b']", each still valid parseable TOML.
String descriptions are untouched.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
kanfil
|
f8e474d6fd |
feat: first-class agent-native runtime hooks for integrations (#3704)
* feat: first-class agent-native runtime hooks for integrations
* refactor: rework integration events per maintainer review
- Rename hooks terminology to 'events' (events:, --events flag, events.py).
- Use snake_case names for canonical events consistent with spec-kit vocabulary.
- Fold event config adapters into integration classes via class attributes (CANONICAL_TO_NATIVE, events_config_file, events_format).
- Lift event command-script resolution to core 'specify event run' command.
- Split events sourcing from integration config writing.
- Support first-class Copilot CLI events JSON generation under '.github/hooks/speckit.json'.
- Rewrite and expand full test suite under 'tests/integrations/test_events.py'.
Assisted-by: opencode (model: litellm/gemini-3.5-flash, autonomous)
* fix(events): resolve ruff lint errors blocking CI
Address Copilot review finding #18 (src/specify_cli/__init__.py event-command
import missing # noqa: E402), #19 (unused console import in commands/event.py),
and #20 (unused patch/yaml/Path/integration imports in test_events.py). Also
fix two stray F541 f-string prefixes in _build_opencode_plugin that ruff
flagged in the same job.
Bump dev version 0.14.2.dev0 -> 0.14.2.dev1 and add a CHANGELOG entry per the
AGENTS.md convention for Specify CLI __init__.py changes.
Refs: PR #3704 Copilot inline review (findings #18, #19, #20)
Assisted-by: opencode (model: glm-5.2, autonomous)
* fix(events): make generated native hooks actually execute
Address Copilot review findings that left generated event hooks inert or
schema-invalid after the rework:
- #2: the resolved events map now carries an ordered list of handlers per
event (dict[str, list[dict]]) so two extensions declaring the same event
both run instead of the last one silently winning. collect_extension_events
accumulates; every adapter emits one native entry per handler.
- #6: Claude/Gemini/Qwen/Devin/Tabnine native schema accepts a single
'command' string, not command+args. Each adapter now renders one complete
shell invocation of the dispatcher via _dispatcher_command().
- #7: Gemini measures hook timeouts in milliseconds; add events_timeout_unit
attr and _native_timeout() so the 60s default becomes 60000ms instead of
terminating the dispatcher after 60ms.
- #4: _resolve_event_command_argv() replaces _extract_script_path() —
scripts: values are command strings (e.g. 'scripts/bash/setup-plan.sh --json'),
not bare paths. Resolves the project's sh/ps/py variant, splits safely into
argv, and prepends the interpreter for .py.
- #5: bundled-template fallback now uses _locate_core_pack()/_repo_root()
(core_pack/commands, not the non-existent core_pack/templates/commands).
- #16: all formatters use IntegrationBase.resolve_python_interpreter() so
generated commands honor the project venv and never hard-code python3
(absent on Windows). The opencode TS plugin bakes in the same resolved
interpreter.
- #13: opencode TS plugin runEvent() now throws on failure instead of
process.exit(2), which killed the OpenCode host process; only the failing
hook is rejected.
- #21: user YAML override is validated (event names, non-empty command
strings) before returning; a malformed override is warned about and
ignored rather than crashing installation on cfg.get().
Bump dev version 0.14.2.dev1 -> 0.14.2.dev2 (gemini/__init__.py change) and
add a CHANGELOG entry.
Refs: PR #3704 Copilot inline review (findings #2, #4, #5, #6, #7, #13, #16, #21)
Assisted-by: opencode (model: glm-5.2, autonomous)
* fix(events): merge/teardown idempotency and data safety
Address Copilot review findings on native-config merge and teardown:
- #9: _has_marker now recurses into nested 'hooks' arrays so a matcher-group
containing Specify-owned inner hooks is recognized and replaced on upgrade
instead of accumulating duplicates.
- #11: _merge_json_fragment strips ALL Specify-marked entries from every event
before adding the new set, so an override that drops an event (pre_tool_use
-> stop) removes the stale marked entry instead of leaving it active.
- #3: an empty resolved map (--events false / disabled override) now runs the
native-config removal path instead of early-returning, so prior Specify
hooks are stripped. The shared dispatcher is left untouched (#10).
- #14: teardown deletes a Spec-Kit-created config that is now empty of user
content (rather than leaving '{}' that confused manifest.uninstall()),
while preserving pre-existing configs with user hooks/settings.
- #10: the shared .specify/events.py dispatcher is deleted only when no other
installed event-capable integration's manifest still references it, so
uninstalling one multi-install integration doesn't break the others.
- #8: Copilot's .github/hooks/speckit.json now merges owned entries (with
markers) into a pre-existing file instead of overwriting, and teardown
removes only owned entries (deleting the file when no user hooks remain).
- #22/#23: JSON/JSONC parse failures in native configs (Claude/Cursor/etc.
and opencode.json) abort the merge with a warning instead of resetting user
content to '{}'.
- #12: write destinations are validated (symlinked-ancestor rejection +
containment) before any bytes are written, so a symlinked .specify or
native config directory can't redirect writes outside the repository.
Refs: PR #3704 Copilot inline review (findings #3, #8, #9, #10, #11, #12, #14, #22, #23)
Assisted-by: opencode (model: glm-5.2, autonomous)
* fix(events): honor enabled flag, refresh on extension lifecycle, strict command validation
Address Copilot review findings on sourcing, validation, and lifecycle:
- #1: collect_extension_events now honors the extension registry's 'enabled'
flag — a disabled extension's events are skipped so disabling an extension
actually deactivates its runtime hooks. Adds refresh_integration_events(),
wired into extension add/remove/enable/disable, so installing, removing,
enabling, or disabling an extension regenerates each installed event-capable
integration's native event config (the documented install-after-init flow is
no longer inert, and disabled/removed extension events are stripped).
- #17: validate_events now requires 'command' to be a non-empty string, not
merely truthy, so a value like 'command: [foo]' is rejected at manifest
load instead of rendering into invalid native configuration.
- #15: updated PR #3704 description to the implemented events terminology
(.specify/events.py, events:, --events, integration-events.yml) replacing
the stale bridge.py / runtime_hooks: / --hooks false / integration-hooks.yml
references that no longer match the shipped API.
(#21 — user YAML override validation — was addressed in the prior tier.)
Refs: PR #3704 Copilot inline review (findings #1, #15, #17)
Assisted-by: opencode (model: glm-5.2, autonomous)
* revert: drop CHANGELOG.md/pyproject.toml version bumps from events fixes
Per maintainer request, the events PR no longer carries CHANGELOG entries or
pyproject version revs. This restores both files to their pre-PR (da6c20d9)
state: pyproject.toml back to 0.14.2.dev0 and the [Unreleased] block removed
from CHANGELOG.md. The AGENTS.md version-rev convention for __init__.py
changes is intentionally waived for this PR by maintainer decision.
This also clears the pending merge conflicts with upstream/main on these two
files (upstream's 0.14.2 release commit
|
||
|
Ali jawwad
|
1fff7a196d |
fix(extensions): guard the required manifest sections so one bad extension cannot break extension list (#3797)
ExtensionManifest.REQUIRED_FIELDS only checks key PRESENCE, so a section that is
written but left empty (`provides:` -> None) or given the wrong shape
(`provides: []`) passes it and then fails on first use:
extension: null -> TypeError: argument of type 'NoneType' is not iterable
requires: null -> TypeError: argument of type 'NoneType' is not iterable
provides: null -> AttributeError: 'NoneType' object has no attribute 'get'
provides: [] -> AttributeError: 'list' object has no attribute 'get'
Neither is a ValidationError, so both escape the callers that already handle
malformed manifests. list_installed() catches ValidationError only and has a
deliberate "Corrupted extension" fallback, so a single bad extension took down
the whole command -- reproduced end-to-end:
before: specify extension list -> exit 1, raw AttributeError, no output
after: specify extension list -> exit 0, the good extension listed, the
bad one shown as "Corrupted extension"
Add an isinstance guard for each required section, mirroring the nested guards
already in this function ("Invalid provides.commands: expected a list", "Invalid
hooks: expected a mapping") and _load_yaml's document-root check. Only the three
REQUIRED sections lacked one.
`provides: {}` is unaffected: it is a well-shaped mapping, so an extension that
provides only hooks still validates, and with no hooks it keeps the pre-existing
"must provide at least one command or hook" message. Both are locked by tests.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Noor ul ain
|
8bfe6e14d1 |
fix(workflows,extensions): tolerate non-list catalog tags in search/info display (#3770)
`workflow search`, `workflow info`, `extension search` and `extension info`
crashed with `TypeError: 'int' object is not iterable` when a catalog entry
carried a scalar `tags:` value (e.g. `tags: 5`). Catalog payloads are
user-editable YAML/JSON, so this shape reaches the display unvalidated.
Both backends already guard their tag *filter* with
`isinstance(raw_tags, list)` — `WorkflowCatalog.search` and
`ExtensionCatalog.search` skip a non-list `tags` cleanly. Only the display
paths were unguarded: they tested truthiness (`if info.get("tags"):`) and
then iterated. A scalar is truthy but not iterable, so `--tag` filtering
survived while plain `search`/`info` rendering blew up.
Note this is distinct from the non-string *element* handling added in
#3746/#3747: coercing elements with `str(t) for t in ...` does not help when
`tags` is not a sequence at all. The fix is the guard the sibling
integration commands already use — `integrations/_query_commands.py:332,402`
gate on `isinstance(tags, list) and tags`. This aligns workflows and
extensions with that reference pattern, leaving all four tag-join display
sites in these modules consistent.
Regression tests drive the full CLI via CliRunner and cover search + info in
one case per module; both fail before the fix with the exact TypeError.
Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Ben Buttigieg
|
655a3cb8ca |
fix(integrations): preserve native skill invocation prefixes (#3663)
* fix(integrations): use native dollar skill invocations Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(integrations): preserve skill post-process idempotence Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(integrations): preserve literal skill invocations Resolve generated command references with the active agent prefix instead of rewriting all slash-form text during post-processing. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(integrations): preserve shared invocation prefix Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(integrations): preserve install invocation prefix Pass dollar-style skill prefixes through bare-project integration installation and cover the shared template output. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(integrations): preserve dollar refs everywhere Use agent-native invocation prefixes in extension command registration and dynamic shared-script command hints. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(shared-infra): preserve dollar command hints Escape dollar-prefixed commands embedded in Bash strings and propagate the native prefix into installed Python command helpers. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(shared-infra): render native helper prefixes Rewrite installed Bash and PowerShell formatter return expressions so direct callers receive the selected integration's native prefix. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(skills): use invocation-neutral hook guidance Describe hook-derived references as command invocations so dollar-prefixed skills do not receive contradictory slash-command terminology. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * test(integrations): expect native fallback invocation Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * refactor(integrations): centralize invocation prefix selection Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 * fix(integrations): add Kimi /skill: prefix and fix docstrings - Add SKILL_COLON_AGENTS frozenset and get_invocation_prefix() to _invocation_style.py so Kimi resolves to '/skill:' in skills mode - Switch invoke_prefix_for_integration() to use get_invocation_prefix() instead of the binary dollar/slash check - Update post_process_skill_content docstring (base.py) to cover both slash and dollar native invocation forms - Update _resolve_command_refs_in_skill docstring (presets/__init__.py) to document the dollar-prefixed result alongside slash forms Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix(agents): use get_invocation_prefix for Kimi in register_commands Replace the binary is_dollar_skills_agent ternary with get_invocation_prefix so that __SPECKIT_COMMAND_*__ tokens in Kimi skill files resolve to /skill:speckit-<name> rather than /speckit-<name>. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix(agents): remove unused is_dollar_skills_agent import Leftover from replacing the inline ternary with get_invocation_prefix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix(integrations): use get_invocation_prefix in post_process_skill_content Replaces the binary is_dollar_skills_agent ternary with get_invocation_prefix so that Kimi's hook-command note is injected as /skill:speckit-git-commit from the start. This keeps _inject_hook_command_note idempotent for Kimi: the previous note with its native prefix now matches on repeated passes, preventing duplicate note injection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix(presets): use get_invocation_prefix in _resolve_skill_command_refs Replace the binary is_dollar_skills_agent ternary with get_invocation_prefix so Kimi tokens resolve to /skill:speckit-* directly rather than /speckit-* (which previously relied on the broad post-process body replacement). Also fix test_restore_skill_preserves_dollar_command_refs to write raw_core with the unresolved __SPECKIT_COMMAND_PLAN__ token, exercising the resolver rather than bypassing it with a pre-resolved string. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * docs(presets): document /skill: form in _resolve_skill_command_refs Add /skill:speckit-<cmd> to the docstring so the contract covers all three native prefix forms returned by get_invocation_prefix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * test(integrations): add Kimi /skill: prefix coverage - test_skill_colon_prefix_core_command: resolve_command_refs with /skill: prefix - test_get_invocation_prefix_skill_colon: get_invocation_prefix returns /skill: for kimi (skills), / for kimi (non-skills), $ for codex, / for claude - test_kimi_skill_post_processing_is_idempotent: verifies Kimi's hook-command note is injected with /skill: prefix and does not duplicate on re-runs - test_installed_bash_formatter_uses_skill_colon_prefix: shared-infra bash formatter outputs /skill:speckit-plan when installed with /skill: prefix Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix(kimi): use get_invocation_prefix in process_template, remove broad replacement process_template() was still using a binary is_dollar_skills_agent ternary to select between dollar and slash prefix, so Kimi tokens were emitted as /speckit-* and then corrected by a broad .replace('/speckit-', '/skill:speckit-') in KimiIntegration.post_process_skill_content(). That broad replacement would also rewrite any literal /speckit-* text in generated skill content, contrary to the PR's token-only behavior. - Use get_invocation_prefix(agent_name, invoke_separator == '-') in process_template() so Kimi tokens are emitted as /skill:speckit-* directly. - Remove the broad .replace() from KimiIntegration.post_process_skill_content(); it is now a no-op (tokens are already correctly prefixed at source). - Add test_process_template_kimi_uses_skill_colon_prefix to guard the fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 02f9e138-da58-4a60-93b9-eae659d2aa19 Copilot-Session: 65ef91d9-4c31-4f31-a009-ed2093fe7f28 |
||
|
Pascal THUET
|
118062eac4 |
harden: secure extension and preset archive downloads (#3141)
* harden: secure extension and preset archive downloads Adopt the shared download-security primitives from #3140 across extension and preset catalog, package, direct-URL, and ZIP-install flows: - bound catalog, package, and inline manifest reads; - verify catalog SHA-256 values when present; - replace path-only extraction with bounded traversal/symlink-safe extraction; - validate malformed hosts and ports before opening download URLs; - handle normalized trailing-backslash directory entries consistently. Redirect enforcement and checksum verification remain owned by the shared helpers already on main; this commit wires them into extension and preset behavior. Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * harden: close archive and catalog download edge cases Preflight ZIP central directories before ZipFile allocates them, bound both declared and actual payload sizes, and reject ambiguous or non-portable archive paths before extraction. Keep extension update manifest selection consistent with extraction, reject unsafe catalog-derived output filenames and malformed URL types, and escape untrusted values in download errors. Add regression coverage for parser differentials, collisions, platform-specific filenames, bounded call sites, and failure ordering. Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * harden: address download security review feedback Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * harden: close ZIP preflight review gaps Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * fix: harden extension update preflight and rollback Assisted-by: OpenAI Codex (model: GPT-5, autonomous) * fix: harden extension update rollback Assisted-by: OpenAI Codex (model: GPT-5, autonomous) |
||
|
Noor ul ain
|
98c9e67ce2 |
fix(extensions): tolerate non-string catalog name in display-name lookup (#3747)
* fix(extensions): tolerate non-string catalog name in display-name lookup
_resolve_catalog_extension() filters catalog search results by display
name with `ext["name"].lower() == argument.lower()`. Extension catalog
JSON is user-editable, so a hand-authored non-string name (e.g.
`name: 123`) crashes the filter with `AttributeError: 'int' object has
no attribute 'lower'`, taking down `extension info <name>` and
`extension add <name>`. A missing `name` key would likewise KeyError.
Coerce defensively with `str(ext.get("name", "")).lower()`, matching the
ambiguous-match display block just below (which already str()-coerces
name for the same reason). A bad-named entry simply doesn't match,
yielding a clean not-found error instead of a traceback.
Adds a regression test invoking `extension info <name>` against a
mocked catalog whose search result has `name: 123`; it fails pre-fix
with AttributeError.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
||
|
Marsel Safin
|
683bfd00c9 |
fix: register extensions for the active integration only (#3459)
* fix: register extensions for the active integration only extension add registered commands for every detected agent, and integration upgrade back-filled enabled extensions for non-active integrations. Maintainer direction on #2948: treat the project as single-active. Only the active integration gets extension artifacts; use/switch rescaffold the target when the user selects it. - extension add now routes through the all-agents pass restricted to the active integration (only_agent), keeping detection and missing-skills-dir recovery safeguards. Projects without recorded init-options fall back to detection-based registration. - integration upgrade re-registers extensions only when upgrading the active integration, reversing the #2886 back-fill for non-active targets at maintainer request. Fixes #2948 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address review feedback on active-only extension registration - Restrict the extension-add active-integration fallback to projects with no recorded active key at all. A recorded but unsupported key (e.g. "generic", deliberately excluded from AGENT_CONFIGS) no longer falls back to registering every detected agent. - Apply the same single-active rule to preset command overrides: PresetManager._register_commands now scopes registration to the active integration via only_agent. - Add PresetManager.register_enabled_presets_for_agent, mirroring ExtensionManager.register_enabled_extensions_for_agent, and call it from integration use/switch/upgrade (active only) alongside the existing extension re-registration so presets are rescaffolded on activation instead of being written for inactive integrations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address second round of review feedback (priority order, fail-closed, docs) - register_enabled_presets_for_agent now processes presets in reverse priority order (lowest-precedence first) so the highest-precedence preset is written last and actually wins after `integration use` rescaffolds two overlapping preset command overrides. Verified this reproduces the previously reported reversed-priority bug and that the fix resolves it. - _register_commands_for_active_agent now checks for the "ai" key's presence separately from its value: a missing key still falls back to detection-based registration for all agents, but a recorded, malformed value (non-string or empty, e.g. [] or null) now fails closed (registers nothing) instead of being treated as "no active integration" or reaching AGENT_CONFIGS.get() with an unhashable key and raising TypeError. - Updated docs/reference/presets.md and docs/reference/integrations.md to describe active-only preset/extension registration and clarify that `integration use`/`switch` is the activation point for installed extensions and presets, and that `upgrade` only re-registers them for the active integration. Adds regression tests: two enabled presets overriding the same command with different priorities (priority winner must survive `use` rescaffolding), and a malformed recorded `ai` value ([]) for `extension add`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address third round of review feedback (multi-integration semantics) Fixes five deeper active-only registration bugs surfaced by Copilot review after 2486c08, all in the presets/extensions single-active integration rule (#2948): 1. presets: _reconcile_composed_commands (run after install/remove) bypassed the active-only filter entirely, writing composition-winner command files for every detected non-skill agent via register_commands_for_non_skill_agents. Added an only_agent param to that registrar method (mirroring register_commands_for_all_agents) and threaded it through all 5 reconciliation call sites. 2. presets: `integration use copilot` with --skills (ai_skills: true) wrote both the static .agent.md command file AND the SKILL.md mirror for the same override. Mirrored the extension path's ai_skills guard in both _register_commands and the reconciliation pass: a command-backed active agent running in skills mode is excluded from non-skill command registration. 3. presets: registered_skills was a flat list, so switching between two skill-mode agents (e.g. Claude -> Codex) and then removing the preset only restored the currently active agent's directory, permanently orphaning the other. _unregister_skills now restores every existing skill-mode agent directory instead of only the active one. 4. extensions: load_init_options() collapses "no file" and "corrupted file" into the same {}, so the round-2 fail-closed fix didn't actually distinguish them. Added a shared resolve_active_agent_for_registration() helper in _init_options.py that checks file existence separately from parse success, returning a distinct sentinel for "file absent" vs None for "corrupted or invalid". extensions/__init__.py now uses this helper. 5. presets: same corruption-collapsing bug in _register_commands's active_agent resolution. Now uses the same shared helper as (4). Adds regression tests for all five: reconciliation active-only filtering, copilot --skills dual-write prevention, multi-skill-agent switch+remove, and corrupted init-options fail-closed behavior for both extension add and preset add. Each test was verified to fail against the pre-fix code and pass with the fix. Targeted (883) and full (3923 passed, 109 skipped) suites pass; ruff check clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address fourth round of review feedback (skill registration provenance) Replace the "enumerate every skill-mode directory and restore all of them" approach from the previous round with precise per-agent provenance tracking, per reviewer feedback that the enumerate-and-restore-everything design was unsound: - registered_skills changes from a flat List[str] to Dict[str, List[str]] (agent name -> skill names actually written), mirroring the shape registered_commands already uses. _register_skills now returns this per-agent mapping instead of a bare list, and every call site (register_enabled_presets_for_agent, install_from_directory, the _reconcile_skills "was this skill previously managed" check) is updated to read/merge the new shape. Legacy flat-list registry entries from before this change are still readable: writes self-migrate the format, and _normalize_registered_skills() handles the transitional read paths. - _unregister_skills now restores exactly the agent directories recorded for a preset instead of guessing at every skill-mode integration that happens to exist on disk. This fixes two problems with the old enumerate-everything design: (1) it could silently overwrite or delete another preset's (or a user's) override in an agent directory the current preset never actually touched, and (2) it depended on transient per-process integration state (_skills_mode), which is unset in a fresh CLI invocation for mode-selectable integrations like Copilot --skills, permanently orphaning their overrides after a process restart. Registries written before this change (flat list, no agent provenance) fall back to best-effort restoration under only the currently active agent, matching the pre-existing guarantee level. - Every directory resolved from persisted provenance is now validated through the project's shared symlink/containment guard (_ensure_safe_shared_directory) before any file in it is read, written, or removed, since restoration may target an agent that isn't currently active and its directory can't be assumed safe just because a name was recorded for it. - _tracked_skill_agent_dirs() (the enumeration helper introduced last round) is removed; it's superseded by the provenance-based design. Adds regression tests: a symlinked skills directory is rejected during removal; removing one preset does not disturb a different preset's override in another agent's directory; and a Copilot --skills registration installed, then removed after switching agents in a fresh PresetManager instance (simulating a new process), is still correctly restored. Updates existing skill-registration assertions across test_presets.py and test_integration_claude.py for the new per-agent registry shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address fifth round of review feedback (symlink presence, rescaffold reconciliation, shared skills dir) - _init_options.py: resolve_active_agent_for_registration() now treats a dangling init-options.json symlink as present (path.is_symlink() check alongside path.exists()), since Path.exists() follows symlinks and returns False for a broken one. Previously a broken symlink fell back to the legacy "no file" path and registered every detected agent instead of failing closed. - presets/__init__.py (register_enabled_presets_for_agent): the integration use/switch rescaffold path now collects affected command names across all presets processed and runs _reconcile_composed_commands/_reconcile_skills once after the loop, matching install/remove. Previously rescaffolding wrote each preset's raw content directly with no follow-up reconciliation, so a project-level override (the highest-priority layer) could be clobbered by a lower-precedence preset after switching agents. - presets/__init__.py (_unregister_skills): multiple integrations can share one physical skills directory (agy/codex/zed all resolve to .agents/skills). Provenance restoration now groups recorded agent entries by resolved directory and restores each physical directory exactly once, preferring the currently active agent's renderer when it owns that directory (otherwise any recorded owner, chosen deterministically). Previously each recorded agent key triggered its own restore pass against the same directory, with whichever agent was iterated last silently winning regardless of which agent was active. Adds regression tests for each: a dangling init-options.json symlink failing closed for both preset resolution and extension add; integration use rescaffold preserving a project override over a lower-priority preset; and a codex/agy shared-directory removal restoring the directory exactly once in the active agent's format. Targeted (tests/integrations/test_integration_subcommand.py, tests/test_presets.py, tests/test_extensions.py, tests/test_extension_skills.py, tests/integrations/test_integration_opencode.py, tests/integrations/test_integration_claude.py): 930 passed. Full suite: 3930 passed, 109 skipped. ruff check: clean on files touched by this change. Refs #2948 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: guard skill subdirectories and active-agent scoping in preset reconciliation Fix 4 issues from round-6 review of the active-only integration registration work (#2948): - remove(): removed_cmd_names only collected primary command names from registered_commands + manifest aliases, missing commands that were only ever registered via skills mode (ai_skills guard returns no command names for command-backed integrations in skills mode). This skipped reconciliation entirely when removing a higher-priority skills-mode preset, causing _unregister_skills() to fall back to core/extension content instead of the surviving lower-priority preset's override. Now every command template's primary name is added to removed_cmd_names unconditionally. - _reconcile_composed_commands(): the "composed is None" branch (fires when no replace-strategy layer remains for a command, e.g. after removing a wrap/append preset's base) called unregister_commands() across every configured non-skill agent, ignoring only_agent. This deleted historical artifacts from integrations that were never active for the preset. Now filtered by only_agent like the rest of the file. - Added _validate_skill_subdir() helper (reusing _ensure_safe_shared_directory/_validate_safe_shared_directory from shared_infra.py) and applied it at every site that reads or writes an individual skill subdirectory (_register_skills, _unregister_skills_in_dir, _reconcile_skills' override_skills restoration loop). _safe_skills_dir_for_agent only validated the parent skills directory; a symlinked leaf subdirectory (e.g. .claude/skills/speckit-specify) would slip past that check since is_dir()/exists() follow symlinks, letting write_text/rmtree operate through it to an arbitrary location outside the project. Added regression tests: removing a higher-priority skills-only preset restores the surviving lower-priority preset's content; composed-is-None unregistration only touches the active agent; symlinked skill subdirectory rejected on restore; symlinked skill subdirectory rejected on write. Targeted (934) and full (3934 passed, 109 skipped) test suites and ruff check pass clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: persist command registration before fallible skills phase on rescaffold Fix remaining round-6 review findings on the active-only integration registration work (#2948): - register_enabled_presets_for_agent(): registered_commands and registered_skills were merged and persisted together in a single registry.update() call after both the commands and skills phases ran. If _register_skills() raised, the per-preset try/except swallowed it before that update() call was reached, even though _register_commands() had already written a real command file to disk. That file became untracked, so preset removal could no longer clean it up. install_from_directory() already persists registered_commands immediately after the commands phase, before starting the independently fallible skills phase; rescaffold now does the same. - test_presets.py: renamed a misleading claude_dir variable (pointing at Gemini's command directory) in test_composed_none_unregister_respects_active_agent to reuse the existing gemini_commands_dir variable already defined earlier in the same test. Added regression test test_rescaffold_persists_commands_before_fallible_skills_phase: simulates a skills-phase failure during rescaffold and asserts the command file already written to disk is still tracked in registered_commands. Verified all other round-6 findings (preset active-integration scoping, preset reconciliation/remove paths, skills-mode switching, override precedence during rescaffold, skill-subdirectory symlink safety) are already addressed by prior commits in this branch; re-checked each against current code before concluding no further change was needed. Targeted (tests/test_presets.py, tests/test_extensions.py: 689 passed) and full (3935 passed, 109 skipped) suites and ruff check on changed files pass clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: unregister stale opposite-mode preset artifact on same-agent skills toggle Fix an Important gap in register_enabled_presets_for_agent() surfaced by quality review (#2948): toggling ai_skills for the *same already-active* command-backed agent (e.g. `integration upgrade copilot` after flipping ai_skills, with copilot staying active throughout) left a stale artifact from the previous mode behind, violating the command/skill mutual- exclusion invariant this PR otherwise enforces. - command -> skills: _register_commands()'s ai_skills guard makes the commands phase a no-op, but the previously-written command file (e.g. .agent.md) and its registered_commands[agent] entry were never cleaned up, so it lingered alongside the newly written SKILL.md. - skills -> command: _get_skills_dir() stops resolving a skills directory once ai_skills is off, making the skills phase a no-op, but the previously-written SKILL.md and its registered_skills[agent] entry were never cleaned up, so it lingered alongside the newly (re)written command file. register_enabled_presets_for_agent() now resolves once per call whether agent_name is a command-backed integration (extension != "/SKILL.md") and the current ai_skills state, then narrowly unregisters the stale opposite- mode entry for that agent via the existing _unregister_commands / _unregister_skills helpers before persisting updated tracking — mirroring the same per-agent, per-preset isolation already used elsewhere in this method. Native skill-only agents (claude, codex, ...) are unaffected: they have no command/skill toggle, so registered_commands and registered_skills legitimately co-exist for them by design. The trailing reconciliation pass, project-override precedence, and per-preset partial-failure isolation are all unchanged. Added red-first regression tests exercising the real install + register_enabled_presets_for_agent rescaffold path in both toggle directions: - test_rescaffold_toggle_command_to_skills_removes_stale_command_file - test_rescaffold_toggle_skills_to_command_removes_stale_skill_file Both failed against the prior code (stale artifact persisted / registry still tracked it) and pass after the fix. Targeted (tests/test_presets.py, tests/test_extensions.py: 691 passed) and full (3937 passed, 109 skipped) suites and ruff check on changed files pass clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: migrate legacy flat-list registered_skills on rescaffold even when unchanged Fix a valid finding from GitHub Copilot's review of HEAD b9d9053 (#2948): register_enabled_presets_for_agent() normalizes a legacy flat-list registered_skills value (predating per-agent provenance) to the {agent_name: [...]} dict shape in memory via _normalize_registered_skills, but the persistence check only compared the two *normalized* forms. When the freshly rescaffolded skill names are identical to what the legacy list already held — the common case, since nothing about the preset or skill actually changed — that comparison is a no-op and registry.update() is skipped, leaving the *raw* on-disk value as the un-migrated flat list. A later switch to a different skill-mode agent and removal then follows _unregister_skills's legacy best-effort path (restore only the currently active agent's directory) instead of the per-agent provenance path, permanently orphaning the first agent's override. Fix: track the raw (pre-normalization) existing value and force persistence whenever it's a non-empty list, independent of whether the normalized content changed. Traced registered_commands for the same class of bug: its registry value has always been Dict[str, List[str]] (no legacy flat-list format ever existed for it — the existing `if not isinstance(existing_commands, dict): existing_commands = {}` guard is not a lossy migration path), so this fix stays scoped to registered_skills only. Added red-first regression test test_rescaffold_migrates_legacy_flat_list_registered_skills: installs a preset, overwrites its registry entry with a raw legacy flat list, rescaffolds the *same* active agent with unchanged skill names, and asserts the raw registry is migrated to per-agent dict form. Extends the scenario with a switch to a second skill-mode agent and preset removal to prove both agents' directories restore cleanly instead of orphaning the first. Failed against the prior code (raw value stayed a list) and passes after the fix. Targeted (tests/test_presets.py, tests/test_extensions.py: 692 passed) and full (3938 passed, 109 skipped) suites and ruff check on changed files pass clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reconcile before fallible skills phase, infer legacy skill provenance, and unregister stale extension artifacts on toggle Three findings from the Copilot review on HEAD b9d9053/3a1e749: 1. `register_enabled_presets_for_agent()` only recorded a preset's command names into `affected_cmd_names` (the set later passed to `_reconcile_composed_commands`/`_reconcile_skills`) in the loop that ran *after* `_register_skills()`, inside the same per-preset `try` block. If `_register_skills` raised, the `except` caught it and `continue`d before that loop ever ran — so a preset whose commands phase already wrote real content to disk never got reconciled against the full priority stack, leaving its raw content in place instead of a project override or higher-precedence preset's content. Fix: record the manifest's command names immediately after the commands phase succeeds and persists, before calling the independently fallible `_register_skills()`. 2. The legacy flat-list `registered_skills` migration (added for the previous review round) attributed every name in the list to whichever agent was currently being (re)activated. If the first operation after upgrading from a pre-#2948 registry was a direct switch to a *different* skill-mode agent (e.g. a legacy Claude override, then `integration use codex` with no intervening Claude rescaffold), the migrated dict only recorded `{"codex": [...]}`, permanently losing Claude's actual provenance and orphaning its override on later removal. Fix: added `_infer_legacy_skill_provenance()`, which probes every configured skill-mode agent's directory (via the same safe, symlink-validated helpers already used for restore/removal) for a `SKILL.md` whose frontmatter records this exact preset as the owner (`metadata.source == "preset:<pack_id>"`). A name found under more than one directory is attributed to every matching agent (the preset may have been active while the user switched between several skill-mode agents before provenance tracking existed); names that can't be matched to any directory still fall back to the previously-active best-effort behaviour. Directory grouping for shared-path aliases (e.g. agy/codex/zed all resolving to `.agents/skills`) intentionally does not call `.resolve()` on the path, since doing so diverges from `project_root`'s own resolution state on platforms where a path component is itself a symlink (e.g. macOS's `/var` -> `/private/var`) and made every subsequent containment check spuriously fail. 3. `register_enabled_extensions_for_agent()` has the same command/skill mutual-exclusion gap the preset path had (fixed in a previous round): toggling `ai_skills` for the *same active* agent left the opposite mode's artifact behind. Command -> skills left the extension's `.agent.md` file and its `registered_commands[agent]` entry in place once `skills_mode_active` made the commands phase a no-op. Skills -> command left the extension's `SKILL.md` file in place, since an empty `_register_extension_skills()` result (because this agent's skills directory no longer resolves once `ai_skills` is off) was treated as "nothing to register" rather than "this was rendered here before and is now stale". This diverges from the preset path in one respect: `registered_skills` for extensions has always been a flat list with no per-agent provenance (extension skills are only ever rendered for the active agent, never per-preset-per-agent tracked), so the fix resolves ownership by checking which of the extension's tracked skill names still exist as directories under this specific agent's directory before removing them — mirroring the same technique `unregister_agent_artifacts` already uses for full agent deactivation, but scoped narrowly to firing only when a toggle is actually detected (`skills_mode_active` / `command_mode_active`), so a same-mode re-run never disturbs already-correct artifacts or a user's manual customizations. Regression tests (all confirmed red before their respective fix, green after): - tests/test_presets.py::TestPresetSkills::test_rescaffold_reconciles_override_even_when_skills_phase_fails - tests/test_presets.py::TestPresetSkills::test_rescaffold_legacy_flat_list_direct_switch_preserves_original_agent - tests/test_extension_skills.py::TestExtensionSkillRegistration::test_rescaffold_toggle_command_to_skills_removes_stale_extension_command_file - tests/test_extension_skills.py::TestExtensionSkillRegistration::test_rescaffold_toggle_skills_to_command_removes_stale_extension_skill_file Verification: tests/test_presets.py + tests/test_extensions.py + tests/test_extension_skills.py (753 passed), tests/integrations/ (1768 passed, 1 skipped), full suite `pytest tests -q` (3942 passed, 109 skipped), `ruff check` on changed files clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: broaden legacy skill provenance inference to command-backed agents _infer_legacy_skill_provenance() only probed agents whose registrar config statically declares extension == "/SKILL.md", excluding command-backed agents (e.g. Copilot) that can also render preset overrides as SKILL.md files when ai_skills is enabled. A real preset-owned .github/skills/.../SKILL.md written while Copilot was the active skills-mode agent was therefore never probed and got misattributed entirely to whichever agent activated first after the upgrade, permanently orphaning Copilot's override on later removal. Broaden the candidate set to every configured integration (CommandRegistrar.AGENT_CONFIGS), reusing the existing safe-path helper (_safe_skills_dir_for_agent, itself built on the shared _get_skills_dir resolver) rather than inventing new path-construction logic. The existing preset-marker match (metadata.source == "preset:<pack_id>") continues to gate every attribution, so command-mode agents that never rendered this preset's skill are not falsely attributed. Add red-first regression tests: a legacy flat-list entry owned by Copilot in skills mode, switched directly to Claude with no intervening Copilot rescaffold, now migrates to a per-agent dict covering both agents, and removal restores both agents' files instead of orphaning Copilot's override; plus a negative-case test confirming a command-mode Copilot with no preset-owned skill marker is not falsely attributed during the same migration. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: preserve extension skill tracking for mirrors in other agent dirs The skills -> command toggle cleanup in register_enabled_extensions_for_agent() recomputed the remaining tracked registered_skills names by checking only the toggling agent's own skills directory. Since registered_skills is a single flat list shared across every agent an extension has ever been activated under (skills are only ever rendered for the active agent, so there is no per-agent registry key), a name whose mirror still existed under a *different*, previously-active agent's directory was incorrectly dropped from tracking as soon as the current agent's own copy was removed. A later full removal only iterates registered_skills, so the orphaned mirror under the other agent's directory was never found or cleaned up. Add _extension_owned_skill_names(), which re-verifies ownership across every configured agent's skills directory (deduped by shared path) the same way the existing _unregister_extension_skills() fallback scan already does, keeping a name only when a SKILL.md with a matching metadata.source == "extension:<id>" marker is found somewhere - read-only, no directory creation, no symlink escape. Use it instead of re-checking only the toggling agent's own directory when recomputing what remains tracked after narrow stale-mirror cleanup. Add a red-first regression test: Auggie is activated in skills mode first (writing a mirror), then Copilot is activated in skills mode (writing its own mirror for the same names), then Copilot toggles to command mode. Before the fix, registered_skills lost both names entirely even though Auggie's mirrors were untouched on disk; after the fix tracking is preserved and a subsequent full removal correctly cleans up Auggie's remaining mirrors too. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reject symlinked skills-directory escape in extension skill scans _extension_owned_skill_names() and the fast/fallback paths of its sibling _unregister_extension_skills() called skills_candidate.resolve() and then checked children relative to that already-resolved candidate. If the candidate directory itself (e.g. .gemini/skills) was a symlink pointing outside the project root, both the resolve() call and the subsequent containment check silently passed through the symlink instead of rejecting it: - _extension_owned_skill_names() would falsely attribute ownership to a marker-matching SKILL.md living outside the project. - _unregister_extension_skills()'s fast path (an explicit skills_dir, as passed by the toggle-cleanup call site) and its fallback scan (used during full extension removal) would both shutil.rmtree() the external directory, deleting unrelated content outside the project. Fix by validating the candidate directory itself with the existing _validate_safe_shared_directory() shared-infra helper before any probe or delete: it rejects a symlink at any path component (walking down from the project root, including the final component) without ever resolving through it, and is already used elsewhere in the codebase for the same class of shared-directory containment check. Unsafe candidates are skipped/refused rather than followed. Add red-first security regression tests reproducing each of the three call sites with a `.gemini/skills` symlink pointing at an external directory containing a marker-matching SKILL.md and an unrelated precious_file.txt: provenance inference must not attribute the name, and both the explicit-skills_dir fast path and the None-skills_dir fallback scan must leave the external directory and file untouched. Existing valid shared/deduped directory tests (e.g. agy/amp/codex/zed sharing .agents/skills) continue to pass, confirming legitimate shared directories still clean up correctly. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix unscoped extension-skill removal and legacy preset provenance on direct remove - _unregister_extension_skills(): omitting skills_dir now always triggers the full multi-directory fallback scan instead of narrowing to the currently active agent's directory. Previously, remove() (the only caller that omits skills_dir) would resolve the active agent's dir and take the scoped fast path, orphaning a previously-active second agent's extension skill mirror during full removal. - PresetManager.remove(): infer legacy flat-list registered_skills provenance (reusing _infer_legacy_skill_provenance from the prior rescaffold fix) before invoking _unregister_skills, so a direct `preset remove` with no intervening rescaffold/switch also restores every previously-active agent's directory instead of only the currently active one. Added regression tests: - test_remove_while_second_agent_still_in_skills_mode_cleans_up_first_agent_mirror - test_remove_infers_legacy_flat_list_provenance_without_prior_rescaffold Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Keep unregister_agent_artifacts scoped to its agent when directory is absent ExtensionManager.unregister_agent_artifacts() converted its resolved agent_skills_dir to None whenever that directory didn't exist, before calling _unregister_extension_skills(). After 1d8f9e3, omitting skills_dir means "genuinely unscoped removal": scan every configured agent's directory, reserved for ExtensionManager.remove()'s full project cleanup. Since unregister_agent_artifacts is agent-scoped (used by switch to clean up the previous integration's artifacts), this caused it to delete every other agent's live extension skill mirrors whenever the target agent's own directory happened to be absent, e.g. unregistering an agent that was never activated. Fix: always pass the explicit, agent-scoped skills_dir, even when it doesn't exist on disk, so the fast path is a safe no-op for an absent directory instead of falling back to the all-agents scan. Registry reconciliation (dropping removed names from the flat registered_skills list) now only runs when the agent's directory actually exists, so an absent directory can't be misread as "these names were removed everywhere" and wipe tracking for mirrors that still legitimately live under other agents' directories. Added regression test: - test_unregister_agent_artifacts_stays_scoped_when_agent_dir_absent Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Preserve global skill tracking across agents in unregister_agent_artifacts The present-directory branch of ExtensionManager.unregister_agent_artifacts() recomputed "remaining" registered_skills only by checking whether each name still existed under the just-cleaned agent's own directory. registered_skills is a single flat list shared across every agent an extension was ever activated under (skills are only ever rendered for the currently active agent, so there's no per-agent registry key). Repro: auggie and copilot both have mirrors for the same extension; unregister_agent_artifacts("auggie") correctly removes auggie's own mirror, sees the names absent from auggie's (now empty) directory, and stores an empty registered_skills list - even though copilot's mirror is still live on disk and now untracked. A later full remove() then reads an empty registry and leaves copilot's mirror orphaned. Fix: after the agent-scoped cleanup, recompute remaining names with _extension_owned_skill_names(), which scans every safe, configured agent skills directory (not just the one just cleaned) and keeps a name only if a marker-verified SKILL.md for this extension still exists somewhere. This is the same helper already used for the analogous same-agent toggle-cleanup case, so no new abstraction was introduced. Explicit per-agent cleanup, marker ownership verification, and symlink/containment safety are unchanged. Added regression test: - test_unregister_agent_artifacts_preserves_tracking_for_other_agent_mirror Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Reconcile every historical agent on preset removal; validate child skill dirs Fixes 3 findings from the Copilot review on HEAD 31c9b97 (#2948): 1. presets/__init__.py: remove()'s command reconciliation only recreated the surviving preset's content for the currently active agent, even though the removed preset's registered_commands could span multiple historical (now-inactive) agents recorded via prior rescaffolds. Now remove() captures every historical agent registered_commands actually targeted (before mutation) and passes it as extra_agents through _reconcile_composed_commands -> _register_for_non_skill_agents / _register_command_from_path -> registrar.register_commands_for_non_ skill_agents, so the active-only restriction for install/use is preserved while post-removal reconciliation restores every touched directory. 2. presets/__init__.py: the analogous gap existed for skills. _unregister_ skills() now returns {skills_dir: renderer_agent} for every directory it actually restored, and _reconcile_skills() accepts extra_skills_dirs to reconcile each of those directories (via a new apply_to_dir() helper), not only the currently active skills directory. _register_skills() gained optional target_dir/target_agent overrides (forcing create_missing_skills off for non-active directories) so a historical directory is only ever restored, never seeded with brand-new skills. 3. extensions/__init__.py: _extension_owned_skill_names() and both the fast and fallback paths of _unregister_extension_skills() validated only the parent skills_dir for symlink escape, then resolved skills_dir / skill_name and checked containment relative to that already-resolved parent. A per-skill child that is itself a symlink to a different, legitimate skill directory within the same (safe) root passed that containment check, so deleting/attributing through the symlink name could destroy or misattribute an unrelated skill reached only via the alias. All three call sites now run the shared _validate_safe_shared_directory() component-wise check against the full skills_dir / skill_name path (not just the parent) before any read or delete, rejecting a symlinked child outright rather than following it, even when its resolved target remains in-bounds. Regression tests added (all confirmed red against pre-fix code, green after): - test_remove_reconciles_command_for_every_historical_agent - test_remove_reconciles_skill_for_every_historical_agent - test_extension_owned_skill_names_rejects_symlinked_child_skill_dir - test_unregister_extension_skills_explicit_dir_rejects_symlinked_child - test_unregister_extension_skills_fallback_rejects_symlinked_child Tests: tests/test_presets.py (361), tests/test_extension_skills.py (69), tests/test_extensions.py (338) all pass; tests/integrations (1768 passed, 1 skipped) pass; full suite 3902 passed / 74 skipped (90 pre-existing, environment-only git-signing tests deselected — confirmed failing identically on the pre-change baseline due to local 1Password SSH-agent signing, unrelated to this change). ruff check clean on all changed files. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Persist historical reconciliation ownership; defer destructive toggle cleanup; validate registry-provided skill names Round 11 review findings (5 comments on HEAD ab6c28c), three root causes: A) Historical-agent reconciliation wrote surviving content to disk but discarded the returned per-agent write map, so the preset's own registered_commands/registered_skills never learned about directories reconciliation restored on its behalf. A later removal of that same preset then orphaned those directories. Added _merge_pack_registered_commands/_merge_pack_registered_skills and wired them into _reconcile_composed_commands and _reconcile_skills's apply_to_dir so every actual write is merged back into the winning preset's registry metadata. B) Command<->skills toggle on an already-active agent deleted the old artifact before the replacement registration ran, in both presets/__init__.py's register_enabled_presets_for_agent and extensions/__init__.py's register_enabled_extensions_for_agent. If the replacement step raised, both artifacts were lost. Deferred the destructive cleanup until after the replacement phase completes without raising (register-new-then-remove-old ordering); the mirror skills->command direction was already safe since the new command file is always registered unconditionally before any cleanup runs. C) _unregister_skills_in_dir and _infer_legacy_skill_provenance joined a registry-provided (untrusted) skill name directly onto a directory before any name-shape validation. An absolute in-project name discards the intended parent directory entirely (Path's "/" operator drops the left side for an absolute right side), letting a corrupted registry entry escape the intended skills subtree while still resolving inside the project root - passing the existing containment/symlink check. Added a centralized _is_safe_registry_skill_name guard (rejecting non-strings, empty strings, absolute paths, multi-component paths, and "."/".." ) and applied it before every path join derived from registry-provided skill names in both functions. Also fixed _infer_legacy_skill_provenance's unmatched-name fallback, which previously still attributed rejected names to fallback_agent even after the loop skipped them. Added red-first regressions for all three root causes, covering: a two-preset historical-command-agent survivor scenario, an analogous skill-agent survivor scenario, injected skills-phase failure during a preset command->skills toggle and the extension equivalent, a direct unit test of the new name-safety guard, an absolute-path escape attempt against _unregister_skills_in_dir, and a false-attribution attempt against _infer_legacy_skill_provenance. Tests: tests/test_presets.py (367 passed), tests/test_extension_skills.py + tests/test_extensions.py (408 passed), tests/integrations (1768 passed, 1 skipped), full suite tests -q deselecting the pre-existing 1Password-signing-affected tests/extensions/git/test_git_extension.py (3909 passed, 74 skipped, 90 deselected). ruff check clean on all changed files. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Verify replacement actually landed before retiring stale toggle artifacts The command<->skills toggle cleanup added for #2948 deferred destructive removal of the old-mode artifact until after the replacement registration call completed without raising. That was necessary but not sufficient: none of _register_skills(), _register_commands(), register_commands_for_agent(), or _register_extension_skills() raise on a missing source template, a safety-validation skip, or a corrupted manifest entry — they simply return an empty or partial result. Treating "did not raise" as "fully replaced" meant a stale artifact could still be deleted (or its tracking dropped) even though its specific replacement never actually landed, leaving neither artifact in place for that logical command/skill. Fix all four affected toggle directions by checking the replacement call's actual return value before allowing any destructive step: - presets command->skills (register_enabled_presets_for_agent): only unregister a stale command name once its corresponding skill name (via the existing _skill_names_for_command() helper) is confirmed present in the skills call's returned names for that agent; the remainder stays tracked and on disk. - presets skills->command (register_enabled_presets_for_agent): only unregister a stale skill name once its corresponding command name is confirmed present in the commands call's returned names for that agent, using the same helper. - extensions skills->command (register_enabled_extensions_for_agent): only remove a skill mirror once the matching command (mapped via the existing HookExecutor._skill_name_from_command() helper) is confirmed present in register_commands_for_agent's returned names. - extensions command->skills (register_enabled_extensions_for_agent): only remove a deferred stale command once its matching skill name is confirmed present in _register_extension_skills()'s returned names. All four reuse the existing command<->skill name-derivation helpers rather than inventing new mapping logic. Registry tracking is updated to retain exactly the unreplaced subset rather than being popped wholesale, so partially-successful toggles leave correct, minimal tracking behind. Added 8 new regression tests (4 presets, 4 extensions) covering both the fully-empty and genuinely-partial result cases for each of the four toggle directions, using real missing-source-file scenarios (not mocked return values) to exercise the actual code paths. Confirmed red before the fix and green after for all 8. Focused (test_presets.py, test_extension_skills.py, test_extensions.py, tests/integrations): 2551 passed, 1 skipped. Full suite (tests, excluding the pre-existing environment-local 1Password-signing git-extension failures): 3917 passed, 74 skipped, 90 deselected. ruff check: clean. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Retire alias command groups on toggle; scope preset cleanup to switched-away agent (#2948) Fixes three current Copilot review findings on HEAD d0d152e: 1. Command->skills toggle cleanup only matched a stale command's own name against the returned replacement skill name. Aliases (CommandRegistrar tracks and returns primary + alias names flattened into one list) never have their own skill rendered -- only the primary command's skill is rendered -- so an alias's name could never match, leaving its command artifact and tracking behind forever even after the primary's replacement landed. Fixed identically in both presets (register_enabled_presets_for_agent) and extensions (register_enabled_extensions_for_agent): build a primary->alias mapping from the manifest, group stale names by primary, and retire/keep the whole group together based solely on whether the primary's skill replacement actually landed. 2. `integration switch` to a not-yet-installed target unregistered the old agent's extension artifacts but had no preset equivalent, so a preset's command overrides (including custom preset commands) and skill mirrors for the deactivated agent lingered as orphans. Added `PresetManager.unregister_agent_artifacts()`, mirroring `ExtensionManager.unregister_agent_artifacts()`: scoped strictly to the given agent, migrates a legacy flat-list `registered_skills` entry via existing on-disk provenance inference before removing anything (so other agents' real ownership is preserved rather than guessed or dropped), and guards against double-processing an artifact through both the commands and skills paths for native SKILL.md agents. Wired via a new `_unregister_presets_for_agent()` helper into the integration switch command's existing old-agent cleanup phase. Added red-first regression tests: - tests/test_presets.py: alias-group retire/keep/partial-multi-group tests for the command->skills toggle; unregister_agent_artifacts scoping tests for commands and legacy-list skill provenance. - tests/test_extension_skills.py: alias-group retire/keep tests for the extension command->skills toggle. - tests/integrations/test_integration_subcommand.py: end-to-end switch test proving a preset's custom command override is cleaned up when switching to a not-yet-installed integration, with tracking updated correctly and the new agent's registration unaffected. All new tests confirmed red (AttributeError / orphaned file assertions) before the fix and green after. Full suite: 3980 passed, 109 skipped. ruff check clean on all changed files. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: track reconciled extension artifacts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix native skill preset reconciliation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix shared native skill cleanup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix partial preset rescaffold tracking Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix preset agent skill lifecycle Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clarify preset removal reconciliation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): address upgrade review feedback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(presets): reconcile partial command writes Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address active artifact cleanup review Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: defer preset skill cleanup to winning command Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: track reconciled and partial preset skills Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reconcile project overrides to legacy skills Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: harden preset skill writes and rollback Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(presets): harden legacy skill restoration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(presets): preserve non-owned legacy skills Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: validate reconciled skill paths Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(presets): preserve reconciled skill ownership Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(presets): clean reconciled agent skills Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: keep legacy cleanup project-local Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(presets): keep active agent's artifacts in its current mode on remove A partially failed command<->skills toggle leaves stale tracking (registered_commands or registered_skills) for the active agent, and remove() replayed that history regardless of the agent's current mode: - extra_agents re-admitted the active skills-mode agent into command reconciliation, recreating its command file from a surviving lower preset even though only_agent excluded it. - _unregister_skills restored (and _reconcile_skills reapplied) a skill artifact for the active command-mode agent instead of deleting the preset-owned leftover. The active agent's participation is now decided exclusively by its current mode: reconciliation strips it from extra_agents, and removal routes its stale skills through _delete_agent_preset_skills. Historical replay still applies to inactive agents only (#2948). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: filter uninstalled-extension commands in reconciliation; allow active-agent layout change with presets Two follow-ups to the upstream-main merge: - Preset reconciliation (_reconcile_composed_commands) now skips extension-scoped commands (speckit.<ext>.<cmd>) whose extension is not installed, at the single chokepoint every install/remove/rescaffold pass funnels through. Registration already refused them, so reconciliation could materialize files no registry entry tracks. The duplicated per-call-site filters collapse into one _extension_installed_for_command helper. - The #3415 layout-change guard predates this PR's agent-scoped preset rescaffold: for the active integration, _register_presets_for_agent now re-registers enabled presets in the new layout and retires the old layout's stale files, so an active-agent command<->skills toggle proceeds and reconciles instead of being rejected. The guard still rejects non-active agents (no rescaffold runs for them) and still fails closed on an unreadable registry. _installed_presets_affecting_agent also understands the per-agent dict shape of registered_skills this PR writes, instead of raising 'malformed'. Regression tests: rescaffold with an uninstalled extension's command, CLI-level legacy<->skills toggle with an installed preset (both directions), secondary-agent rejection, and dict-shaped registered_skills in the guard helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reject active layout change while a disabled preset owns artifacts The post-upgrade preset rescaffold iterates enabled presets only, and a disabled preset's artifacts are deliberately frozen until removal, so an active-agent command<->skills layout change cannot reconcile them. _installed_presets_affecting_agent now reports each preset's enabled state and the guard rejects the migration while any affected preset is disabled, with re-enable/remove guidance. Enabled presets and non-active rejection behave as before. Regression test: disabled preset blocks the toggle untouched; re-enabling unblocks it and reconciles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: replace placeholder prefix in two safety comments Comment-only: spell out why skill deletion is restricted to project-local directories (flat/legacy provenance cannot prove home-directory ownership) instead of an undefined placeholder word. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: correct guard-helper docstring to active-only registration model Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: fail closed on non-list values in per-agent preset provenance A dict-shaped registered_skills/registered_commands entry with a non-list value (e.g. null) left ownership undecidable but read as "no artifacts", letting a layout-changing upgrade proceed on a malformed registry. Validate values are lists and raise _PresetRegistryUnreadableError otherwise, matching the guard's fail-closed contract. Unit test covers both fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: drop eager extension unregister on layout-changing upgrade Unregistering the agent's extension artifacts before re-registration deleted files and registry tracking up front, so a failed or partial re-registration left the extension with no artifacts at all. Retirement of each opposite-mode artifact already belongs to register_enabled_extensions_for_agent's deferred toggle cleanup, which removes an old artifact only after its replacement is confirmed. Also keeps disabled extensions consistent with disabled presets: artifacts stay frozen in place with intact tracking. Regression test corrupts the installed extension manifest so re-registration fails, then asserts the old-layout artifacts and their registry tracking survive the upgrade. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: rescaffold fallback integration after failed switch rollback When Phase 2 of a switch fails, rollback restores another installed integration as the default via _set_default_integration but never re-registered extensions or presets for it. Under active-only registration the fallback may never have received any artifacts (it was installed while another integration was active), and Phase 1 already unregistered the outgoing agent's artifacts — leaving the restored default unusable. Rescaffold both extensions and presets (best-effort) after the fallback default is successfully restored. Regression test: secondary codex install with the git extension, a failing switch to generic, then asserts codex ends up with registered extension artifacts after rollback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs: explain load-bearing pre-create loop in _reconcile_skills The per-skill _validate_skill_subdir(create=True) loop looks like dead code (its result is unused), but it re-creates the tracked skill subdirectories that _unregister_skills just deleted so _register_skills's only-overwrite-existing gate passes during a historical-directory restore. Removing it fails test_skill_reconciliation_preserves_per_directory_names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: preserve dashed-description skill tracking Use the shared frontmatter parser when verifying surviving extension skill mirrors so delimiter substrings cannot hide provenance metadata. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: skip absent extension skills during reconciliation Filter extension-scoped commands before skill reconciliation so historical preset tracking and project overrides cannot recreate artifacts for uninstalled extensions. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: preserve partial native skill cleanup Coordinate native-skill command cleanup with registered skill coverage per agent and command so partial rescaffolds cannot orphan preset artifacts. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Noor ul ain
|
42c7230aa9 |
fix(extensions): tolerate non-string tags in catalog search (#3746)
* fix(extensions): tolerate non-string tags in catalog search ExtensionCatalog.search() assumed catalog `tags` were always strings: the tag filter called `t.lower()` and the query path did `" ".join([...] + tags)`. Extension catalog JSON is user-editable, so a hand-authored `tags: [1, 2]` crashed search with AttributeError (tag filter) or TypeError (query join). Coerce defensively by filtering to `isinstance(t, str)` and guarding the tags value as a list, matching the reference-correct sibling in integrations/catalog.py. Non-string tags are now skipped rather than raising. Adds a regression test driving search(tag=...) and search(query=...) against a catalog with mixed string/int tags; both fail pre-fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(extensions): also coerce non-string author/name in catalog search The same ExtensionCatalog.search() method had two more string assumptions on user-editable catalog fields: the author filter called `ext_data.get("author", "").lower()` (AttributeError on a numeric author) and the query searchable-text joined `name`/`description` uncoerced (TypeError on a numeric name). Coerce both defensively, matching the reference-correct integrations/catalog.py::search. Extends the regression test with non-string author/name coverage; fails pre-fix with AttributeError at the author filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Quratulain-bilal
|
e6a3ccfb27 |
fix(extensions): hyphenate command names in 'extension info' listing (#3744)
The 'Commands:' section of 'specify extension info' for a locally installed extension printed each command in its manifest dotted form (e.g. speckit.jira.sync). Cline and Forge register hyphenated command names (/speckit-jira-sync), so on those projects the displayed names did not match what the user actually invokes. Format each name through the active agent's command-name formatter, mirroring the parity 'extension add' already applies to its 'Provided commands' listing (#3669) and completing the Forge/Cline command-name parity from #3641/#3642. Adds a regression test asserting the hyphenated form appears (and the dotted form does not) for a Forge project. |
||
|
Ali jawwad
|
c1028e5506 |
fix(extensions): guard non-numeric catalog downloads in search/info rendering (#3710)
* fix(extensions): guard non-numeric catalog downloads in search/info rendering `specify extension search` and `specify extension info <id>` format a catalog entry's `downloads` field with the `:,` thousands separator, guarded only by `is not None`. Catalog payloads are only shape-validated -- individual fields are never type-checked and `_get_merged_extensions` returns raw catalog dicts -- so an entry with a non-numeric `downloads` (e.g. the JSON string "1500", realistic from a community / SPECKIT_CATALOG_URL / project catalog) makes the `:,` format raise `ValueError: Cannot specify ',' with 's'`, aborting the whole command with an uncaught traceback. Group-format `downloads` only when it is actually numeric; otherwise render it as-is. Numeric values (int/float, incl. bool) format identically, so correct catalogs are byte-for-byte unchanged. Every other field in these two renderers is already `str()`-wrapped; this closes the one unguarded field. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(extensions): escape the non-numeric downloads fallback for Rich markup Address review feedback: the fallback interpolated the untrusted catalog value straight into a Rich-rendered string, so guarding the ``:,`` ValueError just traded it for a MarkupError -- a catalog entry with downloads "[/red]foo" still aborted `extension search`/`info`, and balanced tags could restyle the output. Wrap the fallback in _escape_markup(str(...)) at both sites, matching how every other catalog field in these renderers is already escaped. Numeric values keep the identical ``:,`` branch, so correct catalogs are unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(extensions): escape 'stars' too, in the same stats string Follow-up to the downloads escaping: `stars` is the other catalog-controlled value joined into the same Rich-rendered stats line, and it was still raw -- verified that stars "[/red]x" raises the same MarkupError and aborts `extension info`/`search`. Hardening one of the two adjacent values would have left the reported defect reachable through the sibling field. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Oscar
|
2fb94e0f9c |
fix(extensions): make shipped scripts executable after install (#3723)
Extension archives are unpacked with zipfile.extractall and directory installs are copied; neither restores a stripped Unix mode. A bundled *.sh therefore lands non-executable, so a documented `.specify/extensions/<id>/scripts/bash/foo.sh` invocation fails with "Permission denied" — e.g. a CI step that runs an extension's gate. It only worked incidentally, after a later `specify init`. Restore permissions at the shared sink. Every extension install route funnels through ExtensionManager.install_from_directory (install_from_zip delegates to it; extension add, extension update, and bundle installs all reach it), so calling the existing ensure_executable_scripts() there covers every route — present and future — by construction rather than by patching each command. The helper already makes .specify scripts executable (init, migrate, and integration-install all call it); it is called plainly, re-establishing the same idempotent "scripts are executable" invariant those flows restore. Deliberately the whole-project call rather than a scoped one: a scan-scope argument would only spare re-walking already-correct files — negligible beside the copy/extract just performed — while widening a simple, widely-used interface for a single caller. Existing callers were audited: init's end-of-init call still covers core .specify/scripts and is untouched; integration-install and migrate do no manager install. Nothing is removed. No-op on Windows; best-effort per file; does not change which files are executable or their mode. Tests: a manager-level regression test asserts a mode-0644 script comes out executable via both install_from_directory and install_from_zip(force=True) (the latter also covering the remove-then-reinstall shape of extension update), plus an end-to-end `extension add --dev` test. Both fail without the change; skipped on Windows. Fixes #3722. |
||
|
Noor ul ain
|
cce47f6900 |
fix(cli): guard lazy .hostname ValueError in extension/preset add --from (#3651)
* fix(cli): guard lazy .hostname ValueError in extension/preset add --from `extension add --from <url>` and `preset add --from <url>` validated the URL by reading `parsed.hostname` OUTSIDE their `try/except ValueError` guards. A bracketed-but-invalid IPv6 authority (e.g. "https://[not-an-ip]/x.zip") parses cleanly under urlparse() on Python < 3.14 and only raises ValueError lazily on the first .hostname access. On the interpreters spec-kit supports (>=3.11) that raw ValueError leaked past the CLI, printing an uncaught traceback instead of the clean "Invalid URL" error. (The raise moved eager into urlparse() only in 3.14.) Same bug class as the catalog/download fixes #3433/#3435/#3437/#3577. - extensions/_commands.py: read parsed.hostname inside the existing try and reuse it for the localhost check. - presets/_commands.py: guard the up-front `urlparse(from_url).hostname` read (preserves the "Invalid URL" message), and harden the nested `_is_allowed_download_url` to take a URL string and parse+read .hostname inside its own try/except -> returns False on malformed input. This also covers the redirect-validator and final-URL (post-redirect) checks, where the URL is server-controlled. Regression tests for each command: a bracketed-non-IP URL, plus a monkeypatched lazy-.hostname raiser that reproduces the pre-3.14 shape independently of the running interpreter (fails with a raw ValueError before the fix, verified via test-the-test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(cli): address Copilot review on --from URL guard comments/tests Copilot's review on #3651 flagged two accuracy problems: 1. The guard comments asserted a specific (and incorrect) CPython version history -- that "https://[not-an-ip]/..." parses cleanly under urlparse() on Python < 3.14 and only raises ValueError lazily on the first .hostname access. In fact the eager bracketed-host check (gh-103848, CVE-2024-11168) was backported to the 3.11 branch and shipped in 3.11.4, so on every interpreter spec-kit supports (>=3.11) that URL is rejected eagerly at urlparse(). Reworded the three source comments to state the guard as a defensive policy (parsing OR the .hostname read can raise ValueError, guard both) without asserting version history. 2. The two monkeypatched lazy-.hostname tests were described as reproducing "the exact production path" / "the Python < 3.14 shape". They are synthetic defensive cases. Relabeled them as synthetic defensive coverage that does not reproduce any specific CPython behavior, and dropped the version-history claims from the bracketed-non-IP test docstrings. The second-round suggestion (_is_allowed_download_url(final_url) instead of _is_allowed_download_url(_urlparse(final_url))) was already applied in the original commit. Behavior unchanged; comments/docstrings only. URL-guard tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Ali jawwad
|
88b3230e2e |
fix(extensions): hyphenate command names in the Forge post-install listing (#3669)
After `specify extension add`, the "Provided commands" summary hyphenated command names only for Cline. For a Forge project the names were printed in dotted form (e.g. `speckit.test-ext.hello`), but Forge registers them hyphenated (`speckit-test-ext-hello`), so the printed names didn't match what the user actually invokes in Forge. Extend the existing Cline handling to Forge via `format_forge_command_name`, completing the Forge command-name parity already fixed for hook invocations (#3641) and the init next-steps panel (#3642). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Pascal THUET
|
5601830ba3 |
harden: bound HTTP reads and enforce strict redirects (#3140)
* harden: bound HTTP reads and enforce strict redirects Add a shared _download_security module (read_response_limited, is_https_or_localhost_http, size constants) and route the GitHub release and Azure DevOps token network reads through bounded reads so an oversized response can't exhaust memory. Add a strict_redirects mode to authentication.open_url: the redirect handler now rejects any redirect whose target isn't HTTPS (or HTTP to localhost), composing with the existing per-hop redirect_validator and auth-stripping. The Azure DevOps token POST is routed through that handler so a 307/308 cannot forward the client_secret body to a non-HTTPS host. Assisted-by: Codex (model: GPT-5, autonomous) * test: align HTTP fakes with bounded reads Assisted-by: Codex (model: GPT-5, autonomous) * fix: tolerate invalid token response encoding Assisted-by: Codex (model: GPT-5, autonomous) * test: align GHES fakes with bounded reads Assisted-by: Codex (model: GPT-5, autonomous) * test: reuse shared upgrade HTTP response helper Assisted-by: Codex (model: GPT-5, autonomous) * fix: include rejected redirect target in error Assisted-by: Codex (model: GPT-5, autonomous) * fix: enforce strict redirects by default Assisted-by: Codex (model: GPT-5, autonomous) * fix: close redirect credential and SSRF gaps Assisted-by: Codex (model: GPT-5, autonomous) |
||
|
Ali jawwad
|
735fe0c5da |
fix(extensions): render hyphenated hook invocations for Forge projects (#3641)
Forge is a hyphenated slash-command agent: it registers its commands as
`/speckit-<name>` (see `format_forge_command_name` and
`ForgeIntegration.build_command_invocation`), exactly like Cline.
`HookExecutor._render_hook_invocation` special-cases dollar-skills agents,
kimi, cline, and slash-skills agents, but had no Forge branch. Forge
matches none of those, so it fell through to `return f"/{command_id}"`
and rendered the DOTTED form — `/speckit.plan`, `/speckit.git.commit` —
which Forge does not recognize as a registered command.
Add a Forge branch mirroring the adjacent Cline branch, using
`format_forge_command_name` (idempotent, same contract as the Cline
formatter). Non-Forge agents are unaffected.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Ali jawwad
|
69c8b64301 |
fix(extensions): re-validate catalog URL after redirects (HTTPS parity/security) (#3524)
* fix(extensions): re-validate catalog URL after redirects (HTTPS parity) ExtensionCatalog._fetch_single_catalog opened the catalog URL and trusted the payload without re-validating response.geturl() after redirects. _open_url follows redirects (stripping auth only on an HTTPS->HTTP downgrade), so an https:// catalog entry that 30x-redirects to http://attacker/... was still fetched and trusted. The payload supplies each extension's download_url + sha256, so a redirected payload can drive install of an arbitrary archive that passes sha256 verification. Add the post-redirect geturl() re-validation via _validate_catalog_url, mirroring integrations/catalog.py, presets, workflows/catalog.py, and bundler adapters. Sibling of the same fix in the presets catalog fetcher. Test: an HTTPS URL whose response.geturl() reports http:// is rejected (ExtensionError). Completed existing fetch-test mocks that predated this behavior to report geturl() like a real urllib response. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(extensions): correct auth-strip comment; validate every redirect hop + guard legacy fetch_catalog - Correct the comment: _StripAuthOnRedirect strips auth not only on an HTTPS->HTTP downgrade but also whenever the redirect leaves the configured trusted hosts. The comment now describes both cases. - Parity with the presets fix: validate EVERY redirect hop (not just the terminal URL) so an https -> http -> attacker-https chain can't slip a redirected payload past the final-URL check. _open_url forwards a redirect_validator to open_url; _fetch_single_catalog passes _validate_catalog_url through it while keeping the final geturl() check. - Give the legacy public fetch_catalog() single-catalog path the same redirect_validator + final geturl() validation (it previously parsed the body with no redirect check). Tests: an intermediate http hop is rejected, and the legacy fetch_catalog() rejects an HTTPS->http redirected payload (both fail before). Full test_extensions.py (356) green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(extensions): cover legacy fetch_catalog() per-hop redirect validation The legacy fetch_catalog() regression test only exercised the terminal geturl() check, so it would still pass if the per-hop redirect_validator were dropped from that duplicated path. Add test_fetch_catalog_legacy_validates_every_redirect_hop, which asserts fetch_catalog() supplies a redirect_validator that rejects an insecure intermediate hop (fails before: the legacy path passed no validator -> NoneType not callable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Marsel Safin
|
2a0ada9a6a |
feat(scripts): port create-new-feature, setup-plan and setup-tasks to Python (#3386)
* feat(scripts): port create-new-feature, setup-plan and setup-tasks to Python Ports the three core workflow scripts to Python as part of #3280, following the check-prerequisites PoC pattern from #3302. Adds resolve_template() to the shared common.py module and parity tests that run bash and Python side by side. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(tests): treat only None env as unset in parity run helper Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): fall back to directory scan on any registry error, skip hidden preset dirs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * feat(templates): add py: lines for setup_plan and setup_tasks Ships with the scripts they reference; the remaining templates got their py: lines in #3403. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: support py variant in skills placeholder resolver resolve_skill_placeholders only accepted sh/ps, so a py init option fell into the fallback path and {SCRIPT} rendered without an interpreter prefix. Accept py and prefix the resolved interpreter, matching process_template. Also guard ps_cmd against a missing PowerShell with a clear assert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: pin clean-error behavior for invalid --number Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(scripts): reword unused-arg comment to match implementation The loop accepts and silently ignores extra positional args (it doesn't build a collected list); match the wording to what the code and setup-plan.sh actually do. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: fall back when configured script variant is missing from frontmatter Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): reject signed/whitespace --number values to match bash 10# parity The bash twin uses $((10#$BRANCH_NUMBER)), which rejects signed and whitespace-padded values. Python's int() accepted them (e.g. -1), producing a malformed -01-... prefix that sequential scans ignore. Restrict --number to unsigned decimal digits before conversion, and pin the parity with a bash-comparison test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): complete Python port installation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): fall back for missing script variants Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: make Python script checks platform-aware Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix Windows Python command invocation parity Use PowerShell's call operator for spaced Python interpreter paths and align setup-tasks missing-template errors across script variants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): preserve cross-platform Python parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reject signed PowerShell feature numbers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): align feature number range Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): reject exhausted feature numbers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): complete create feature parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): align create feature outputs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): harden cross-platform parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): keep truncation JSON clean Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): align setup failure parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): close parity edge cases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): propagate PowerShell setup errors Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): harden fallback resolution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): stabilize PowerShell fallbacks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): complete setup-plan parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(cli): require runnable script fallbacks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(cli): preserve shell fallback without preference Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): restore help and symlink parity - setup-tasks.ps1: check -Help before unknown-argument validation so '-Help --bogus' exits 0 like the Bash/Python variants - common.py: strip the repo root prefix lexically in persist_feature_json instead of resolve(), so a symlinked specs/ still persists the relative 'specs/NNN-name' path the Bash/PowerShell helpers store Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(scripts): align persist-hint quoting with shlex.quote - create-new-feature.sh: replace printf %q with a shell_quote helper that emits shlex.quote-identical output, so the persistence hints stay byte-identical between the Bash and Python variants (printf %q output also varies between bash versions) - promote the negative --number test to an all-variants parity test now that Bash and PowerShell reject signed values consistently - add a spaced-repo-path parity test for the persistence hints Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Andrew Chen
|
7873c447bd |
fix(agents): parse frontmatter on the --- delimiter line, not any --- substring (#3590)
CommandRegistrar.parse_frontmatter located the closing delimiter with
content.find("---", 3), a raw substring search. It stopped at the first
"---" anywhere after the opening — including one embedded in a
frontmatter value (e.g. a description "Separate sections with ---
markers") or inside an indented literal block — which truncated the
frontmatter and spilled the remainder into the body, silently corrupting
both the parsed metadata and the rendered command body.
Match the closing "---" on line boundaries, mirroring the line-anchored
scan already used by VibeIntegration._inject_frontmatter_flag.
|
||
|
github-actions[bot]
|
eabfabb490 |
[bug-fix] Fix reinstall-overwrites-kept-config: preserve config on plain reinstall after --keep-config (#3449)
* Fix reinstall-overwrites-kept-config: preserve config on plain reinstall after --keep-config Apply the remediation from the bug assessment on issue #3427. Before the unconditional shutil.rmtree(dest_dir), scan dest_dir for any *-config.yml and *-config.local.yml files and hold their contents in memory. After shutil.copytree succeeds, write them back so user-customized values always win over the packaged defaults. This mirrors the existing backup/restore logic for the --force reinstall path but handles the case where remove --keep-config left config files behind in an unregistered extension directory. Refs #3427 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: restore method decl, move config restore before registration, preserve file mode - Restore missing `test_install_force_without_existing` method declaration in tests/test_extensions.py so pytest collects it as a separate test. - Move stranded-config restoration to immediately after `copytree`, before command/skill/hook registration, so a failed registration step can't leave preserved configs permanently lost. - Store `(bytes, mode)` tuples instead of bare bytes when rescuing stranded configs, and reapply the original file mode after writing so permission bits (e.g. 0600 for credential files) are faithfully restored. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous) * fix: mask setuid/setgid bits when restoring stranded config file mode Only preserve user/group read-write bits (mode & 0o660) to avoid restoring setuid, setgid, or world-writable permissions from a user-modified config file. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous) * fix: add copytree rollback path and strengthen regression test with packaged default config - Wrap shutil.copytree in a try/except BaseException so stranded configs rescued before rmtree are written back even if copytree fails mid-way (addresses review comment: configs were permanently lost on copy failure) - Add a packaged default config to extension_dir in the regression test so a naive 'restore only when absent' implementation would fail; assert the user's customized values beat the packaged defaults after reinstall Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix: restore configs with secure atomic writes Assisted-by: GitHub Copilot (model: gpt-5, autonomous) * fix: write secure temp file then chmod to preserved_mode; add copytree-failure test - _restore_stranded_config_file: write content while temp file is at its secure OS-default mode (typically 0600 on POSIX), then apply the original preserved_mode after the file is fully written and before the atomic os.replace. Removes the & 0o660 mask that was silently stripping world-read and executable bits (e.g. 0644 → 0640). - Add test_copytree_failure_restores_stranded_config: patches shutil.copytree to create a partial destination then raise OSError, then asserts that the preserved config bytes and file mode are restored by the rollback path and that the extension remains unregistered. Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * 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> * Potential fix for pull request finding 'Module is imported with 'import' and 'import from'' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * fix: durable staging for stranded configs and import style fix - Stage stranded config files to a durable rescue_staging_dir (extensions_dir/.rescue-staging-<id>) before rmtree so original bytes survive partial rmtree, copytree failure, or partial restore on retry. On retry the staging dir is detected and its content reused instead of whatever mix of packaged defaults and partial restores remains on disk. The staging dir is cleaned up only after every restore succeeds. - Fix CodeQL: change `import specify_cli.extensions as _ext_module` to `from specify_cli import extensions as _ext_module` in test file. Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix: harden rescue staging dir - symlink checks, secure writes, cleanup errors - Thread 14: Change except BaseException to except Exception in the staging fallback block so KeyboardInterrupt/SystemExit propagate correctly - Thread 15: Add explanatory comment to the bare pass in the chmod except block to satisfy static analysis - Thread 16: Reject a symlinked staging directory and only reload non-symlinked files whose names match the two recognised config suffixes - Thread 17: Create each staging file via os.open with mode 0600 and O_CREAT|O_EXCL before writing so preserved bytes are never transiently exposed to other local users - Thread 18: Remove ignore_errors=True from the final staging-dir cleanup so a failed rmtree propagates rather than silently leaving a stale backup that could be misread on the next retry Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix: mask file-type bits from chmod, harden staging dir symlink check - Add `import stat` to imports - Use `stat.S_IMODE(mode)` before chmod in staging write (thread 20, line 1464) - Use `stat.S_IMODE(preserved_mode)` and make chmod best-effort in `_restore_stranded_config_file` (thread 18, line 1492) - Add `not rescue_staging_dir.is_symlink()` guard to cleanup (thread 19, line 1522) Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) * fix: use completion marker for rescue staging, abort on staging failure, full os.write Assisted-by: GitHub Copilot (model: GPT-5.3-Codex, autonomous) * fix(extensions): make rescue staging durable Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * test(extensions): fix flaky copytree regression test Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * test(extensions): fix module import alias for review feedback Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(workflows): keep cleanup warnings single-line and remove dead helper Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding 'Module is imported with 'import' and 'import from'' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * Preserve rescued extension config across retry Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Clarify ignored directory fsync cleanup errors Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix reinstall durability and workflow cleanup warnings Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Open rescue staging file in binary mode to fix Windows CRLF corruption On Windows os.open() defaults to text mode, so os.write() of preserved config bytes containing \r\n was translated to \r\r\n, corrupting the staged backup and failing the retry-restore regression test. Add O_BINARY (0 on POSIX) to the staging file open flags. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Load .extensionignore before deleting dest_dir on reinstall The .extensionignore loader can raise ValidationError (invalid UTF-8) or OSError. Previously it ran after dest_dir was removed, so such a failure left the kept config only in the hidden staging directory rather than its documented location. Load/validate it before the rmtree so every post-deletion failure path restores the config. Adds a regression test. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Validate .extensionignore before publishing rescue staging Loading .extensionignore after the rescue staging directory was published meant a validation failure left a complete staging copy behind. A later retry (after the user fixed the ignore file and edited the kept config) would reload the stale staged bytes and silently overwrite the newer config. Move the loader ahead of reading/creating rescue staging so a failure aborts while the kept config is still authoritative on disk, and extend the regression test to prove no staging is published and a retry adopts the newer bytes. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden preserved-config rescue against divergence and long names Address three review findings on the reinstall config-rescue path: - A complete .rescue-complete marker proves only that staging finished, not that dest_dir was modified. A crash after staging sync but before the rmtree leaves the live kept config intact; if the user edits it before retrying, preferring the staged bytes silently overwrote the newer config. The two copies are indistinguishable in provenance from disk, so detect divergence between a complete staging copy and the live config and abort (preserving both) instead of unconditionally choosing staging. - The staging directory embedded the full extension ID in one path component. Extension IDs are length-unbounded, so a valid long ID could install at dest_dir yet fail every reinstall-after-keep-config with ENAMETOOLONG. Derive the staging component from a fixed-length hash via a new _rescue_staging_dir() helper. - The stranded-config restore used the full config filename as a NamedTemporaryFile prefix; a name already near the component limit plus the random suffix raised ENAMETOOLONG. Use a short fixed prefix. Updates the retry regression test to the new divergence semantics and adds conflict-abort, long-ID, and fixed-prefix coverage. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Harden preserved-config rescue divergence check and fix test path Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) * fix: reject/flag symlinked preserved configs on reinstall Assisted-by: GitHub Copilot (model: GPT-5.6-Sol, autonomous) * fix: include symlinks in live-dir config enumeration and address review feedback - _recognized_config_names() now accepts follow_symlinks=False for live dir so symlinked *-config.yml entries are detected and treated as conflicts rather than being silently deleted by rmtree. - Add explanatory comment to bare 'except OSError: pass' in _restore_stranded_config_file's finally block. - Resolve CodeQL dual-import style: use 'from specify_cli import extensions as _ext_module' instead of 'import specify_cli.extensions as _ext_module'. Assisted-by: GitHub Copilot (model: claude-sonnet-4, autonomous) * test: add staging-failure fault-injection test for rescue staging block Add test_staging_failure_aborts_before_dest_dir_removal covering three failure modes (mkdir, os.open/O_CREAT, fsync with EIO) in the rescue staging block. Each parametrized case verifies: - the install aborts before dest_dir is removed - the preserved config bytes remain authoritative - any partial staging is cleaned up and not left as complete - the extension stays unregistered Addresses review feedback on PRRT_kwDOPiFCnc6R351t. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous) * test: add test_retry_restores_config_from_staging_when_live_absent Exercises the retry-from-staging branch (if staging_is_complete at line 1505 of extensions/__init__.py) in a scenario where the live config is absent — simulating a power loss that interrupted the rollback before it could write the config back. When the live copy is gone, the live-dir fallback (elif dest_dir.exists()) finds no stranded configs and the packaged default would be kept. Only the staging-complete branch can restore the original bytes and mode. This proves staging (not the fallback) is used on retry. Addresses review feedback on PRRT_kwDOPiFCnc6SAL3L. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous) * fix: keep staging files writable; record modes in .rescue-modes.json; fix live-only conflict message Thread 64: Remove os.fchmod/chmod from staged files to avoid Windows read-only attribute that prevents shutil.rmtree from cleaning up. Original permission bits are now written to a .rescue-modes.json sidecar in the staging dir and reloaded during retry, with a fall-back to the staged file's own mode for backwards-compat with pre-sidecar staging dirs. Thread 65: Split the ValidationError message for staging-vs-live conflicts into two accurate cases: files that diverged between both locations ("Both copies have been preserved") and live-only files that have no backup counterpart, which previously incorrectly claimed "Both copies have been preserved" and offered a restore instruction that was impossible. Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous) * Potential fix for pull request finding 'Empty except' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * fix: add .keep-config provenance marker to guard rescue path against partially-failed installs When `remove --keep-config` strands config files, write a `.keep-config` marker into the extension directory. `install_from_directory` now only enters the rescue path when that marker is present, preventing a partially- failed install (which also leaves dest_dir with no registry entry but no marker) from having its packaged default configs treated as user-preserved data on a retry from an updated package. Refs: https://github.com/github/spec-kit/pull/3449#discussion_r3606283457 Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous) * refactor: extract _has_keep_config_marker helper and document empty-content choice Assisted-by: GitHub Copilot (model: claude-sonnet-4.5, autonomous) * fix: defer rescue-backup cleanup until registry commit; validate modes sidecar shape Assisted-by: GitHub Copilot (model: GPT-5.6, autonomous) * fix legacy keep-config rescue and retry baseline handling --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Manfred Riem <15701806+mnriem@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> |
||
|
Noor ul ain
|
8a5bcc21a5 |
fix(extensions,presets): surface clean error on malformed download URL (#3577)
* fix(extensions,presets): surface clean error on malformed download URL `ExtensionCatalog.download_extension` and `PresetCatalog.download_pack` read `download_url` from catalog payload data and pass it to `urlparse(...).hostname` during the HTTPS validation. A malformed authority (e.g. an unterminated IPv6 bracket like `https://[::1`) makes urlparse/hostname raise a raw `ValueError`, which escapes past the command handlers — they only catch `ExtensionError` / `PresetError` — and surfaces as an uncaught traceback. Guard the parse in a try/except and re-raise as the domain error so the CLI reports a clean "download URL is malformed" message. Mirrors the same fix in catalogs (#3435) and workflows/catalog.py (#3484). Adds regression coverage for both catalogs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(presets): escape markup in preset_add error handlers Copilot review on #3577 flagged that the malformed-URL fix stopped short: `download_pack` now raises a clean `PresetError`, but the `preset_add` handler rendered `{e}` unescaped. A catalog `download_url` like `https://[not-an-ip]/x` is embedded verbatim in the message, so Rich interprets `[not-an-ip]` as a markup tag and can raise a style/markup exception while rendering the error — the CLI still crashes instead of exiting cleanly. Escape `str(e)` in the preset command handlers, matching the extension handler at `extensions/_commands.py:657`, and hoist the `rich.markup` import to module scope (dropping the two inline imports). Adds CLI-level regression tests: a bracketed-host `download_url` exits cleanly, and the compatibility/validation/error handlers escape markup-bearing messages. Both tests fail on the pre-fix handler (test-the-test verified). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Ali jawwad
|
eb2252a1cb |
fix(catalogs): 'priority: .inf' yields a clean validation error instead of crashing (#3525)
* fix(catalogs): priority: .inf yields a clean validation error, not OverflowError
_load_catalog_config coerces a catalog entry's priority with int() inside
except (TypeError, ValueError). int(float('inf')) raises OverflowError, which is
not in that tuple, so a YAML 'priority: .inf' escaped as an uncaught traceback
instead of the intended 'expected integer' validation error (the bool-is-int
case is already guarded just above). Add OverflowError to the except tuple.
Test mirrors the existing rejects_boolean_priority test with priority: .inf
(fails before: OverflowError; passes after: ValidationError naming the config).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(presets): priority: .inf in a preset catalog config yields a clean error
The PresetCatalog._load_catalog_config priority parser has its own loader
(separate from CatalogStackBase) that caught only TypeError/ValueError, so a
YAML 'priority: .inf' escaped as an uncaught OverflowError from int(float('inf')).
Add OverflowError to the except tuple (the bool-is-int case is already guarded
just above), matching catalogs.py.
Test mirrors rejects_boolean_priority with priority: .inf (fails before:
OverflowError; passes after: PresetValidationError).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
thejesh23
|
2537be8144 |
fix(extensions): stop env-var config leaking across prefix-colliding extension IDs (#3497)
* fix(extensions): stop env-var config leaking across prefix-colliding IDs (#3494) Because ``_`` doubles as both the separator between an extension ID and its config path AND the substitute for ``-`` inside an extension ID, an env var like ``SPECKIT_GIT_HOOKS_URL`` starts with *both* the ``SPECKIT_GIT_`` prefix of the ``git`` extension and the ``SPECKIT_GIT_HOOKS_`` prefix of a co-installed ``git-hooks`` extension. ``ConfigManager._get_env_config`` matched only on the shorter prefix, so the same env var silently surfaced inside both extensions' configs (as ``{'hooks': {'url': ...}}`` for ``git`` and ``{'url': ...}`` for ``git-hooks``). Impact: config intended for one extension leaked into another and, worse, could flip ``config.<field> is set`` hook conditions on the wrong extension. Route the env var to the extension whose normalized ID is the longest match — the more specific one. When another installed sibling's normalized ID + ``_`` claims the remainder, skip the var here. The sibling scan reads ``.specify/extensions/`` directly and degrades to a no-op if the dir is missing (fresh project / ad-hoc harness), so the pre-fix single-extension behaviour is unchanged when there is no collision. Distinct from #3350 (intra-extension prefix collision between two keys of the same extension) — this fixes the cross-extension case. Fixes #3494 * fix(extensions): source sibling scan from registry, not directory Address Copilot review on #3497: ``ExtensionManager.remove(..., keep_config=True)`` preserves the extension directory but drops the registry entry, so the previous directory-scan approach would treat a config-only leftover as an installed sibling and silently discard ``SPECKIT_<sibling>_*`` env vars into no owner. Sourced the sibling list from ``ExtensionRegistry.keys()`` — the registry is the source of truth for "installed" — and kept the same graceful ``[]`` fallback so the fresh-project / ad-hoc harness path is unaffected. Updated the ``TestConfigManagerCrossExtensionEnvLeak`` ``_install`` helper to register its fake installations and added ``test_config_only_leftover_not_treated_as_sibling`` to lock in the new behaviour for the ``keep_config=True`` scenario. Full suite: 3978 passed, 110 skipped. * fix(extensions): swallow non-UTF-8 registry in sibling scan Address Copilot follow-up on #3497: ``ExtensionRegistry._load()`` catches ``JSONDecodeError`` / ``FileNotFoundError`` but not decode failures — a registry file with invalid text encoding would surface a ``UnicodeDecodeError`` out of ``_sibling_extension_ids`` and break every config read instead of degrading to the documented pre-fix behaviour. Extend the fallback in ``_sibling_extension_ids`` to also catch ``UnicodeError`` and add ``test_non_utf8_registry_does_not_crash`` as a regression pin (kept ``_load()`` itself out of scope — that broader hardening belongs in a separate PR since it affects all readers). Full suite: 3979 passed, 110 skipped. |
||
|
Marsel Safin
|
3b7d95a408 |
fix: rewrite extension-relative subdir paths in generated command bodies (#3444)
* fix: rewrite extension-relative subdir paths in generated command bodies Extension command bodies reference bundled files relative to the extension root (agents/, knowledge-base/, templates/, ...). Generated SKILL.md and command files emitted those paths verbatim, so agents resolved them against the workspace root where they do not exist. Add CommandRegistrar.rewrite_extension_paths, which rewrites references to subdirectories that actually exist in the installed extension to .specify/extensions/<id>/..., and call it once in register_commands so every output format and alias gets the fix. commands/, specs/ and dot-directories are never rewritten. Fixes #2101 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: only rewrite relative extension path references Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use callable re.sub replacement for extension subdir rewrite subdir and extension_id come from filesystem directory names and were interpolated into a re.sub string replacement template. A directory name containing a backslash (e.g. assets\q) would raise re.error: bad escape, aborting command registration even when the body didn't reference it. Use a callable replacement so these values are treated literally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: make subdir rewrite regression test cross-platform Renamed the test's subdir fixture from "assets\\q" to "assets[q]": on Windows, backslash is a path separator, so mkdir would create nested "assets/q" dirs instead of one literally-named directory, and iterdir() would only discover "assets", never exercising the rewrite. extension_id keeps a real backslash/"\\1" since it isn't used to create a directory, still verifying the callable replacement handles it literally. Added a sanity assertion for this assumption. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: apply extension subdir path rewrite in skills-mode renderer register_commands() rewrote extension-relative subdir references (agents/, knowledge-base/, etc.) via rewrite_extension_paths(), but _register_extension_skills() - the separate renderer used for active non-native skills agents (e.g. Claude with ai_skills: true) - never called it. Generated SKILL.md files left agents/... and knowledge-base/... unresolved, and mapped the extension's own templates/ through the generic project-level rewrite instead of its installed .specify/extensions/<id>/templates/ location. Reuse the existing rewrite_extension_paths() helper in _register_extension_skills() at the same point register_commands() applies it (before resolve_skill_placeholders' generic rewrite), and add a skills-mode regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: apply extension subdir path rewrite on preset restore/reconcile paths _unregister_skills() restored extension-backed SKILL.md content via resolve_skill_placeholders() without first calling rewrite_extension_paths(), so removing a preset override that shadowed an extension command restored the bare, unresolvable agents/... and knowledge-base/... references. Carried extension_id/extension_dir through _build_extension_skill_restore_index() and applied the same rewrite used at initial registration before restoring. Found the identical gap in _reconcile_composed_commands()'s non-skill agent path: when a removed preset's command reverts to an extension winner, register_commands_for_non_skill_agents() was called without extension_id, so the rewrite never ran for plain command-file agents either. Passed extension_id through there too. Added regression tests for both restore paths (skills-mode and non-skill-agent command files) in tests/test_presets.py. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: apply extension subdir path rewrite when composing over extension base PresetResolver.resolve_content() read the effective base layer's raw content directly via path.read_text() before composing append/prepend/ wrap overlays on top of it, and its outright-replace shortcut did the same. When that base layer was extension-provided, neither read path applied rewrite_extension_paths(), so composing a preset over an extension command (or an extension winning outright through resolve_content) left bare, unresolvable agents/... and knowledge-base/... references in the composed output. All three call sites (PresetManager._register_commands()'s composed path, _reconcile_composed_commands()'s composed path, and skills-mode reading the .composed file written by either) consume resolve_content's return value, so fixing the read at its source covers command output, skill output, and both initial-install and reconcile flows without threading extension identity through each caller. Tagged extension layers in collect_all_layers() with extension_id/ extension_dir, and added a _read_layer_content() helper in resolve_content() that applies rewrite_extension_paths() whenever a layer carries that extension identity — used at both raw-read sites (outright-replace shortcut and composition base). Composing (append/prepend/wrap) layers are never extension-provided (extensions are always inserted with strategy "replace"), so no other read site needs the rewrite. Added regression tests: a parametrized resolve_content() test covering append/prepend/wrap composing over an extension base, a skills-mode test asserting the composed SKILL.md resolves the extension's subdir references, and a non-skill-agent (Gemini) install-time test matching the reported live repro. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Ali jawwad
|
903d707d21 |
fix(extensions): set-priority repairs corrupted boolean priority (#3268)
The set-priority skip guard 'isinstance(raw_priority, int) and raw_priority == priority' treats a stored boolean as a match because isinstance(True, int) is True and True == 1 (False == 0). So a corrupted boolean priority short-circuits to 'already has priority N' and is never rewritten to a real int — contradicting the adjacent comment that promises corrupted values get repaired. Exclude bools explicitly, mirroring normalize_priority's own bool guard. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Ali jawwad
|
e59da78677 |
fix(extensions): handle prefix-colliding env vars in _get_env_config (#3350)
* fix(extensions): handle prefix-colliding env vars in _get_env_config
_get_env_config built the nested dict with 'if part not in current:
current[part] = {}' and an unconditional leaf assignment. Two env vars
that collide on a prefix — e.g. SPECKIT_X_CONNECTION and
SPECKIT_X_CONNECTION_URL — then either crash (scalar processed first:
the walk indexes into a str -> TypeError 'str object does not support
item assignment') or silently clobber the nested dict (scalar processed
last). Via should_execute_hook's blanket except, the crash silently
disables every config-based hook for the extension. Guard the walk and
the leaf assignment with isinstance checks so a colliding scalar yields
to the nested dict; result is order-independent
({'connection': {'url': ...}} either way), matching _merge_configs'
dict-preserving semantics.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(extensions): assert via public should_execute_hook, not private helper
Per review: the colliding-env hook test described should_execute_hook
swallowing the TypeError, but asserted on the private _evaluate_condition.
Assert on the public should_execute_hook instead — it returns False
(silently disabled) before the fix and True after, matching the
real-world failure mode and not coupling to a private helper.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(extensions): ignore malformed env var names in _get_env_config
Per review: a name like SPECKIT_<EXT>_ (no key) or with consecutive
underscores produced empty path components, creating surprising entries
under an empty key (env_config[''] = ...). Filter out empty parts and
skip the variable entirely when nothing remains.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Quratulain-bilal
|
94c7ec288f |
fix(toml): escape control characters so generated command files parse (#3341)
* fix(toml): escape control characters so generated command files parse both toml renderers (TomlIntegration._render_toml_string for gemini/tabnine and CommandRegistrar.render_toml_command for extension/preset commands) wrote control characters raw into a multiline or basic string. toml forbids literal control chars (U+0000-U+001F except tab/newline, and U+007F) in every string form, and a bare CR that is not part of a CRLF pair, so a description or body containing one produced a .toml file that fails to parse. route any value with such a character to a fully-escaped basic string that emits the leftover control chars as \uXXXX. added regression tests that round-trip through tomllib. * refactor(toml): centralize control-char escaping in one shared helper the control-char detection and basic-string escaping added for both toml renderers were copy-pasted into agents.py and integrations/base.py. move the two functions into specify_cli/_toml_string.py and have both renderers delegate to it, so the escaping rules can't drift apart later. no behavior change; both renderers now reference the same implementation. |
||
|
Marsel Safin
|
882e1e90d0 |
fix(cli): exit cleanly on malformed IPv6 URLs in extension/preset/workflow add (#3369)
* fix(cli): exit cleanly on malformed IPv6 URLs in extension/preset/workflow add extension add --from, preset add --from, and workflow add <url> parsed the user-supplied URL with a bare urlparse before their HTTPS/host validation, so an unclosed IPv6 bracket escaped as a raw ValueError traceback. Wrap each parse and emit the surrounding validation's clean error style + typer.Exit(1) instead. Fixes #3368 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(cli): convert malformed redirect URLs to URLError in shared redirect handler Parse the redirect target once in _StripAuthOnRedirect.redirect_request before the validator and stdlib handler run, converting ValueError into URLError which every download path already catches. Also escape from_url in the preset install message so IPv6 brackets don't break Rich markup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Zhiyao Wen
|
1930f89d17 |
fix extension-local script path rewriting (#3364)
Co-authored-by: Zhiyao <zhiyao@ZhiyaodeMacBook-Air.local> |
||
|
Ali jawwad
|
52480ee50f |
fix(extensions): coerce non-mapping YAML config roots to {} in ConfigManager (#3345)
ConfigManager._load_yaml_config returned yaml.safe_load(...) or {}, which
only guards falsy roots — a truthy non-mapping root (a YAML list or
scalar) flows straight into _merge_configs, whose .items() raises
AttributeError. get_config()/has_value()/get_value() then crash, and via
should_execute_hook's blanket 'except Exception: return False' every
config-based hook condition for that extension is silently disabled.
Coerce a non-dict root to {}, mirroring the existing non-dict-root guard
in get_project_config(). Hardens all three call sites in one place.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Noor ul ain
|
f59fd81608 |
fix(extensions): resolve core-command dirs via _assets helpers (#3274) (#3287)
`_load_core_command_names()` computed its candidate command dirs with bespoke `Path(__file__)` arithmetic. The #3014 move of this module from `specify_cli/extensions.py` to `specify_cli/extensions/__init__.py` pushed the file one directory deeper but left the `.parent` counts unchanged, so both candidates resolved to non-existent paths: wheel -> specify_cli/extensions/core_pack/commands (real: specify_cli/core_pack/commands) source -> src/templates/commands (real: repo-root templates/commands) Neither exists, so every call silently fell through to `_FALLBACK_CORE_COMMAND_NAMES`. Discovery is latent-dead: the fallback happens to equal the real stems today, but the shadowing guard (#1994) that depends on it now relies on someone hand-editing the fallback on every core-command add/remove (as already happened for `converge`, #3001). Delegate path resolution to the canonical `_locate_core_pack` / `_repo_root` resolvers in `_assets` — the same ones the presets and bundle loaders use. They are anchored to the package root, so discovery survives future module moves. Add regression tests that point the resolvers at a temp tree with *different* command names, proving discovery reads from disk rather than returning the fallback (they fail on the pre-fix code). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Dyan Galih
|
6fb7e77b3e |
fix: allow prerelease spec-kit versions in compatibility checks (#2695)
* docs: generate integrations reference from catalog * refactor: integrate table rendering into specify integration search --markdown - Remove standalone scripts/generate_integrations_reference.py - Strip doc injection machinery from catalog_docs.py; keep only table rendering - Wire render_integrations_table() into existing --markdown flag of integration search - Remove old simple markdown table block from integration_search (was Name|ID|Version|Description|Author) - Simplify tests: drop subprocess/doc-path tests, keep table rendering and metadata tests - Clean up docs/reference/integrations.md: remove generated markers, update note * fix: address Copilot review feedback on catalog_docs and integration_search - Warn when --markdown is combined with filters (query/--tag/--author) which are silently ignored; catch ValueError/FileNotFoundError and surface clean error via console instead of raw traceback (r3244821516) - Add coverage enforcement in list_integrations_for_docs(): raises ValueError with actionable message if any registry key is missing from INTEGRATION_DOC_URLS, preventing silently incomplete doc tables (r3244821589) - Rename test to accurately reflect sources: label derives from registry config, URL comes from INTEGRATION_DOC_URLS doc map — not solely from registry (r3244821607) - Simplify test dict construction to idiomatic dict comprehension (r3244821619) * fix: add sync test, INTEGRATIONS_REFERENCE_PATH constant, and fix naming * revert: restore docs/reference/integrations.md to upstream/main; remove sync test (GH Actions job will handle) * fix: remove dead INTEGRATIONS_REFERENCE_PATH, drop URL-length padding, fix docstring, drop FileNotFoundError * fix: send --markdown warnings/errors to stderr, rename test for clarity * fix: detect stale doc-map keys, test _render_cell escaping, strengthen header assertion * refactor: promote _render_cell to public render_cell function * test: mock registry and doc maps to avoid brittle live registry coupling * refactor: flatten patches, remove unused imports, fix trailing whitespace, optimize missing calculation * refactor: make validation non-fatal, fix context manager syntax, add CLI tests * fix: improve docstring clarity, test robustness, and exception handling * fix: improve test assertions, disable warnings by default, enhance exception handling * fix: make CLI tests deterministic and improve config access resilience * fix: remove extra blank line, add stale keys validation, add regression test for docs sync * Fix 5 remaining feedback items: - Rename _get_mocked_cli_runner() to _get_catalog_docs_patches() for clarity - Use ExitStack context manager for guaranteed patch cleanup - Add explicit UTF-8 encoding to file reads - Skip doc sync test gracefully when docs aren't present - Remove exception chaining from typer.Exit to avoid noisy tracebacks * address all outstanding copilot review feedback on PR 2563 * Address Copilot feedback: escape URLs in markdown links, deduplicate cell rendering, fix table parser for escaped pipes * Address 3 new Copilot feedback: add URL escaping test, fix parse_first_markdown_table for escaped pipes, guard community tests with skip * Address 3 new Copilot feedback: escape id field, remove unused alias, escape integration URLs * Address 3 new Copilot feedback: fix comment name, include all integrations in list * Fix architectural issue: escape raw fields before composing Markdown to prevent double-escaping * Deduplicate _escape_url_for_markdown_link and add URL escaping test * Address 4 new Copilot feedback: add trailing newline, fix test helper ExitStack, update warning message * Address 4 new Copilot feedback: make escape function public, fix error message, validate test rows, prevent double newline * Update error message in test_missing_catalog_file for clarity * Remove obsolete integrations sync test * keep integrations docs in sync * fix: allow prerelease spec-kit versions in compatibility checks Allow prerelease/dev builds to satisfy extension and preset compatibility checks when their version number falls within the required specifier range. Also harden the integrations docs rendering helpers and add regression coverage for the markdown table parsing and version gating paths. Tests: pytest -q; python3 -m compileall -q .; black/flake8 unavailable Reference: branch 002-generate-integrations-docs; source patch /tmp/spec-kit-changes.patch * fix: isolate prerelease compatibility gate changes Keep the prerelease/version compatibility fix on its own branch and remove the unrelated integrations docs updates that belong with PR 2563. Tests: full suite passed on the prerelease branch before splitting; docs branch covered by targeted docs tests Reference: upstream/main; source patch /tmp/spec-kit-changes.patch * Address PR 2695 feedback: Centralize prerelease policy and add boundary test * Address remaining Copilot PR feedback: revert docs and add preset prerelease tests * Remove unreachable raise CompatibilityError * Fix PEP8 E302 and E303 formatting issues |
||
|
Manfred Riem
|
bbc5f176e3 |
fix(extensions): apply GHES auth and resolve release assets for extension add --from (#3217)
* fix(extensions): apply GHES auth and resolve release assets for --from The 'specify extension add --from <url>' path fetched ZIPs via a bare open_url with no GitHub release-asset resolution and no Accept header, diverging from the catalog download path. Against GHES it received an HTML login page and failed obscurely with zipfile.BadZipFile. Route --from through ExtensionCatalog so configured GHES credentials apply and release-download URLs resolve via /api/v3, and reject non-ZIP content with a clear error pointing at auth.json. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(extensions): use zipfile.is_zipfile for --from content guard Replace the weak zip_data.startswith(b"PK") prefix check with zipfile.is_zipfile() on a BytesIO so any non-ZIP payload (not just those lacking the PK magic) is rejected with the friendly error before install_from_zip can raise BadZipFile. Addresses PR review feedback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Si Zengyu
|
1add20341d |
fix(extensions,presets,workflows): resolve private GHES release assets via /api/v3 (#3157)
* feat(auth): add github_provider_hosts() to enumerate GHES hosts from auth.json Assisted-by: Claude Code (model: claude-sonnet-4-6, autonomous) * fix(extensions): resolve GHES release assets via /api/v3 Generalizes resolve_github_release_asset_api_url to GitHub Enterprise Server hosts (gated by auth.json github hosts), fixing private GHES extension/preset downloads. github/spec-kit#3147 Assisted-by: Claude Code (model: claude-sonnet-4-6, autonomous) * fix(extensions,presets): pass auth.json github hosts into release resolver Assisted-by: Claude Code (model: claude-sonnet-4-6, autonomous) * docs(auth): document GHES private catalog + release-asset auth Assisted-by: Claude Code (model: claude-sonnet-4-6, autonomous) * fix(presets,workflows): pass auth.json github hosts into remaining release resolvers Wires preset add --from and workflow add through github_provider_hosts() so private GHES release assets resolve via /api/v3 there too. github/spec-kit#3147 Assisted-by: Claude Code (model: claude-sonnet-4-6, autonomous) * test(presets): use module-level io.BytesIO in GHES preset test Addresses Copilot review on PR #3157: drop unnecessary __import__("io") in test_preset_add_from_ghes_release_url_resolves_via_api_v3 since io is already imported at module level. * fix(github-http): pass through GHES asset API URLs by path shape Addresses Copilot review on PR #3157. A direct GHES /api/v3 release asset URL was only returned as already-resolved when its host was in the allowlist; otherwise the resolver returned None and the caller downloaded the same URL without 'Accept: application/octet-stream', fetching JSON metadata instead of the binary. Gate the passthrough on path shape alone, mirroring the github.com case. This is safe: passthrough returns the input URL unchanged and the caller fetches it either way, so no new request to an arbitrary host is induced; the token stays independently gated by auth.json in open_url. The allowlist remains the anti-SSRF gate on the tag-lookup resolving path. Add test_passthrough_for_unlisted_ghes_api_asset_url. |
||
|
meymchen
|
dc840f07d0 |
feat(integration): update Kimi integration for Kimi Code CLI (#2979)
* feat(integration): update Kimi integration for Kimi Code CLI Update the Kimi integration to target the new Kimi Code CLI (MoonshotAI/kimi-code) layout: - Change skills directory from .kimi/skills/ to .kimi-code/skills/ - Change context file from KIMI.md to AGENTS.md - Extend --migrate-legacy to move old .kimi/skills/ installs and migrate KIMI.md user content to AGENTS.md - Clean up leftover legacy .kimi/skills/ directories on teardown - Update devcontainer installer to @moonshot-ai/kimi-code - Update docs and tests Relates to #1532 * fix(integration): align Kimi dispatch and harden legacy migration - Override build_command_invocation to emit /skill:speckit-<stem> so dispatched commands match Kimi Code CLI's native slash syntax. - Skip symlinked .kimi/skills directories during legacy migration and teardown to avoid operating on files outside the project. - Remove kimi from the multi-install-safe integrations table. - Add tests for command invocation and symlink safety. * fix(integration): resolve custom context markers in Kimi legacy migration Use IntegrationBase._resolve_context_markers() when migrating legacy KIMI.md content so that projects with customized context_markers in .specify/extensions/agent-context/agent-context-config.yml have the managed section stripped with the correct markers instead of the hard-coded defaults. Adds a test verifying custom markers are respected during --migrate-legacy. * fix(integration): harden Kimi legacy migration against symlinked paths * fix(kimi): guard symlinked SKILL.md during migration and teardown * docs(kimi): mention KIMI.md→AGENTS.md migration in --migrate-legacy help The --migrate-legacy help text listed only the skills directory move and dotted→hyphenated renaming, but the flag also migrates KIMI.md user content into AGENTS.md. Align the help with the actual behavior, docs, and tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(kimi): validate legacy migration destination; clarify docstrings Address Copilot review feedback on PR #2979: - setup(): gate skills migration on _is_safe_legacy_dir(new_skills_dir) as well as the source. base setup() already rejects a destination that escapes the project root, but an in-tree symlinked .kimi-code/skills (e.g. -> .) could still misdirect the move; this gives the destination the same symlink-component protection as the source. - _migrate_legacy_kimi_dotted_skills: rewrite docstring as a compatibility shim describing same-path delegation to _migrate_legacy_kimi_skills_dir. - test_presets: clarify that the dotted-skill test exercises legacy naming under the current .kimi-code/ base, not the legacy .kimi/ location. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(kimi): harden legacy KIMI.md→AGENTS.md context migration - Skip context-file migration when the agent-context extension is disabled, matching upsert/remove_context_section opt-out behavior so an opted-out project's KIMI.md/AGENTS.md are left untouched. - Safely skip (instead of raising) on filesystem edge cases: unreadable or non-UTF-8 KIMI.md, and AGENTS.md existing as a non-file/unwritable. - Refuse to migrate a corrupted managed section (single marker, or end before start) so a partial managed block is never copied into AGENTS.md; KIMI.md is preserved for manual repair. Add regression tests for all three cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Approve fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * chore(kimi): revert CHANGELOG.md edit (auto-generated) The CHANGELOG is generated from merged PR titles, so a hand-written entry is redundant; it was also placed under the already-released 0.10.2 section, which would make those release notes historically inaccurate. Revert to match main per maintainer feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(kimi): skip symlink-safety tests when symlinks are unavailable The Kimi legacy-migration safety tests create symlinks to assert that migration/teardown never follow them out of the project. Symlink creation fails on Windows without the create-symlink privilege and in some restricted CI sandboxes, so these tests errored during setup instead of skipping. Wrap every symlink_to() call in a shared _symlink_or_skip() helper that pytest.skip()s on OSError/NotImplementedError, matching the guard pattern already used by one of these tests. Verified on Windows: the 6 symlink tests now skip cleanly (51 passed, 6 skipped) instead of erroring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(kimi): reject symlinked skills destination before install Add a destination symlink pre-check in KimiIntegration.setup() before super().setup() writes any SKILL.md. The base class only rejects a destination that escapes project_root after resolve(), so an in-tree symlinked .kimi-code/.kimi-code/skills (e.g. `-> .`) would still misdirect writes into an unintended in-tree location (./skills/). Extract the symlink-component walk into a shared _has_symlinked_component() helper and reuse it from _is_safe_legacy_dir(). Add a regression test. Also clarify that --migrate-legacy only migrates KIMI.md -> AGENTS.md when the agent-context extension is enabled, in the CLI help text and the integration docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Refactor formatting and simplify logic in Kimi integration * fix(kimi): reject symlinked target dir during legacy skills migration When the migration destination already exists, guard against a symlinked (or non-directory) target_dir before comparing SKILL.md bytes, so the comparison never follows a link outside the project root. Also skip a missing/non-file target SKILL.md explicitly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Zied Jlassi
|
b042d2a843 |
feat(extensions): verify catalog archive sha256 before install (#3080)
* feat(extensions): verify catalog archive sha256 before install Extension and preset archives were downloaded over HTTPS and unpacked (with Zip-Slip protection) but their bytes were never checked against a known digest. Trust rested entirely on TLS and the integrity of the release host, so a tampered or swapped archive from a compromised third-party release would be installed silently. Maintainers do not audit extension code, so consumer-side integrity is the only available defence. Catalog entries may now pin an optional `sha256` digest. When present, the downloaded archive is verified before it is written to disk and installed; a mismatch aborts with a clear error. Entries without `sha256` keep working unchanged (a DEBUG line records that the download was unverified), so the change is backwards compatible. The check runs on both download paths (extensions and presets) via a single shared helper so the two stay in parity. - Add `verify_archive_sha256` helper in shared_infra (digest match, `sha256:` prefix, case-insensitive; DEBUG log when no digest declared) - Enforce it in ExtensionCatalog.download_extension and PresetCatalog.download_pack, before the archive is written to disk - Document the optional `sha256` field in the publishing guides - Tests: helper unit tests + matching/mismatch/no-digest on both paths Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Assisted-by: AI * fix(extensions): harden sha256 parsing and tidy download test mocks Follow-up to the review on #3080: - shared_infra.verify_archive_sha256: strip only a literal `sha256:` algorithm prefix (case-insensitive) instead of `split(':', 1)[-1]`, which silently dropped any prefix — so `md5:<64-hex>` was accepted as if it were a valid SHA-256. Validate that the declared value is exactly 64 hex characters and raise a clear error otherwise, and compare with `hmac.compare_digest` for a constant-time check. Add tests covering a malformed digest and a non-`sha256:` prefix (both previously accepted). - Download test helpers: configure the context-manager mock via `__enter__.return_value`/`__exit__.return_value` rather than assigning a `lambda s: s`, which is clearer and independent of the invocation arity. Assisted-by: AI Signed-off-by: Zied Jlassi (Architect AI) <6190550+zied-jlassi@users.noreply.github.com> * fix(extensions): reject a declared-but-empty sha256 instead of skipping verification verify_archive_sha256 skipped on any falsy expected value, so a present-but-empty digest (e.g. sha256: "" reached via ...get("sha256")) silently disabled the integrity check instead of surfacing the authoring error. Guard on expected is None so only an absent digest skips; blank/whitespace/bare-prefix values fall through to the 64-hex validation and are rejected. Adds a regression test. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> * docs(shared_infra): clarify _SHA256_HEX_RE accepts and normalizes uppercase The comment described the regex as matching '64 lowercase' hex characters, but verify_archive_sha256 lowercases the declared value (raw.lower()) before matching, so an uppercase digest is accepted and normalized rather than rejected. Clarify the comment to avoid misleading future readers. Addresses Copilot review feedback on shared_infra.py. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> * test(presets): cover the no-sha256 backwards-compatible path Address Copilot review: download_pack's optional sha256 verification was tested for match/mismatch but not the backwards-compatible path where a catalog entry has no sha256 (pack_info.get("sha256") is None). Add a no-sha256 test mirroring the extensions coverage so the helper never silently becomes mandatory for presets. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> --------- Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Signed-off-by: Zied Jlassi (Architect AI) <6190550+zied-jlassi@users.noreply.github.com> |