161 Commits

Author	SHA1	Message	Date
Ali jawwad	e4318a3d1a	fix(catalogs): validate the port in the shared catalog-URL validator, like its mirrors do (#3804) ... * fix(catalogs): validate the port in the shared catalog-URL validator `CatalogStackBase._validate_catalog_url()` reads `parsed.hostname` inside its `try/except ValueError` but never reads `parsed.port`. `urlparse()` and `.hostname` do not perform port validation — only `.port` does — so a catalog URL with a non-numeric or out-of-range port passes validation. Every implementation that documents itself as mirroring this function already reads `.port` inside the same try: workflows/catalog.py (4 sites), bundler/services/adapters.py (2), bundler/commands_impl/catalog_config.py, and commands/bundle/__init__.py. The shared base — inherited by ExtensionCatalog and IntegrationCatalog — is the only one without it. The accepted URL then escapes as a raw `http.client.InvalidURL`, which is neither `urllib.error.URLError` nor `json.JSONDecodeError` (the only two the fetcher converts), so it surfaces as an unhandled traceback rather than the validator's normal error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(catalogs): describe both bad-port failure modes accurately The comment attributed both malformed-port cases to http.client.InvalidURL. Only a non-numeric port raises that (when the connection object is built); an out-of-range port constructs fine and fails later in the socket layer. Measured: example.invalid:notaport -> http.client.InvalidURL: nonnumeric port example.invalid:65536 -> HTTPSConnection() OK, connect() fails Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-30 13:41:17 -05:00
Marsel Safin	e916fd1b3b	fix: preserve unreadable event config files (#3861) ... Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-30 09:08:27 -05:00
Marsel Safin	227b4f5e11	fix: normalize non-UTF-8 integration manifests (#3862) ... Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-30 07:46:23 -05:00
github-actions[bot]	8394c8d536	[bug-fix] Fix upgrade-overwrites-copilot-skills: pass force=True to extension skill re-registration after upgrade (#3853) ... * Fix upgrade-overwrites-copilot-skills: pass force=True to extension skill re-registration after upgrade Apply the remediation from the bug assessment on issue #3849. _register_extension_skills() had a skip guard that refused to overwrite existing SKILL.md files (protecting user customizations). In the upgrade path, setup() regenerates all core-template SKILL.md files first, then calls register_enabled_extensions_for_agent(). The guard then sees those freshly-written core files as 'existing' and skips every extension, leaving only core template content on disk. Fix: add force: bool = False to _register_extension_skills() and thread it through register_enabled_extensions_for_agent() and _register_extensions_for_agent(). In integration_upgrade(), pass force=True so extension content layers on top of the just-regenerated core files. The force flag is off-by-default so plain extension add still protects user-modified skill files. Refs #3849 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding 'Unused local variable' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> * test: add end-to-end regression guard for upgrade-overwrites-copilot-skills (#3849) The existing regression tests in TestRegisterExtensionSkillsForceFlag exercise the new force parameter at the helper level, so without the fix they fail only with a TypeError (unknown kwarg) rather than on the user-facing behaviour. Add a command-level test that runs 'specify integration upgrade copilot --skills --force' end-to-end and asserts the installed git extension's SKILL.md is restored (with its extension content, not a bare core-template stub) when the skill directory already exists — the exact skill_dir_preexists path the bug depends on. The test fails on pre-fix source (the skill is never recreated) and passes with the fix, so it is a genuine behavioural regression guard rather than an API-surface check. Refs #3849 Assisted-by: GitHub Copilot (model: claude-opus-4.8, autonomous) --------- 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: 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>	2026-07-29 10:03:14 -05:00
Ali jawwad	89126f3a33	fix(integrations): don't abort uninstall when the manifest can't be deleted (#3805) ... `IntegrationManifest.uninstall()` guards every tracked-file `path.unlink()` with `except OSError: skipped.append(path)`, but the manifest's own `manifest.unlink()` is bare. The manifest is deleted *last*, so an undeletable manifest (read-only file, a directory left at the path, a Windows lock) raises after the tracked files are already gone. The caller loses the `(removed, skipped)` result and never runs its post-uninstall bookkeeping — reassigning the default integration, rewriting/removing `integration.json`, clearing init options — leaving a removed integration still recorded as installed. Report it in `skipped` like any other file we could not remove, mirroring the `path.unlink()` guard above and the same `except OSError: skipped.append(...)` pattern in kimi's legacy-directory cleanup. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-29 09:28:29 -05:00
Marsel Safin	b7b0e966cc	fix(integrations): preserve non-UTF-8 VS Code settings (#3833) ... Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-29 08:43:03 -05:00
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 c0fe0e43): our side now makes no net change to them relative to the merge-base, so a future upstream merge takes theirs on both without conflict. Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): compose --events into Copilot/Devin options() (#8, #9) Copilot and Devin are event-capable, but their options() overrides returned only --skills without calling super(), so the base class never declared --events. The documented --integration-options "--events false" opt-out was therefore rejected as unknown for both adapters. Both now compose with super().options() (mirroring Codex and Cursor) so --events is declared alongside --skills. Added a TestEventCapableOptionsCompo sition test class asserting --events appears in Copilot, Devin, Cursor, and Codex options() output. Refs: PR #3704 Copilot review 4790195897 (findings #8, #9) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): Cursor version field, matcher grouping, Copilot cross-OS Address three Copilot review findings on native-config generation: - #7: Cursor's .cursor/hooks.json schema requires top-level "version": 1, but json-flat used _merge_json_fragment() which only writes hooks, so a freshly generated file was missing the required schema version. Added a version kwarg to _merge_json_fragment (preserving a user's value if present) and the Cursor json-flat branch now passes version=1. - S3: json-nested placed all handlers under the first handler's matcher, so two extensions registering the same event with different matchers both ran for the first matcher and neither for the later. Handlers are now grouped by distinct matcher, emitting one matcher-group per matcher (handlers sharing a matcher stay in one group). - S4: Copilot's bash and powershell fields both received the same host-resolved command, so a config generated on Linux wrote a POSIX venv path into the PowerShell hook (and vice-versa). _dispatcher_command gains a target_os kwarg; Copilot now emits an independent POSIX interpreter (python3) for bash and a Windows interpreter (python) for powershell, so the checked-in config works on either OS. Tests: added TestCursorJsonWriting (version present + preserved) and matcher-grouping regressions (per-distinct-matcher, shared-matcher); updated the Copilot generation test to assert bash != powershell with OS-appropriate interpreters. Refs: PR #3704 Copilot review 4790195897 (findings #7, S3, S4) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): anchor py scripts and prefix ps launcher in command runner Address two Copilot review findings on the core command runner: - S2: the py variant called build_python_invocation() on the raw scripts: command string, which left 'scripts/...' anchored at the project root instead of under .specify/ (or .specify/extensions/<id>/). Every event command in a project configured with --script py launched a nonexistent project-root path. The py branch now shares the same base-anchoring as sh/ps and prepends the resolved interpreter as argv (no shell quoting needed for subprocess.run(shell=False)). - S6: the ps variant returned the .ps1 path as the executable, but Windows subprocess.run(shell=False) cannot execute a PowerShell script directly, so event dispatch failed on the default Windows script type. The ps branch now prefixes argv with 'pwsh -File' (PowerShell 7+), falling back to 'powershell -File' (Windows PowerShell) when pwsh is absent. Tests: added test_py_variant_anchored_under_specify and test_ps_variant_prefixed_with_powershell_launcher covering the new argv shapes (interpreter + .specify-anchored path; launcher -File + path). Refs: PR #3704 Copilot review 4790195897 (findings S2, S6) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): skip-tracking on parse fail, drop dispatcher claim on retain, honor --events false in refresh, preserve layers on invalid override Address four Copilot review findings on merge/teardown/refresh safety: - S5: _merge_json_fragment/_merge_opencode_plugin_ref/_merge_copilot_json now return bool (wrote). Install branches skip manifest.record_existing() and created.append() when a merge was skipped on parse failure, so a user's JSONC/malformed native config is not tracked and manifest.uninstall() can't later delete the untouched file. - S1: remove_integration_events now drops this integration's manifest claim on the shared dispatcher (manifest.remove) even when the file is retained because another integration references it. Previously the retained file stayed tracked, so the subsequent manifest.uninstall() in teardown() saw the matching hash and deleted the file another integration still depended on. The unit test now exercises full teardown() (not just remove_integration_events) to cover the gap. - S7: refresh_integration_events reads each integration's stored parsed_options via _resolve_integration_options and passes them to resolve_events, so a persisted --events false is honored across extension add/enable/disable instead of being discarded (which re-enabled events the user had disabled). - #10: an invalid override entry now abandons the entire override and keeps the accumulated built-in + extension layers, instead of resetting resolved_override to {} and assigning that empty map to events (which silently disabled all hooks on a single typo). Only a fully-valid override (including an explicit events: {}) replaces the prior layers. Tests: added TestOverridePreserveLayers (invalid entry keeps layers; explicit empty disables), TestSkippedMergeNotTracked (JSONC not recorded), and TestDispatcherManifestClaimDroppedOnRetain (full teardown keeps dispatcher when another integration references it). Added S7 refresh-honors-events-false regression. Refs: PR #3704 Copilot review 4790195897 (findings S5, S1, S7, #10) Assisted-by: opencode (model: glm-5.2, autonomous) * test(extensions): update stale validation-message assertion The 'no commands/hooks/events' validation message changed to 'Extension must provide at least one command, hook, or event' when the events feature added a third provider kind, but test_no_commands_no_hooks still matched the old 'must provide at least one command or hook' text and failed on every CI job. Update the regex to the current message. Refs: PR #3704 CI failure (test_extensions.py:579) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): forced-teardown data safety, manifest-driven command resolution, toml teardown safe-dest Address three findings from Copilot review 4791088500: - S9: _remove_native_event_hooks now unconditionally drops this integration's manifest claim on the native config, not only when the file was deleted. Previously a config whose owned entries were cleaned but user content retained stayed tracked, so teardown(force=True) -> manifest.uninstall( force=True) deleted the entire user-owned settings file. This is the config-file mirror of the earlier shared-dispatcher fix. - S8: _find_command_template resolved extension event commands via a broken registry lookup (the registry stores per-agent registered_commands name-lists, not a {name, file} map) and a file-stem scan that only matched when the .md stem equaled the command name. A manifest mapping speckit.selftest.extension -> commands/selftest.md resolved as missing. It now enumerates installed extensions via ExtensionManager.get_extension() and matches provides.commands[].name -> file, with the directory scan and core-template lookups kept as fallbacks. - R3: _remove_toml_entries now validates the destination with _ensure_safe_destination before read/write, matching the merge path, so a symlink swap of .codex/config.toml after install can't make teardown overwrite a file outside the project. Tests: forced full teardown preserves a user settings file; an extension command whose file stem differs from its name resolves via the manifest; TOML teardown rejects a symlinked config destination. Refs: PR #3704 Copilot review 4791088500 (findings S8, S9, R3) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): subprocess cwd, shell quoting, TOML matcher escaping, Tabnine ms Address four findings from Copilot review 4791088500: - R1: the generated dispatcher and resolve_and_run_event_command now run their subprocesses with cwd set to the dispatcher-derived project root. Previously 'specify event run' (and the resolved script) inherited the agent's working directory, but event_run resolves the project via Path.cwd(), so a hook fired from a subdirectory targeted the wrong project and reported the command missing. - R2: _dispatcher_command now shell-quotes each component (interpreter, command, event) for the target shell (POSIX via shlex.quote; PowerShell via single-quoted literals with doubled quotes). An interpreter path containing spaces or an extension/override command containing shell metacharacters is passed as a single argument instead of being reinterpreted by the native hook shell. Claude's prefix is left unquoted so the shell still expands it (prefix + relative path are fixed, safe strings). - R4: the Codex TOML matcher is now rendered through the shared TOML escaper like command, so a matcher containing a quote/backslash/newline/control character no longer produces malformed config.toml. - R5: Tabnine declares events_timeout_unit='ms' (its hook schema mirrors Gemini's BeforeTool/AfterTool), so the 60s default becomes 60000ms instead of timeout: 60 (60 ms), which would terminate the dispatcher immediately. Tests: cwd-forced execution from a subdirectory; POSIX/PowerShell quoting of metacharacter and space-bearing components; TOML matcher with a quote parses cleanly; Tabnine timeout converts to 60000. Updated the Copilot generation test for the new quoted args. Refs: PR #3704 Copilot review 4791088500 (findings R1, R2, R4, R5) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): POSIX dispatcher path constant + platform-agnostic tests Three Windows test failures, one a real cross-OS bug: - W1 (bug): EVENTS_DISPATCHER_REL was str(Path('.specify')/'events.py'), which yields '.specify\events.py' on Windows. Manifest keys are stored in POSIX form (.as_posix()), so 'dispatcher_rel in manifest.files' was always False on Windows: the shared-dispatcher manifest-claim drop was skipped and manifest.uninstall(force=True) deleted the dispatcher another integration still depended on. Make it a POSIX constant (.as_posix()) so it matches manifest keys on every platform. - W2/W3 (tests): the py/ps argv assertions used endswith() and an exact launcher-name set that broke on Windows backslash paths and the pwsh.EXE/full-path launcher returned by shutil.which. Compare in POSIX form and match the launcher by case-insensitive stem. Refs: PR #3704 Windows CI failures Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): override layer preservation, matcher validation, event command-ref canonicalization Address four Copilot review findings: - C4: a malformed override handler (e.g. "stop: []" or "stop: bad-value") normalizes to no handlers. Previously the entry was skipped and the override still adopted, so an override whose only entry was malformed silently disabled every built-in and extension hook. The empty-handler case now abandons the whole override (keeps prior layers); an explicit "events: {}" (no entries) remains a valid disable. - C6: a non-mapping integration entry (e.g. "claude: bad") was coerced to "events: {}" and treated as a valid explicit disable. It now warns and abandons the override, keeping the accumulated layers. Only an explicitly present, mapping-valued "events" field replaces the prior layers. - C10: matcher is now validated as a string (or absent) in both validate_events (manifest) and _validate_resolved_event (override). A non-string matcher such as "matcher: []" previously passed validation but crashed by_matcher.setdefault(matcher, ...) with TypeError: unhashable type, aborting init or refresh. - C11: ExtensionManifest._validate now applies the same rename + alias-lift canonicalization to event command references that it already applies to hook references. An event referencing an auto-corrected command (e.g. my-ext.boot -> speckit.my-ext.boot) previously kept the obsolete name, so dispatch reported no command and the event silently no-oped. Tests: empty-handler/non-mapping override preserves layers; non-string matcher rejected in manifest and abandoned in override; event command ref lifted to canonical form with a warning. Refs: PR #3704 Copilot review (findings C4, C6, C10, C11) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): protect shared dispatcher from stale cleanup, delete Cursor version stub, non-destructive refresh Address three Copilot review findings: - C3: the shared .specify/events.py dispatcher is now in events_stale_exclusions(). It is written into every event-capable integration's manifest but reference-counted across them; an upgrade with --events false omits events.py from the new manifest, so the generic stale pass would delete it without the refcount check, breaking any other installed event-capable integration. Its deletion is left to remove_integration_events(), which checks the refcount. - C5: _remove_json_entries now deletes a Spec-Kit-created Cursor file that retains only {"version": 1} after all owned hooks are removed (we added the version field), mirroring _remove_copilot_entries. Previously the generic remover only deleted a literally-empty object, so clean teardown left a generated stub behind. - C12: refresh_integration_events now resolves first and calls install_integration_events once, instead of running the destructive _remove_native_event_hooks pre-step before resolution. A later failure (invalid destination, write error, formatter error) no longer destroys the working native config before the new one is written. install_integration_events already removes stale Specify-marked entries and handles an empty map (stripping prior hooks), so the pre-step was both unsafe and redundant. Tests: dispatcher in stale exclusions; Cursor version-only stub deleted on teardown; refresh failure preserves the pre-existing config (no pre-strip). Refs: PR #3704 Copilot review (findings C3, C5, C12) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): host target uses POSIX quoting, Claude dispatcher double-quoted, & for windows Address two Copilot review findings on the shell-quoting added in the prior round (R2): - C1: _shell_quote("host") now always uses POSIX shlex.quote, not PowerShell single-quoting on Windows. The single-command-string formats (Claude/Gemini/Qwen/Devin/Tabnine) are run via the agent's POSIX-ish shell (Git Bash on Windows), and a single-quoted 'python' is not invoked as a command by PowerShell without the call operator — so generated hooks failed to launch the dispatcher on Windows. Safe tokens pass through bare (python3, speckit.ext.cmd) on every platform. PowerShell single-quoting is now used only for the explicit target_os="windows" (Copilot's powershell field), where the quoted interpreter is prefixed with "& " so it is actually invoked. - C2: Claude's ${CLAUDE_PROJECT_DIR} dispatcher path is now double-quoted ("${CLAUDE_PROJECT_DIR}/.specify/events.py") so the variable still expands (double quotes allow expansion in POSIX shells) but a project path containing spaces no longer word-splits and breaks dispatcher launch. Tests: host target never emits PowerShell quotes; windows target carries the & call operator; Claude dispatcher is double-quoted; updated Copilot generation assertions for the &-prefixed powershell command. Refs: PR #3704 Copilot review (findings C1, C2) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): opencode TS plugin resolves dispatcher from directory, execFileSync argv, forwards input+output Address three Copilot review findings on the opencode TS plugin: - C8: the dispatcher and interpreter are now resolved per-project at plugin load from the `directory` OpenCode passes to the plugin factory, not process.cwd(). OpenCode may be launched from a parent directory or host another workspace, in which case process.cwd() pointed at the wrong project and every event failed. The resolver prefers a project-local venv interpreter, then falls back to python3. - C9: the dispatcher is launched with execFileSync and an argv array [interpreter, dispatcher, command, event] instead of a shell command string built by interpolating the interpreter/command/event into a template literal. Command/event strings are only validated as non-empty, so quotes or backticks could previously break the generated TypeScript and shell metacharacters could execute outside the dispatcher; an interpreter path with spaces also failed. No shell is involved now. - C7: tool callbacks now forward both `input` and `output` to runEvent (combined into one JSON payload), so pre_tool_use can inspect the tool arguments and post_tool_use can inspect the result — the primary payload for those events. Previously only `input` was forwarded. Tests: plugin resolves dispatcher/interpreter from `directory` (no process.cwd() path.join), uses execFileSync (no shell string), and forwards output to runEvent for both pre/post_tool_use. Refs: PR #3704 Copilot review (findings C7, C8, C9) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): Qwen ms timeout, Devin root-nested format, Copilot agentStop Address three Copilot review findings on adapter mappings (verified against each agent's published hook documentation): - U1: Qwen Code command hooks measure timeout in milliseconds (default 60000), per the Qwen Code hooks docs. The adapter previously inherited the seconds default, so every generated handler got timeout: 60 (60 ms) and was killed before the dispatcher could start. Declare events_timeout_unit="ms". - U2: Devin's .devin/hooks.v1.json is a root event map ({"PreToolUse": [...]}) with no top-level "hooks" wrapper (the docs state "the hooks object is the entire file"). The adapter reused json-nested, which writes events under a "hooks" key Devin never reads. Add a json-root-nested format with a matching writer (_merge_json_root) and remover (_remove_json_root_entries) that operate on the root event keys, sharing the matcher-grouping, marker, and JSONC-abort behavior of the nested variants. - U3: Copilot CLI supports the canonical per-turn stop lifecycle as native agentStop; add "stop": "agentStop" to the mapping so an extension's stop handler fires for Copilot. Tests: Qwen timeout converts to 60000; Devin events written at the root (no "hooks" wrapper) and teardown preserves user root entries; Copilot stop maps to agentStop. Refs: PR #3704 Copilot review (findings U1, U2, U3) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): collect events via validated manifest, surface refresh failures Address two Copilot review findings: - R1: collect_extension_events now reads events from a validated ExtensionManifest (whose command refs were canonicalized at install validation, C11) instead of the raw extension.yml YAML. Previously an event command ref like my-ext.boot was normalized to speckit.my-ext.boot during install validation, but the on-disk YAML kept the obsolete name; refresh then emitted it and _find_command_template could not match it, leaving the hook silently inert. Registry-tracked extensions use the validated manifest; on-disk extensions not yet in the registry fall back to the raw YAML (preserving the partial-staged-install scan behavior). - R3: refresh_integration_events now accumulates per-integration failures and raises EventRefreshError at the end (after refreshing the others) so the extension lifecycle commands (add/remove/enable/disable) can't claim an extension was fully deactivated while a stale native hook may still be active. A new _refresh_events_and_warn helper surfaces the aggregated failures as a warning at each call site without aborting the overall command (the extension was already added/removed/enabled/disabled). Tests: event command ref canonicalized via the validated manifest; refresh failure raises EventRefreshError (aggregated) while still preserving the pre-existing config. Refs: PR #3704 Copilot review (findings R1, R3) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): probe venv for specify_cli before selecting it; python on Windows Address two Copilot review findings on interpreter resolution: - R2: the dispatcher's _find_specify and the opencode TS resolver both selected a project-local venv python and ran `-m specify_cli` without checking that specify_cli is importable there. In a typical project where Spec Kit is installed globally (or via uv tool) but the project has its own unrelated virtualenv, every event invoked that interpreter and failed instead of reaching the PATH `specify` fallback. Both now probe the candidate interpreter (subprocess `import specify_cli` / execFileSync probe) before selecting it, falling through to the fallback when the venv lacks Spec Kit. - S2: the opencode TS PATH fallback was always `python3`, which is commonly unavailable on Windows. It is now `python` on Windows (process.platform === 'win32') and `python3` on POSIX. Tests: the generated dispatcher contains the _has_specify_cli probe and the PATH fallback; the opencode TS plugin probes for specify_cli and uses a platform-appropriate PATH interpreter. Refs: PR #3704 Copilot review (findings R2, S2) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): serialize opencode TS plugin string literals as JSON Address Copilot review finding S1: command and matcher values come from user/extension YAML but were interpolated into single-quoted TypeScript literals without escaping. A quote, backslash, or backtick in a command or matcher produced invalid generated TypeScript and could inject code into the plugin. _build_opencode_plugin now serializes every interpolated value (command, event name, native hook key, matcher tool names) as a JSON string literal via json.dumps, which produces a valid double-quoted, fully-escaped TS/JS string. Tests: a command and matcher containing quotes/backticks render inside JSON double-quoted literals; the dangerous single-quoted form is absent. Updated the forwards-output test for the new double-quoted literals. Refs: PR #3704 Copilot review (finding S1) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): thread per-handler timeout through dispatcher, bash launcher for sh on Windows Address two Copilot review findings: - S4: the dispatcher and inner runner both hardcoded timeout=120, so a valid handler configured with a timeout above 120 seconds could never run for its full duration. The resolved per-handler timeout now flows through the chain: _dispatcher_command appends it (in the integration's native unit, plus a small buffer) as a 4th argument; the generated dispatcher reads sys.argv[3] and uses it for its inner subprocess and the `event run` invocation; `event run` accepts a timeout argument and passes it to resolve_and_run_event_command, which uses it for the script subprocess. Defaults to 120s when absent (backward compat with already-deployed dispatchers that don't pass the arg). - S5: for a project configured with the sh script type on Windows, subprocess.run(shell=False) cannot execute a .sh file directly (chmod doesn't change that). The sh variant now prefixes a bash/sh launcher (resolved via shutil.which) on Windows, mirroring the ps branch's pwsh -File handling. Tests: dispatcher reads the timeout arg and uses it; the native command appends the resolved timeout; the sh variant uses a launcher on Windows. Refs: PR #3704 Copilot review (findings S4, S5) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): delete shared dispatcher when last event integration disables events Address Copilot review finding S3: the empty-resolved-map install path (--events false upgrade, or override disabling events) stripped prior native hooks but left the shared dispatcher behind. Because the new manifest no longer claims it and stale cleanup excludes it (C3), .specify/events.py became permanently orphaned when this was the last event-capable integration — uninstall could not remove it. Extracted the dispatcher refcount cleanup into _cleanup_shared_dispatcher (shared by remove_integration_events and the empty-map install path) and called it from the empty-map path so the dispatcher is deleted when no other installed event-capable integration's manifest references it, while still being retained when another integration does. Tests: an --events false upgrade of the last event integration deletes the dispatcher; with another integration still referencing it, the dispatcher is retained. Refs: PR #3704 Copilot review (finding S3) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): map user_prompt_submit/stop for Gemini and Tabnine Address two Copilot review findings on adapter mappings: - S6: Gemini exposes BeforeAgent for the per-turn prompt-submit lifecycle point (verified against Gemini CLI's hooks docs — BeforeAgent fires after the user submits a prompt, before planning). The mapping omitted user_prompt_submit, so valid extension handlers were skipped. Added user_prompt_submit -> BeforeAgent. - S7: Tabnine's Gemini-compatible schema also provides BeforeAgent and AfterAgent, but the mapping omitted user_prompt_submit and stop. Added user_prompt_submit -> BeforeAgent and stop -> AfterAgent so those extension events fire instead of being warned about and skipped. Tests: Gemini and Tabnine mappings include BeforeAgent/AfterAgent. Refs: PR #3704 Copilot review (findings S6, S7) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): correct timeout unit threading through dispatcher and opencode TS Address two Copilot review findings on the per-handler timeout threading added in the prior round (S4): - R2: _dispatcher_command passed _native_timeout(timeout_seconds) as the dispatcher's 4th argument, but the dispatcher interprets that argument as seconds. For Gemini/Qwen/Tabnine (ms adapters), 60 seconds became 60000 seconds (~16h). It now passes the raw seconds (no unit conversion). The +5s buffer moves to the native hook timeout field (_native_timeout(seconds + EVENT_TIMEOUT_BUFFER)) so the agent's outer cap fires after the dispatcher's inner subprocess timeout — letting the inner kill its child cleanly instead of being killed mid-flight (which orphaned the grandchild script process). - S3: the opencode TS runEvent hardcoded timeout: 60000 (60s) and invoked the dispatcher without its timeout argument, so handlers configured above 60s were killed early while the inner runner defaulted to 120s. runEvent now accepts a timeoutSec parameter (seconds); execFileSync uses (timeoutSec + buffer) * 1000 ms and appends String(timeoutSec) to the dispatcher argv, so both layers honor the per-handler timeout. Tests: the dispatcher arg is raw seconds for ms adapters (60, not 60000); the native timeout field carries the buffer (65 for a 60s Claude handler); opencode runEvent threads the per-handler timeout as the 5th argument. Refs: PR #3704 Copilot review (findings R2, S3) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): skip disabled extensions in _find_command_template and disk fallback Address Copilot review finding S1: _find_command_template resolved event commands without filtering enabled: false — the registry loop used registry.keys() and the raw directory fallback could also rediscover disabled extensions. If native cleanup is skipped (e.g. a JSONC config cannot be parsed), a stale hook would therefore continue executing a disabled extension. Extracted the disabled-ID logic into _disabled_extension_ids (shared with collect_extension_events) and applied it to both the manifest-resolution loop and the on-disk fallback scan in _find_command_template, so a disabled extension's command is never resolved for dispatch. Tests: a disabled extension's command resolves to None via both the manifest loop and the disk-fallback path. Refs: PR #3704 Copilot review (finding S1) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): delete shared dispatcher regardless of fresh manifest claim Address Copilot review finding S2: _cleanup_shared_dispatcher gated the no-other-references deletion on `dispatcher_rel in manifest.files`. An `integration upgrade --integration-options "--events false"` passes a fresh manifest (created in _migrate_commands) that never recorded the dispatcher, so the condition was false even though the old on-disk manifest owned the file — and stale cleanup explicitly excludes it (C3), leaving .specify/events.py orphaned after the last integration disabled events. The refcount deletion now runs independently of whether the new manifest contains the key; manifest.remove() stays conditional (a no-op when the key is absent). Tests: an upgrade passing a fresh manifest (no dispatcher claim) still deletes the shared dispatcher when no other integration references it. Refs: PR #3704 Copilot review (finding S2) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): refresh native event config after extension update Address Copilot review finding S4: the _refresh_events_and_warn helper was wired to extension add/remove/enable/disable, but not to extension_update, which replaces the installed extension.yml (remove + install_from_zip). If an update adds, removes, or changes event declarations, native configs remained stale until a manual integration upgrade. extension_update now refreshes once after the update loop finalizes its successful updates (skipped on rollback/failure), mirroring the other lifecycle commands. Refs: PR #3704 Copilot review (finding S4) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): make the dispatcher self-contained for one-time/temporary installs Address Copilot review finding R1: the dispatcher required a persistent `specify` executable at runtime. The supported one-time flow runs `specify init` through a temporary `uvx` environment that is discarded, so generated hooks later reached the PATH fallback with no `specify` on PATH and every event failed. The generated .specify/events.py is now self-contained: - Preferred path: it imports specify_cli.events.resolve_and_run_event_command when the package is importable (durable pip/pipx/uv-tool install), which handles extension manifests whose file stem differs from the command name and the project's custom script selection, staying in sync with the CLI. - Fallback path: an inline stdlib-only resolver finds the command template, parses its scripts: frontmatter, resolves the project's script variant (reading .specify/init-options.json directly), and runs the script with the correct launcher (pwsh/bash/interpreter), so one-time and temporary installs work without a persistent `specify` executable on PATH. The `event run` CLI command remains available for manual use; the dispatcher no longer depends on it. Tests: the dispatcher delegates to specify_cli when importable and falls back to the inline resolver when it is not; the inline fallback finds the command template and runs its script end-to-end (shadowing specify_cli with an empty package to force the fallback); the preferred path also runs end-to-end. Refs: PR #3704 Copilot review (finding R1) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): validate safe destination on all removers and teardown unlinks Address Copilot review findings (inline #1, suppressed #2, #3): - Guard all removers (_remove_json_entries, _remove_copilot_entries, _remove_json_root_entries, _remove_opencode_entries, _remove_native_event_hooks), _cleanup_shared_dispatcher, and remove_integration_events with _ensure_safe_destination(dst) before reading, rewriting, or unlinking. - Prevents teardown or removal operations from overwriting or unlinking external files if a config file, plugin path, or .specify directory is replaced with a symlink post-installation. Tests: added unit tests in TestSafeWriteDestination covering JSON config, OpenCode plugin, and TOML teardown symlink rejection. Refs: PR #3704 Copilot review (findings inline #1, suppressed #2, #3) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): manifest-driven resolution and disabled-extension filter in dispatcher template Address Copilot review finding (suppressed #1): - In _EVENTS_DISPATCHER_TEMPLATE's _find_command_template, read .specify/extensions/.registry to identify disabled extensions (enabled == false). - Parse provides.commands in each enabled extension's extension.yml to match command_name to its declared file, so commands whose file stem differs from the command name (e.g. speckit.selftest.extension -> commands/selftest.md) resolve correctly when specify_cli is unavailable (one-time uvx installs). - Skip disabled extensions in both manifest-driven and on-disk fallback scans. Refs: PR #3704 Copilot review (finding suppressed #1) Assisted-by: opencode (model: glm-5.2, autonomous) * fix(events): positive integer timeout validation and OpenCode multi-handler error aggregation Address Copilot review findings (suppressed #4, #6): - In validate_events and _validate_resolved_event, validate that timeout (when present) is a positive integer (isinstance(t, int) and not isinstance(t, bool) and t > 0). Rejects string, boolean, zero, or negative timeouts at manifest and override validation time instead of crashing during setup/refresh. - In _build_opencode_plugin, wrap each runEvent invocation inside _ev() in a try/catch block, collect error messages, and throw an aggregate error at the end if any handler failed. Guarantees that all handlers for an event execute to completion even if an earlier handler throws. Tests: added TestTimeoutValidation testing string, boolean, and zero timeout rejections; updated OpenCode plugin merging tests for try/catch error collection. Refs: PR #3704 Copilot review (findings suppressed #4, #6) Assisted-by: opencode (model: glm-5.2, autonomous)	2026-07-29 08:00:26 -05:00
Ali jawwad	6ef96373e2	fix(integrations): reject empty --commands-dir in generic raw_options (#3714) ... * fix(integrations): reject empty --commands-dir in generic raw_options GenericIntegration._resolve_commands_dir has a parity gap: the parsed-options branch guards emptiness (`if commands_dir:`), but the raw_options fallback returned the value verbatim with no check. So `--integration-options= "--commands-dir="` (or `--commands-dir ""`) resolves to `""`, which makes setup() compute `dest = project_root / "" == project_root` and write every speckit command file (specify.md, plan.md, ...) directly into the PROJECT ROOT — silently bypassing the documented "--commands-dir is required" contract and polluting the repo root. Apply the same non-empty guard to the raw_options branch so an empty value falls through to the existing "required" ValueError on every input form. Non-empty values resolve exactly as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(integrations): reject a BLANK --commands-dir, not just an empty one Self-review follow-up: bare truthiness only closes the empty-string subset. A whitespace-only value passed both branches (verified: raw "--commands-dir ' '" returned ' ', parsed {"commands_dir": " "} returned ' '), so command files still landed in a directory literally named " " instead of failing with the documented "required" error. Require a non-BLANK value and normalize the padding, in the parsed branch as well as raw_options so the two cannot drift apart -- a padded but real value (" .myagent/cmds ") now resolves to ".myagent/cmds" rather than being rejected, matching how other padded config references are normalized. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(integrations): use strip() only to test blankness, return the value verbatim Address review feedback: normalizing with strip() changed EXISTING valid values, contrary to this PR's "no behaviour change for valid usage" claim -- a quoted `--commands-dir ' commands '` previously targeted the literal ` commands ` directory and would have started writing to `commands` instead. The blankness test still uses strip(), but the accepted value is now returned unchanged, so the fix stays limited to empty/blank input. Test updated accordingly: a padded non-blank value must round-trip verbatim (quoted in raw_options, since shlex.split() consumes unquoted padding before this code sees it). 🤖 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>	2026-07-28 17:25:04 -05:00
Ali jawwad	3dad624e5d	fix(integrations): render hyphenated /speckit-<name> for Droid (always-slash agent) (#3688) ... * fix(integrations): render hyphenated /speckit-<name> for Droid (always-slash agent) DroidIntegration is an always-skills agent: it installs commands as .factory/skills/speckit-<name>/SKILL.md and its build_command_invocation returns the hyphenated /speckit-<name>. But "droid" was missing from every _invocation_style set, so is_slash_skills_agent("droid", True) returned False and both HookExecutor._render_hook_invocation and `specify init` next-steps fell through to the dotted /speckit.<name> form — a command Droid never registers. Add "droid" to ALWAYS_SLASH_AGENTS, matching its always-skills siblings grok/trae/zed/devin (each added there by their own integration PR; droid's #3587 omitted it). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(integrations): assert Droid is ALWAYS-slash (disabled case too) Address review: the test only covered ai_skills=True, which would also pass if Droid were miscategorized as CONDITIONAL_SLASH. Add the ai_skills=False assertion — True there is what distinguishes an ALWAYS_SLASH agent from a conditional one. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-28 17:14:31 -05:00
Quratulain-bilal	89e204ca3c	fix: use bounded read for integration catalog HTTP responses (#3763) ... * fix(skills): match closing frontmatter delimiter on its own line SkillsIntegration.setup parsed each command template's frontmatter with raw.split("---", 2). A bare substring split stops at the first `---` *anywhere*, so a template whose description embeds `---` (e.g. "Separate sections with --- markers") truncated the parsed frontmatter: later keys were dropped, the description fell back to the generic default, and the leftover frontmatter spilled into the skill body. Scan for the closing `---` on its own line instead, for both the description parse and the body strip. The frontmatter block is parsed unstripped so trailing newlines in literal (|) block scalars still survive, and the body slice keeps the newline after the marker so output stays byte-for-byte identical to the old split for well-formed templates. Adds regression tests covering the dashed-description truncation and the frontmatter-spilled-into-body cases. * fix: use bounded read for integration catalog HTTP responses The integration catalog fetch used unbounded resp.read() to read HTTP responses into memory. A malicious or misconfigured catalog server could return an arbitrarily large response causing OOM. Replace with read_response_limited() capped at MAX_JSON_METADATA_BYTES (1 MiB), consistent with how other JSON fetch paths in the codebase (_version.py, _github_http.py, authentication/azure_devops.py) already enforce bounded reads. Pass error_type=IntegrationCatalogError so oversized catalogs are caught by the existing per-entry recovery path in _get_merged_integrations() rather than aborting the entire merge. Add regression test verifying oversized responses are rejected as IntegrationCatalogError and that healthy catalogs remain usable.	2026-07-28 14:32:31 -05:00
Marsel Safin	2e44ed60e8	fix(integrations): escape catalog metadata in discovery output (#3772) ... * fix(integrations): escape catalog metadata in discovery output Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): escape unknown query IDs 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>	2026-07-28 10:20:31 -05:00
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	2026-07-28 15:48:40 +01:00
Faqeha Noor	9602ad2edf	fix(copilot): honor preset command template overrides (#3592) ... * fix(copilot): honor preset command template overrides * fix(copilot): resolve canonical preset command names --------- Co-authored-by: Faqeha Noor <faqehanoor022@gmail.com>	2026-07-28 08:26:12 -05:00
Eric X Engstfeld	7fc5b236c8	feat: Add Alquimia AI integration (#2734) ... * Add alquimia-ai as new integration: https://alquimia.ai * Fix test cases for alquimia-ai integration. Add alquimia-ai to workflow.yml * Add install url to alquimia-ai integration * Renamed alquimia-ai to alquimia (cli native denomination) * Fix unit tests for alquimia integration * Minor fix in alquimia integration * Fix typos and copilot findings * 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> * Update tests cases and lint formatting * Final fixes * Rename alquimia_ai to alquimia module integration * Make cli optional for alquimiia integration * resolve review comments * Fix copilot review * Minor fixes: naming, remove unused code * Update tests cases. Fix issues * Fix unit tests * Add alquimia context to default agent-context extension. Update cli requirment to support workflows * Fix hints (suggestion) * Add Alquimia AI as agent in github issue template. Fix unit tests * Address review comments. Update docs * Update test cases --------- Co-authored-by: Eric Engstfeld <ericengstfeld@Erics-MacBook-Pro.local> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>	2026-07-28 07:54:13 -05:00
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>	2026-07-27 14:09:40 -05:00
Noor ul ain	99dc915ae3	fix: escape Rich markup in catalog list output (#3738) ... The `catalog list` subcommands for workflows, workflow steps, presets, and integrations printed user-editable catalog fields (name/url/ description from the `*-catalogs.yml` files) through `console.print` with Rich markup enabled. Any bracketed content such as a description `Does [stuff] nicely` was parsed as a style tag and silently swallowed, and a malformed tag could raise while rendering. Route each untrusted field through the module's already-imported `escape` helper, matching the pattern already used by `extension catalog list`. Adds regression tests for all four commands that inject bracketed name/url/description and assert the brackets survive verbatim in the output. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-27 10:15:37 -05:00
WOLIKIMCHENG	ae82c74339	fix(kilocode): install commands under .kilo/commands (#3672) ... * fix(kilocode): write commands to .kilo/commands * fix: guard Kilo legacy command migration --------- Co-authored-by: root <kinsonnee@gmail.com>	2026-07-24 11:05:32 -05:00
Ali jawwad	391cc0dff8	fix(integrations): declare PiIntegration multi_install_safe (#3652) ... * fix(integrations): declare PiIntegration multi_install_safe PiIntegration writes only to its isolated, static root .pi/prompts, disjoint from every other integration, yet never declared multi_install_safe — so it inherited the IntegrationBase default False, leaving `specify integration status` in a permanent unsafe-multi-install ERROR state when pi is co-installed alongside another agent. Add `multi_install_safe = True`, mirroring the isolated MarkdownIntegration cohort (qwen, shai, qodercli) and the kiro-cli #3471 fix. The parametrized registry isolation contracts auto-include pi and pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(integrations): list pi in the multi-install-safe reference table Declaring PiIntegration multi_install_safe means the reference table in docs/reference/integrations.md (which states it lists all currently declared multi-install-safe integrations) should include it. Add the alphabetized pi row with its .pi/prompts isolation path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-24 09:48:30 -05:00
Ali jawwad	e9dfe900f6	fix(integrations): declare OmpIntegration multi_install_safe (#3650) ... * fix(integrations): declare OmpIntegration multi_install_safe OmpIntegration is a plain MarkdownIntegration whose files live only under its isolated, static root .omp/commands/, disjoint from every other integration. But it never declared multi_install_safe, so it inherited the IntegrationBase default False — leaving `specify integration status` in a permanent unsafe-multi-install ERROR state whenever omp is co-installed alongside another agent, with no acknowledgment path. Add `multi_install_safe = True`, mirroring the isolated MarkdownIntegration cohort (qwen, shai, qodercli, junie, kilocode) and the kiro-cli #3471 fix. The parametrized registry isolation contracts auto-include omp once the flag is set and pass (.omp/commands is isolated and its manifest disjoint). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(integrations): list omp in the multi-install-safe reference table Declaring OmpIntegration multi_install_safe means the reference table in docs/reference/integrations.md (which states it lists all currently declared multi-install-safe integrations) should include it. Add the alphabetized omp row with its .omp/commands isolation path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-23 11:07:40 -05:00
Ali jawwad	93dbf6d575	fix(integrations): recompute invoke_separator from retained parsed_options (#3664) ... with_integration_setting recomputed invoke_separator from the raw parsed_options argument. When only script_type changes (parsed_options and raw_options both None), the previously-stored parsed_options are retained on the setting, but the separator was derived from the None argument — dropping an options-dependent separator (e.g. Copilot --skills -> "-") back to the default ".", desynchronizing invoke_separator from the stored options. Derive the separator from current.get("parsed_options") — the options actually stored after the update — so it stays consistent in every branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-23 08:44:55 -05:00
Ali jawwad	9fb467f8de	fix(integrations): declare LingmaIntegration multi_install_safe (#3654) ... LingmaIntegration writes only to its isolated, static root .lingma/skills, disjoint from every other integration, yet never declared multi_install_safe — inheriting the IntegrationBase default False and leaving `specify integration status` in a permanent unsafe-multi-install ERROR state when lingma is co-installed alongside another agent. Add `multi_install_safe = True`, mirroring the structurally-identical trae/zcode SkillsIntegrations and the kiro-cli #3471 fix. The parametrized registry isolation contracts auto-include lingma and pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-22 13:37:37 -05:00
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)	2026-07-22 11:08:32 -05:00
Ali jawwad	a6743ab5e0	fix(integrations): validate cached catalog shape before returning it (#3627) ... * fix(integrations): validate cached catalog shape before returning it The catalog cache-read branch returned json.loads(cache_file) directly, skipping the shape validation the fresh-fetch branch enforces (dict root + 'integrations' mapping). A poisoned or older-format cache (e.g. {"integrations": []}) was therefore returned as-is and later crashed with 'AttributeError: list object has no attribute items' when the caller iterated integrations. Validate the cached object the same way; the raised ValueError is already caught by the surrounding handler, which drops the corrupt cache and refetches from source. Test: a fresh-but-mis-shaped cache is dropped and the valid source refetched (fails before: AttributeError). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(integrations): share one catalog-shape validator across cache and fetch Address review: the cache-read path checked only that the payload was a dict with a dict 'integrations', while the fresh-fetch path also required 'schema_version'. That asymmetry let an older/poisoned cache such as {"integrations": {}} (no schema_version) bypass the format contract instead of being dropped and refetched. Introduce a shared `_catalog_shape_error()` helper and use it in both paths so they enforce the same contract (dict + schema_version + dict integrations). The fresh path still raises IntegrationCatalogError with the "Invalid catalog format from <url>" prefix; the cache path still raises ValueError (caught to drop+refetch). Add a test for the missing-schema_version cache case. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(integrations): unit-test the shared catalog-shape validator directly Replace the integration-level missing-schema_version cache test (which was masked by multi-source merging — a sibling catalog source still supplied the entry, so it passed regardless of the fix) with a direct unit test of _catalog_shape_error. This deterministically proves both paths now reject a payload missing schema_version, a non-dict integrations, or a non-dict payload, and accept a well-formed one. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-22 08:57:39 -05:00
Nurfitra Pujo Santiko	470ac5b6e6	feat: add Factory Droid CLI integration (#822) (#3587) ... Adds a skills-based integration for the Factory Droid CLI alongside the existing Claude/Codex skills agents. The integration scaffolds `.factory/skills/speckit-*` directories and documents the install step in the devcontainer post-create script via the official npm distribution (`npm install -g droid`), which matches the layout of every other CLI install block above and avoids executing an unverified remote shell installer. The integration is also added to the user-facing supported-agent table in `docs/reference/integrations.md` so the new key is discoverable from the published documentation, as required by the "Updating this documentation" guideline in AGENTS.md. Operator-supplied extra args via `SPECKIT_INTEGRATION_DROID_EXTRA_ARGS` are appended after the canonical Spec Kit flags so the canonical flags are always present in argv. The Factory Droid CLI parser uses last-wins duplicate-flag semantics (verified empirically against droid 0.175.0), so a later operator-supplied value may override the canonical one — this is a deliberate inversion of the cursor-agent / opencode / codex ordering. Includes: - `src/specify_cli/integrations/droid/__init__.py` (subpackage) - `tests/integrations/test_integration_droid.py` (46 tests, including regression coverage for the no-trailing-newline frontmatter fusion bug, idempotent skill injection, and env-var path resolution) - `integrations/catalog.json` entry + `updated_at` bump - Alphabetical registration in `src/specify_cli/integrations/__init__.py` and `tests/integrations/test_registry.py` - Devcontainer Droid install block via the npm distribution (`npm install -g droid`), replacing the earlier curl-based installer - User-facing supported-agent table row in `docs/reference/integrations.md` (key `droid`, `.factory/skills/` layout, `/speckit-<command>` invocation) - `AGENT_CONFIG` entry and matching alphabetical entries in `tests/test_agent_config_consistency.py` (`ISSUE_TEMPLATE_AGENT_KEYS`) and the three issue-template dropdowns (`agent_request.yml`, `bug_report.yml`, `feature_request.yml`) so `test_issue_template_agent_lists_match_runtime_integrations` keeps the runtime/template surfaces synchronized Closes #822 Assisted-by: Droid (oracle-reviewer) Assisted-by: Droid (model: MiniMax M3, autonomous) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>	2026-07-22 07:49:48 -05:00
Ali jawwad	5674fd03a9	fix(init): show hyphenated /speckit-<name> in Next Steps for Forge projects (#3642) ... The post-init "Next Steps" panel renders recommended slash commands via the nested `_display_cmd()`. It special-cased dollar-skills agents, kimi, slash-skills agents, and cline, but not Forge. For a Forge project `_display_cmd` fell through to `return f"/speckit.{name}"`, printing `/speckit.constitution`, `/speckit.specify`, etc. Forge only registers the hyphenated form (`/speckit-<name>`, per `format_forge_command_name` / `ForgeIntegration.build_command_invocation`, and the generated command-file tests already assert this), so the panel told Forge users to run commands that don't exist under the dotted name. Add `forge_skill_mode` alongside `cline_skill_mode` and include it in the hyphenated-slash condition, mirroring how cline (also a non-skills markdown agent with hyphenated commands) is handled. Other agents unaffected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-22 07:46:45 -05:00
Ali jawwad	d39f8fd5e5	fix(integrations): Cline dispatches hyphenated /speckit-<cmd> invocations (#3622) ... Cline installs its slash-commands with hyphenated names (speckit-plan, speckit-git-commit) via format_cline_command_name + the hyphenated command_filename, but ClineIntegration inherited MarkdownIntegration's build_command_invocation, which builds the dotted /speckit.<cmd> — a name Cline never registered. Add a build_command_invocation override reusing format_cline_command_name, producing /speckit-<name>, mirroring the ForgeIntegration fix. Cline was the only remaining markdown integration with invoke_separator='-' + hyphenated command_filename that lacked the override. Tests assert Cline core + extension invocations are hyphenated, incl. args (fail before: dotted /speckit.plan / /speckit.git.commit). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-22 07:26:28 -05:00
Quratulain-bilal	d9e4565cf8	fix(integrations): escape Rich markup in --integration-options error messages (#3458) ... * fix(integrations): exit cleanly on malformed --integration-options quoting _parse_integration_options called shlex.split(raw_options) unguarded. an unbalanced quote (e.g. --integration-options='--commands-dir "foo') makes shlex raise ValueError('No closing quotation'), so a raw traceback escaped instead of the typer.Exit(1) error every other bad-input path in this function produces. reachable from specify init and every integration install/switch/ upgrade/migrate that accepts --integration-options. wrap the split and convert ValueError into the same clean CLI error. added a regression test; confirmed it fails on the pre-fix code (raw ValueError). * escape user-controlled values in integration-options error messages the malformed-quoting handler (and the unexpected/unknown option branches) interpolate raw_options/token into console.print. a value carrying an unbalanced rich tag like '--commands-dir "[/red]foo' first trips the intended shlex ValueError, but the error print then raises rich.errors.MarkupError and leaks a traceback anyway. escape all three before printing so the clean typer.Exit survives. added a regression covering both the shlex path and a bare markup token. * address review: drop redundant, mis-described markup test The removed test's docstring claimed the shlex failure branch interpolates raw_options into console.print; it only prints {exc}, which never contains the caller's markup. Its two assertions also duplicated test_bad_option_token_with_rich_markup_exits_cleanly (the '[/red]foo' case) and the shlex-path case already covered by test_unbalanced_quote_exits_cleanly. The real change here remains the escape() of the two user-controlled token prints.	2026-07-22 06:50:45 -05:00
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>	2026-07-21 11:40:15 -05:00
Davide Barletta	74662cffad	feat: update Bob integration to skills-based layout for Bob 2.0 (#3415) ... * feat: update Bob integration to skills-based layout for Bob 2.0 Bob 2.0 replaces the command-based workflow (.bob/commands/*.md) with a skills-based layout (.bob/skills/speckit-<name>/SKILL.md), matching the pattern used by Claude Code, Codex, and other skills-first agents. - Switch BobIntegration from MarkdownIntegration to SkillsIntegration - Update folder/dir from .bob/commands to .bob/skills - Change extension from .md to /SKILL.md (skills layout) - Add --skills option (default: True) consistent with Codex pattern - Update tests to inherit from SkillsIntegrationTests (28 tests pass) - Bump catalog entry to version 2.0.0 with updated description Assisted-by: IBM Bob (model: claude-sonnet-4-5, autonomous) * PR comments fix: keep old Bob 1 commands till next release * Copilot suggested change Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * feat(bob): address copilot comments, make skills layout default, demote legacy commands to opt-in * fix(bob): honor legacy_commands in ai_skills persistence and add bob to ALWAYS_SLASH_AGENTS - init.py: suppress ai_skills=True when --legacy-commands is passed so extensions and presets target .bob/commands, not .bob/skills - _invocation_style.py: add 'bob' to ALWAYS_SLASH_AGENTS so init next-steps and hook invocations always show /speckit-<name> (skills is the default layout; no ai_skills flag required) * Copilot suggestion Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(bob): extend IntegrationBase directly to avoid false isinstance(SkillsIntegration) - bob/__init__.py: switch BobIntegration base from SkillsIntegration to IntegrationBase; add _BobSkillsHelper for skills-mode delegation; set invoke_separator='-' explicitly; set _skills_mode flag in setup() so consumers can derive the effective mode without isinstance checks - _helpers.py: replace isinstance(integration, SkillsIntegration) guard with getattr(_skills_mode) so legacy-commands mode does not persist ai_skills=True - _invocation_style.py: remove 'bob' from ALWAYS_SLASH_AGENTS — Bob 2.0 skills are invoked via natural language, not /skill-name slash commands - integrations/catalog.json: advance updated_at to 2026-07-15 * fix(lint): remove unused SkillsIntegration import from _helpers.py * Copilot suggested change Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * feat(bob): add bob skills integration with registrar-based mode detection * address 3 comments from copilot * feat(bob): update registrar config to use legacy commands layout * fix lint * Suggested fix from Copilot Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix pr comment * fix pr comment * fix pr comment * refactor(bob): resolve skills mode via base-class hooks + fix command-ref separators Rework the dual-mode handling introduced for Bob 2.0 so an integration's internal representation never leaks into shared init/install/upgrade code, and fix the legacy command-reference separator surfaced in review. Base-class contract: - Add IntegrationBase.is_skills_mode(parsed_options) — the single hook the shared machinery consults to decide whether to persist ai_skills and render skill invocations. SkillsIntegration returns True; Copilot honors --skills / self._skills_mode; Bob returns `not legacy_commands`. - Add IntegrationBase.invoke_separator_for_mode(skills_enabled) — resolves the command-ref separator from a project's persisted mode for registration paths that only have the ai_skills flag (no CLI parsed_options). Default is behavior-preserving; Bob maps skills->"-", legacy->".". - BobIntegration stays on IntegrationBase (mirroring Copilot, the other dual-mode agent) and delegates setup() to internal _BobSkillsHelper / _BobMarkdownHelper. Removes the _skills_mode method and all isinstance(SkillsIntegration) / callable(_skills_mode) probing from _helpers.py and init.py. Fix legacy separator (review feedback): CommandRegistrar.register_commands and PresetManager._resolve_skill_command_refs previously read the single static AGENT_CONFIGS[key]["invoke_separator"], so legacy .bob/commands/ extension and preset command refs rendered /speckit-<cmd> instead of Bob 1.x /speckit.<cmd>. Both now resolve the separator per project mode via invoke_separator_for_mode. Tests: add regression coverage for the is_skills_mode / invoke_separator_for_mode hooks and legacy extension command-ref separators; normalize a width-sensitive workflow assertion to match its siblings. Full suite green. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob,copilot): address review — preserve legacy layout, dual-mode separators, extension-skill token resolution Addresses PR review 4716036212 (3 comments): 1. Bob legacy-install regression: `use`/`switch`/`upgrade` on an existing Bob 1.x project (only `.bob/commands/` on disk, no stored `legacy_commands`) called `is_skills_mode(None)` -> True and rewrote `ai_skills=True`, silently switching extension/command-reference handling to the skills layout. `is_skills_mode` now takes an optional `project_root`; Bob preserves an already-installed legacy layout until an explicit upgrade creates `.bob/skills/`. A fresh project still defaults to skills. 2. Copilot dual-mode separator: `invoke_separator_for_mode` was inherited from the base (mode-independent) and returned Copilot's static `.`, so preset/extension command refs in a Copilot skills project rendered `/speckit.<name>` instead of `/speckit-<name>`. Override it on Copilot to track the persisted `ai_skills` state, consistent with `build_command_invocation` and `effective_invoke_separator`. 3. Bob extension-skill command-ref tokens: verified that merging main's generic `_resolve_command_ref_tokens` (#3544) resolves Bob's tokens via the `CONDITIONAL_SLASH_AGENTS` path (`/speckit-<name>`); added Bob to the command-ref regression parametrize plus dedicated Bob use-path tests. All tests pass (full suite green; merged with current main incl. #3544). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): resolve command-ref separator with project-aware mode before shared-infra refresh (review #3415) The `use`/`switch` paths refresh shared infrastructure via `_with_integration_setting()` / `_invoke_separator_for_integration()`, which previously resolved the invoke separator through `effective_invoke_separator` / `is_skills_mode` WITHOUT a project_root. For a pre-PR Bob 1.x project (.bob/commands/ on disk, no stored options), this defaulted to the skills "-" separator and rewrote rendered shared-template command refs to /speckit-*, even though ai_skills stayed false. Thread project_root through effective_invoke_separator, the two runtime helpers, and every call site so Bob's on-disk legacy detection governs the separator before shared infra is refreshed. Add a rendered-shared-template regression test covering `use --force`. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): scope persisted ai_skills flag to active agent when resolving command-ref separator (review #3415) `register_commands` runs once per detected agent, but the persisted `ai_skills` flag describes only the active integration (`opts["ai"]`). When another agent (e.g. Copilot) is active in skills mode while a legacy `.bob/commands` layout is also present, the previous code passed that global `True` to Bob's `invoke_separator_for_mode`, rewriting Bob 1.x command refs to `/speckit-*` instead of `/speckit.*`. Only consult the persisted flag for the agent it describes (`opts["ai"] == agent_name`); otherwise resolve the separator from the agent's own project-aware `effective_invoke_separator(None, project_root)`. Add regression tests covering the mismatched-active-agent case and a control for Bob-active skills mode. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): detect Spec Kit layout from managed artifacts, not any skills dir (review #3415) Two related mis-detections from review 4723246468: 1. `BobIntegration.is_skills_mode` treated the mere presence of a `.bob/skills/` directory as proof the project is skills-based. A legacy Spec Kit install (managed `.bob/commands/speckit.*.md`) that also carried unrelated Bob 2 skills would be misclassified as skills, so `integration use bob` persisted `ai_skills` and rewrote shared refs. Now the layout is inferred from managed Spec Kit artifacts: legacy/command mode only when managed `speckit.*.md` command files exist and no managed `speckit-*` skill dirs do. 2. The `register_commands` separator for an inactive agent used a disk-based `effective_invoke_separator(None, project_root)` fallback that could pick the skills separator even though the registrar writes the static command layout (`.bob/commands/*.md`). Inactive agents now resolve the separator from the registrar's actual output layout (`extension == "/SKILL.md"`), so command-layout files keep `/speckit.*` refs regardless of sibling dirs. Update the affected hook/E2E tests to use managed artifacts and add regression tests for the mixed-layout and inactive-registrar scenarios. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): apply managed-artifact detection on upgrade + consistent skill post-processing (review #3415) Two issues from review 4723782860: 1. `BobIntegration.setup()` resolved the layout via `is_skills_mode(parsed_options)` WITHOUT `project_root`, so `integration upgrade bob` on a Bob 1.x install (managed `.bob/commands/speckit.*.md`, no stored options) ignored the existing command files, generated skills, and stale-deleted the legacy commands — silently migrating the project. Pass `project_root` so the same managed-artifact detection used by `use` also governs upgrades. 2. Only `_BobSkillsHelper` overrode `post_process_skill_content` to suppress the shared slash-command hook note. Preset/extension skill generators call that hook on the registered `BobIntegration`, which inherited `IntegrationBase`'s note-injecting default. Repeat the no-op (delegating to the skills helper) on the registered class so every Bob skill-generation path is consistent with intent-activated core Bob skills. Add regression tests for the upgrade-preservation and post-processing paths. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * feat(bob): add --skills migration opt-in; fix separator + manifest loss (review #3415) Address review #3415 (4724160183): - Comment 1: Add an explicit `--skills` opt-in to BobIntegration. It forces the skills layout over on-disk auto-detection, giving legacy Bob 1.x installs a supported migration path (`integration upgrade bob --integration-options="--skills"`). `--skills` and `--legacy-commands` are mutually exclusive (clean exit-1 error). - Comment 2: In CommandRegistrar.register_commands, derive the command-ref separator from the output layout (agent_config["extension"]) for the active agent too, not the persisted ai_skills flag. A command-layout file (.bob/commands/*.md, .github/agents/*.agent.md) always renders /speckit.*; only a /SKILL.md scaffold uses /speckit-*. Dual-layout agents (Bob, Copilot) write skills via their own setup()/skills path, so register_commands only ever emits their command-layout files. - Comment 3: Update docs/reference/integrations.md Bob entry to document the skills-based default (.bob/skills/), the deprecated --legacy-commands opt-out, and the --skills migration path. Also fix a latent manifest-loss bug surfaced by the migration path: the upgrade Phase 2 stale-file cleanup built a throwaway manifest sharing the integration key and called uninstall(), which always deleted {key}.manifest.json. Any layout-shrinking upgrade (e.g. legacy->skills) thus wiped the freshly-saved manifest, leaving the project untracked and un-upgradeable. uninstall() now takes remove_manifest (default True); the stale-cleanup pass passes False. Adds regression tests for the --skills opt-in, mutual exclusion, corrected active-agent separator, remove_manifest=False, and an end-to-end legacy->skills migration that verifies the manifest survives and the project remains upgradeable. Full suite: 4555 passed, 5 skipped. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * docs(agents): align token-resolution comment with output-layout separator rule (review #3415) Address review #3415 (4725516805). The comment above resolve_command_refs still described the removed state-based behavior ("resolve it from the integration using the project's persisted skills state"). Update it to describe the output-layout rule that register_commands now uses: _sep is derived from the layout this registrar writes (a /SKILL.md scaffold uses the skills separator; a command-layout file uses the command separator), not the persisted ai_skills state. Comment-only change; no behavior change. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): reconcile extension artifacts on layout change (review #3415) When a dual-mode agent (Bob) flips between the legacy commands layout and the skills layout during `integration upgrade` (via `--skills` / `--legacy-commands`), the old layout's extension command/skill files were left orphaned: Phase 2 stale cleanup only removes files tracked by the *integration* manifest, while extension artifacts are tracked in the extension registry. Detect the layout flip by comparing whether the old vs new manifest tracks a `/SKILL.md` scaffold, and when it changed, unregister the agent's extension artifacts before the existing re-registration so they are recreated in the new layout (and the per-agent registry is updated). Preset artifacts are documented as a known, pre-existing cross-cutting gap: no agent-scoped preset re-registration exists in use/switch/upgrade for any agent, so reconciling them is out of scope for this Bob migration. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): reject layout migration when preset overrides are installed (review #3415) A command↔skills layout change during `integration upgrade` cannot reconcile preset artifacts: presets track their command/skill files in per-preset `registered_commands`/`registered_skills` metadata, and there is no agent-scoped preset re-registration anywhere in the CLI. Migrating would delete a preset's old-layout files without recreating them in the new layout and leave the preset registry claiming artifacts that no longer exist. Detect the intended layout via `is_skills_mode` (so a plain same-layout upgrade is unaffected) and, when it flips while preset overrides are installed for the agent, reject the upgrade *before any mutation* with an actionable error pointing at the remove → upgrade → reinstall workaround. Extension artifacts are still reconciled for the safe (no-preset) case. Adds a regression test and documents the migration caveat in the Bob integration reference entry. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): restrict layout reconciliation to the active integration (review #3415) `integration_upgrade` supports upgrading a secondary (non-active) integration, but the layout-change extension reconciliation was unsafe there. `ExtensionManager.unregister_agent_artifacts()` treats the unscoped per-extension `registered_skills` list as belonging to the passed agent and, when that agent's skills directory is absent, falls back to scanning every agent's skills directory — so reconciling a secondary Bob layout flip could delete or untrack the *active* agent's extension skills. The subsequent re-registration cannot repair that because extension skill rendering is intentionally scoped to the active agent (#2948). Gate the unregister-before-register reconciliation on `installed_key == key` so it only runs for the active integration. Secondary agents only ever have extension command files (skills are active-agent-only), which the existing re-registration rewrites in place, so skipping the unregister orphans nothing new. Adds a regression test asserting a secondary Bob layout change leaves the active agent's extension skill intact on disk and in the registry. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): fail closed when preset registry is unreadable (review #3415) Address review 4744636079: - _migrate_commands: the preset guard previously failed *open* — a registry read/parse error returned an empty "no presets" list, so a --force layout-changing upgrade could delete preset-overridden command files while their registry state was unknown. Read the registry file directly and raise _PresetRegistryUnreadableError on any read/parse failure or malformed structure, rejecting the migration before any mutation. A genuinely absent registry still returns [] (safe). - bob: correct the is_skills_mode docstring — upgrade *does* run setup(); disk detection is needed because legacy Bob 1.x installs never persisted a legacy_commands option, so the stored mode is unavailable. - tests: add fail-closed E2E (corrupted registry rejected, valid-empty allowed) plus a unit test for _installed_presets_affecting_agent covering absent / corrupted / malformed / valid / affecting-agent cases. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf * fix(bob): fail closed on malformed preset entries too (review #3415) Address review 4745191015: the preset guard read a parseable registry but silently skipped malformed per-preset metadata and treated a malformed registered_commands value as "no matching artifacts". A registry such as {"presets":{"p1":[]}} therefore allowed a layout migration even though p1's ownership is unknown, risking deletion of preset-managed files. Now raise _PresetRegistryUnreadableError for a non-dict preset entry, a non-dict registered_commands, or a non-list registered_skills. Extend the unit test to cover these malformed shapes. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 63f93544-a77f-4f01-bf04-c88806a97dbf --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Manfred Riem <15701806+mnriem@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>	2026-07-21 09:20:20 -05:00
Andrew Chen	6d77b4a099	fix(integrations): catch OverflowError on a priority: .inf in add/remove (#3589) ... IntegrationCatalog.add_catalog and remove_catalog re-validate the existing catalog entries' priorities inline, separately from the base loader. Both did `int(raw_priority)` under `except (TypeError, ValueError)`, so a `priority: .inf` (float('inf')) raised OverflowError: add_catalog leaked a raw traceback instead of IntegrationValidationError, and remove_catalog crashed while building the display order. Add OverflowError to both handlers, matching the base loader (#3525) and the workflow/step loaders (#3526). add_catalog now raises IntegrationValidationError; remove_catalog falls back to positional order like the other non-integer priorities. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-07-21 08:28:27 -05:00
Ali jawwad	c864fc7447	fix(integrations): Forge dispatches hyphenated /speckit-<cmd> invocations (#3529) ... Forge installs its slash-commands with hyphenated names (speckit-foo-bar, via format_forge_command_name and the injected frontmatter name), but ForgeIntegration inherited MarkdownIntegration.build_command_invocation, which builds the dotted /speckit.<cmd>. So 'workflow'/command dispatch invoked /speckit.plan while the registered command is /speckit-plan — a name Forge never registered. Override build_command_invocation to reuse format_forge_command_name, producing /speckit-<name> (with '.'-to-'-' for extension commands), mirroring the skills agents' hyphenated invocation. Tests assert Forge core + extension invocations are hyphenated, incl. args (fail before: dotted /speckit.plan / /speckit.git.commit). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-17 14:12:36 -05:00
Nate Chadwick	fd101d531e	feat(integrations): add Grok Build skills-based integration (#3535) ... * feat(integrations): add Grok Build skills-based integration Add first-class support for xAI Grok Build via SkillsIntegration, installing speckit skills under .grok/skills and wiring init/invocation/catalog surfaces. Assisted-by: Grok Build (model: grok-4, supervised) * test+docs: address Copilot review on Grok multi-install and next steps Assert init next-steps guidance for Grok (.grok/skills, /speckit-*) and clarify that multi-install safety is path/manifest isolation, not agent-context defaults such as shared AGENTS.md. * fix(integrations): Grok headless --always-approve and isolation paths Document Grok multi-install isolation as .grok/skills and .grok/rules. Override build_exec_args to pass --always-approve so non-interactive dispatch is not blocked at tool permission gates. * docs(integrations): list only managed .grok/skills for Grok isolation Multi-install isolation documents Spec Kit-managed paths; Grok only writes .grok/skills, so drop the read-only .grok/rules entry. * fix(integrations): always-slash Grok hooks and refresh catalog date Move grok to ALWAYS_SLASH_AGENTS so hooks never emit /speckit.plan when ai_skills is missing/false. Update slash-format tests, persist ai_skills on init, and bump catalog updated_at for the Grok entry. --------- Co-authored-by: Nate Chadwick <1232206+natechadwick@users.noreply.github.com> Co-authored-by: test <test@example.com>	2026-07-15 14:55:25 -05:00
github-actions[bot]	f065e27478	test: cover preset constitution seeding through init CLI (#3297) ... * Fix preset-constitution-not-installed: use PresetResolver in constitution setup Apply the remediation from the bug assessment on issue #3272. Changes: 1. Modify ensure_constitution_from_template (init.py) to resolve the constitution-template through the preset priority stack via PresetResolver, instead of hardcoding the core template path. This ensures a preset's replacement constitution-template is used when seeding .specify/memory/constitution.md. 2. Reorder init flow: move ensure_constitution_from_template to after the preset installation block so that 'specify init --preset' seeds the memory file from the already-resolved template stack, not from the generic template that existed before the preset arrived. 3. Add _maybe_reseed_constitution to PresetManager (presets/__init__.py): a post-install hook that re-seeds .specify/memory/constitution.md from the preset's constitution-template during 'specify preset add' on an existing project, but only when the memory file still contains generic placeholder tokens ([PROJECT_NAME] or [PRINCIPLE_1_NAME]). Legitimately authored constitutions (no placeholder tokens) are never overwritten. 4. Add regression tests covering both code paths (TestConstitutionReseedOnPresetInstall and TestEnsureConstitutionFromTemplate in tests/test_presets.py). Refs #3272 Assisted-by: GitHub Copilot (model: claude-sonnet-4.6, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden preset constitution resolution Use manifest-aware composed content, atomic safe writes, and conservative generic-template matching for constitution seeding and re-seeding. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 49891a32-bec4-462c-a7f2-6d6ec4eefcdb * Limit preset CLI change to regression test Remove accidental whole-file Ruff formatting introduced during conflict resolution so the PR contains only the intended end-to-end test. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 49891a32-bec4-462c-a7f2-6d6ec4eefcdb * Make preset init test depend on init ordering Disable preset-install lifecycle seeding in the regression test so it fails unless init materializes the constitution after registering the preset. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 49891a32-bec4-462c-a7f2-6d6ec4eefcdb --------- 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: Ben Buttigieg <70525+BenBtg@users.noreply.github.com>	2026-07-15 17:52:14 +01:00
Manfred Riem	2fb18c73cb	fix(integration): preserve ai_skills on use for skills-mode Copilot (#3550) (#3551) ... `specify integration use copilot` against a Copilot install configured with `--integration-options "--skills"` dropped `"ai_skills": true` from init-options.json and regenerated extension commands in the legacy `.agent.md`/`.prompt.md` layout, contradicting `integration.json`'s stored `parsed_options.skills: true`. `_update_init_options_for_integration` only inspected `SkillsIntegration` / the instance `_skills_mode` flag. On the `use` path no `setup()` runs, so the freshly-resolved Copilot instance has `_skills_mode == False` and the stored skills intent in `parsed_options` was ignored. Thread the resolved `parsed_options` through and treat `parsed_options["skills"]` as skills mode. Adds a regression test that resets the registry singleton's `_skills_mode` to simulate a fresh process (in-process singleton reuse otherwise masks the bug). Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Copilot-Session: 06fb6ae9-f444-4dfd-ab3f-d0669c5d0604 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>	2026-07-15 11:18:54 -05:00
Ben Buttigieg	353851e966	fix(presets): seed constitution from preset constitution-template (#3272) (#3276) ... * fix(presets): seed constitution from preset constitution-template (#3272) The constitution is the only template materialized to a live file (.specify/memory/constitution.md) rather than resolved on demand, yet ensure_constitution_from_template hardcoded a copy from the core template and ignored PresetResolver. Combined with init seeding the constitution before preset installation, a preset's constitution-template (e.g. strategy: replace with a ratified constitution) could never go live. Changes: - ensure_constitution_from_template now resolves constitution-template through PresetResolver, so a preset/override/extension wins and core is the fallback. - init seeds the constitution after preset installation so init --preset uses the resolved stack. - install_from_directory re-seeds memory/constitution.md from the resolved preset template, guarded to only act when the memory file is missing or still contains generic placeholder tokens — authored constitutions are never overwritten. Covers preset add and install_from_zip. - Tests for preset seeding, placeholder re-seed, authored-constitution preservation, override resolution, and resolver-aware init seeding. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(presets): compose constitution-template when seeding memory Take on review feedback from Copilot and gglachant: - constitution seeding previously copied the top layer file path verbatim even when the winning layer used a composing strategy (prepend/append/wrap), which could leave {CORE_TEMPLATE} unresolved. - both seeding paths now inspect resolver layers and only copy verbatim for replace; non-replace strategies materialize composed content via PresetResolver.resolve_content(). - add regression tests for wrap strategy composition in both PresetManager seeding and ensure_constitution_from_template. - add a drift-guard test pinning _CONSTITUTION_PLACEHOLDER_TOKENS to the placeholders in templates/constitution-template.md. Assisted-by: GitHub Copilot (model: GPT-5.3-Codex, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * refactor(presets): unify constitution template materialization Address latest Copilot feedback on the constitution seeding path: - moved resolver/layer I/O behind the existing-memory fast path in init - corrected tracker output for composed materialization - deduplicated materialization logic shared by init and preset install seeding into presets._materialize_constitution_template() Behavior is unchanged for replace strategies (copy verbatim) and remains composed for prepend/append/wrap via resolve_content(). Assisted-by: GitHub Copilot (model: GPT-5.3-Codex, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(init): restore shutil import The constitution materialization refactor removed the module import, but init still uses shutil.rmtree when cleaning up a failed new-project initialization. Restore the import so the required ruff check passes. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix(presets): harden constitution materialization Address the outstanding review batch for preset constitution seeding: - use checked atomic writes and reject symlinked memory paths - replace placeholder heuristics with hash/source provenance - rematerialize unchanged generated constitutions by resolver priority - preserve authored or edited constitutions, including placeholder mentions - warn non-fatally when post-install materialization cannot complete - retain exact core-template comparison for legacy projects without provenance Add focused provenance, priority, symlink, and failure-path coverage, and update integration inventories for the generated provenance sidecar. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): reconcile constitution after removal When the removed preset supplied constitution-template, rematerialize the winning remaining resolver layer only if provenance proves the live file is still generated and unchanged. Preserve edited constitutions and report post-removal reconciliation failures as non-fatal warnings. Add coverage for restoring the core layer, falling back from a removed higher-priority preset, and preserving edited generated content. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): tighten legacy constitution provenance Trust only the immutable bundled/source constitution template when migrating legacy projects without provenance. Do not infer core provenance from mutable project templates or preset source labels, including IDs beginning with core. Also detect convention-based constitution-template files before preset removal so unchanged generated constitutions reconcile to the next resolver layer. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): preserve files with invalid provenance Use immutable-core legacy migration only when the provenance sidecar is absent. If a sidecar is malformed or its hash does not match the live constitution, treat the file as edited and preserve it. Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): reconcile constitution on stack changes Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 * fix(presets): guard constitution reconciliation edges Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b2c095d-b45c-4d52-8d56-bd6121d96ab6 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>	2026-07-15 11:42:50 +01:00
Marsel Safin	91839fba50	feat(extensions): port git extension scripts to Python (#3400) ... * feat(extensions): port git extension scripts to Python Ports git-common, initialize-repo, auto-commit, and create-new-feature-branch to extensions/git/scripts/python/, mirroring the bash/PowerShell twins. Parity tests run each bash script and its Python twin in identical projects and compare output, exit codes, and resulting git state. Fixes #3282 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: match bash error message for whitespace-only descriptions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Handle unreadable git-config.yml and assert stderr parity An unreadable config file raised OSError with a full traceback from _parse_auto_commit_config. Treat it like a missing config: auto-commit stays disabled. Covered by a chmod-000 test (skipped on non-POSIX and as root). _assert_parity now also compares stderr so warning or usage-text regressions between the bash and Python twins fail the suite. All existing parity tests pass with the stricter assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(extensions/git): pass script path to core.get_repo_root for cwd-outside-repo callers Without script_file, core.get_repo_root() falls back to Path.cwd() when SPECIFY_INIT_DIR is unset and no .specify root is found upward — the bash twin instead falls back to the script's install location (.specify/scripts/...). Pass script_file so both twins resolve the same repo_root; TypeError fallback keeps older cores working. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: exercise SPECIFY_INIT_DIR from outside the project Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(extensions/git): handle UnicodeDecodeError and USER/USERNAME fallback - Catch (OSError, UnicodeDecodeError) when reading git-config.yml in create_new_feature_branch.py, initialize_repo.py, and auto_commit.py so invalid UTF-8 config falls back to defaults instead of crashing with a traceback. - Fall back to USERNAME (then "unknown") when USER is unset when deriving the branch author token, matching the PowerShell twin's Windows-friendly fallback chain. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(extensions/git): platform-aware persist hint and stronger SPECIFY_INIT_DIR test - Add a shared _persist_hint() helper in create_new_feature_branch.py and use it for both the JSON-mode stderr hint and the human-readable stdout hint, so there is a single place emitting the SPECIFY_FEATURE persistence guidance. On Windows (os.name == "nt") it prints PowerShell $env:VAR = "..." syntax; elsewhere it keeps the existing POSIX export VAR=... syntax (parity with the bash twin). - Rework test_specify_init_dir_resolves_target_project so SPECIFY_INIT_DIR is the only thing that can produce the observed result: the script now runs from a separate host_proj (no existing specs, so script/cwd-based discovery would yield 001) while SPECIFY_INIT_DIR points at a different target_proj that already has an existing spec (007-existing, so the override must yield 008). The old version pointed SPECIFY_INIT_DIR at the same project the script was installed in, so it passed even if the env var were ignored. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(extensions): tolerate missing Git executable Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(extensions): quote PowerShell persist hint Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(git): match bash persist hint escaping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(git): ignore unterminated config record Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(git): handle Windows persist hint parity Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(init): install Python shared scripts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(git): normalize Windows persistence hints Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-14 09:56:10 -05:00
Marsel Safin	6ab0c1dac1	fix(integrations): escape control characters in goose recipe YAML renderer (#3384) ... * fix(integrations): escape control characters in goose recipe YAML renderer YAML forbids C0 control characters (except tab and newline) and DEL in every scalar form, and a bare CR acts as a line break inside a block scalar. _render_yaml wrote the body verbatim into a |2 literal block scalar, so such bodies produced recipes the YAML parser rejects. Detect block-scalar-unsafe characters and fall back to an escaped double-quoted scalar via yaml.safe_dump, mirroring the TOML renderer's fallback strategy from #3341. Fixes #3382 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): use sys.maxsize instead of float inf for yaml width Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): extend block-scalar guard to C1 controls and Unicode line breaks YAML's printable set excludes C1 controls (U+0080-U+009F except NEL), and YAML 1.1 treats NEL/LS/PS as line breaks inside a literal block scalar, so bodies carrying any of these still produced unparseable recipes. Widen the fallback guard to the full class and cover it in the regression loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): also treat surrogates and U+FFFE/U+FFFF as block-scalar unsafe YAML's printable set also excludes lone UTF-16 surrogates and the non-characters U+FFFE/U+FFFF; bodies carrying them still hit the literal block path and produced unparseable recipes. Extend the guard and the regression loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(integrations): clarify YAML prompt serialization Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-14 07:15:04 -05:00
Ali jawwad	993083405e	fix(init): don't block on confirmation for 'init --here' without a TTY (#3236) ... * fix(init): don't block on confirmation for 'init --here' without a TTY When 'specify init --here' targets a non-empty directory without --force, it called typer.confirm() unconditionally. In a non-interactive session (no TTY -- CI, piped, agent) there is no input, so the prompt reads EOF and aborts unhelpfully (or blocks), with no actionable message. The named-project path already fails fast and points to --force; --here was the inconsistent outlier. Guard the confirmation with the existing _stdin_is_interactive() helper: when non-interactive, print a clear 'directory not empty; re-run with --force' error and exit 1 instead of prompting. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): honor piped confirmation for 'init --here'; only fail-fast on empty stdin The first version of this fix short-circuited on '_stdin_is_interactive()' (isatty) before typer.confirm, which broke 'init --here' when confirmation is piped (e.g. 'echo y | specify init --here' / CliRunner input='y\n') -- a non-TTY pipe with valid input was wrongly rejected, regressing test_init_here_without_force_preserves_shared_infra. Instead, call typer.confirm normally (piped 'y'/'n' is honored) and catch the Abort/EOFError it raises only when stdin is empty, converting that to the actionable '--force' guidance. This keeps the UX win for the no-input case without rejecting piped input. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): distinguish interactive cancel from no-input; defer merge warning Address Copilot review on the --here non-empty path: (1) treat typer.Abort during an interactive confirm (e.g. Ctrl+C) as a normal cancellation (exit 0), and only emit the '--force' guidance + exit 1 when there is no TTY (empty stdin / EOF) -- no longer conflating the two; (2) move the 'will be merged / may overwrite' warning so it only shows when actually proceeding (force) or folded into the confirmation prompt, not on the fail-fast path where nothing is merged. Piped confirmation (e.g. 'echo y | specify init --here') is still honored, which is why the prompt is attempted rather than refused outright when non-interactive -- the existing test_init_here_without_force_preserves_shared_infra pipes 'y' and must succeed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): fail fast on non-interactive --here instead of prompting Per Copilot review: do not call typer.confirm when stdin is not a TTY -- an open-but-idle non-TTY stdin (CI/agent) could block on the prompt. When the directory is non-empty and --force is not given, fail fast with '--force' guidance unless an interactive terminal is present. Interactive confirm still offers the merge-but-preserve path (distinct from --force, which overwrites); a Ctrl+C there is treated as a normal cancellation (exit 0). The merge/overwrite warning is only printed when actually proceeding, not on the fail-fast path. Updated the preserve-merge E2E test to simulate an interactive terminal so it exercises the confirm path (non-interactive sessions now require --force). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(init): honor piped y/n for 'init --here', error only on no-input Per maintainer review: restore the second-revision shape. Calling typer.confirm normally keeps 'echo y | specify init --here' reaching the non-destructive preserve-merge path (and piped 'n' cancels with exit 0). Only when no confirmation input is available at all (closed/empty stdin -> typer.Abort/EOFError) is it converted into the actionable error that points at --force. This drops the _stdin_is_interactive fail-fast that broke the common piped-confirm idiom and made preserve-merge interactive-only. The preserve test no longer needs to monkeypatch _stdin_is_interactive - it passes on the real contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(init): preserve interactive-cancel semantics; fold merge risk into the prompt Two review-driven refinements to the 'init --here' non-empty confirm, keeping the maintainer-endorsed control flow (piped y/n honored; non-interactive EOF → actionable --force error): 1. typer.confirm raises typer.Abort for BOTH an interactive Ctrl+C and an EOF on closed/empty stdin. Catching it unconditionally reported 'no confirmation input available, use --force' and exited 1 even when the user cancelled at a real TTY. Branch on _stdin_is_interactive(): a TTY cancel is a normal exit 0 ('Operation cancelled'); only non-interactive EOF becomes the --force error. 2. Fold the merge-risk warning into the confirmation question instead of printing it unconditionally beforehand, so the EOF/no-input path (which exits without changing anything) no longer prints a misleading 'will be merged' line first. Adds test_init_here_interactive_cancel_exits_zero (fails before: exit 1 with --force; passes after: exit 0, 'cancelled', pre-existing file untouched). The non-interactive EOF and piped-y preserve-merge tests are unchanged and still 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>	2026-07-13 15:29:15 -05:00
Noor ul ain	c05a626cbc	fix(integrations): exit cleanly on unbalanced quote in --integration-options (#3457) (#3466) ... * fix(integrations): exit cleanly on unbalanced quote in --integration-options (#3457) `_parse_integration_options` called `shlex.split(raw_options)` unguarded, so an unbalanced quote in the flag value (e.g. `--integration-options='--commands-dir "foo'`) made shlex raise `ValueError: No closing quotation` and a raw traceback escaped — unlike every other bad-input path in this function (unknown option, missing value, unexpected value), which print a message and exit 1. Reachable from `specify init --integration-options=...` and every `specify integration install/switch/upgrade/migrate --integration-options=...`. Wrap the split in a try/except ValueError that prints a one-line error and raises `typer.Exit(1)`, matching the existing loud-fail UX. Add a test asserting the unbalanced-quote input raises `typer.Exit` with exit code 1. 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> --------- 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>	2026-07-13 14:33:02 -05:00
Noor ul ain	0acb5c6461	fix(integrations): declare kiro-cli multi-install safe (#3471) (#3485) ... kiro-cli confines all of its managed files to an isolated agent root (`.kiro/`, with commands in `.kiro/prompts`) that no other integration writes to, so it meets every documented criterion for multi-install safety — but `KiroCliIntegration` never set `multi_install_safe = True`. As a result, co-installing kiro-cli alongside any other integration left `specify integration status` permanently in ERROR: error unsafe-multi-install: Installed integrations are not all declared multi-install safe: kiro-cli `--force` bypasses the install-time gate but does not clear the status error, and there is no flag or config to acknowledge it, so the error is permanent while both integrations remain installed. Set `multi_install_safe = True`. The registry's parametrized multi-install-safe contract tests (static isolated root, distinct agent roots / command dirs, disjoint manifests) now cover kiro-cli automatically, and a focused regression test pins the declaration so a future edit cannot silently drop it and reintroduce the error. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-13 14:13:44 -05:00
NgoQuocViet2001	6664cf813c	fix: mark Kiro integration as multi-install safe (#3472) ... Assisted-by: Codex (model: GPT-5, autonomous)	2026-07-13 10:57:26 -05:00
Quratulain-bilal	14fa3ada08	fix(catalogs): raise catalog error, not raw ValueError, on a malformed URL (#3435) ... CatalogStackBase._validate_catalog_url read parsed.hostname, which raises ValueError on a malformed authority (e.g. an unclosed ipv6 bracket "https://[::1"). every other reject path raises the class catalog error, so the raw ValueError leaked to the caller. wrap the parse + hostname access and convert ValueError to the normal error via cls._error. twin of the bundler fix in adapters._validate_remote_url.	2026-07-10 13:39:01 -05:00
Vincent Lee	c8ce488073	chore: add pre-commit config and fix trailing whitespace/end-of-file (#3430) ... issues	2026-07-10 11:47:36 -05:00
Marsel Safin	15ac745e8d	fix(integrations): skip Windows Store python3 alias stub in resolve_python_interpreter (#3385) ... * fix(integrations): skip Windows Store python3 alias stub in resolve_python_interpreter On stock Windows, python3 on PATH is the Microsoft Store App Execution Alias stub: it exists but only prints an installer hint and exits non-zero, so generated {SCRIPT} invocations for the py script type were broken. Verify the found interpreter actually runs before accepting it, on Windows only, mirroring the parse-success-not-availability approach of #3312/#3320 for the sh scripts. POSIX keeps the plain existence check. sys.executable remains the fallback and is always live. Fixes #3383 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(integrations): probe interpreter isolated and without site, discard I/O Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: pin POSIX platform in PATH-resolution tests The tests fake shutil.which with POSIX paths; on Windows CI the real sys.platform made the stub probe run against those fake paths and fall through to sys.executable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-09 08:14:29 -05:00
Marsel Safin	8e2e2d2f25	fix(integrations): escape control characters in SKILL.md frontmatter (#3399) ... yaml.safe_dump with default_style='"' replaces the hand-rolled quote helpers in base.py and hermes, so newlines and control characters in template descriptions round-trip instead of producing unparseable YAML. Fixes #3391 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-09 08:13:27 -05:00
Ali jawwad	892dd656f2	fix(shared-infra): refresh_shared_templates preserves recovered user files (#3378) ... * fix(shared-infra): refresh_shared_templates preserves recovered user files refresh_shared_templates skipped a shared template only when it was untracked or modified, ignoring the manifest's is_recovered marker that install_shared_infra already honors. So a pre-existing user template (adopted via record_existing(recovered=True), hence tracked and hash-unmodified) was silently overwritten with bundled content on refresh — the exact data-loss class that #2918 fixed for install_shared_infra. Add the is_recovered check to the skip predicate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(shared-infra): include recovered files in refresh skip warning The skip predicate now also skips recovered (pre-existing user) files, so the warning saying only 'modified or untracked' could mislead a user into thinking they edited a file that was simply recorded as recovered. Reword to 'modified, untracked, or preserved (recovered)'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-07-09 07:49:29 -05:00
Ali jawwad	54ed736479	fix(agents): resolve skill placeholders in Goose (yaml) command output (#3374) ... * fix(agents): resolve skill placeholders in Goose (yaml) command output CommandRegistrar.register_commands resolves {SCRIPT}/__AGENT__ and the $ARGUMENTS placeholder in the markdown and toml branches, but the yaml branch (Goose recipes) called render_yaml_command directly, skipping both. So extension/preset command bodies installed for Goose kept literal {SCRIPT}, __AGENT__, and repo-relative script paths in the generated .goose/recipes/*.yaml prompt. Mirror the markdown/toml branches: run resolve_skill_placeholders + _convert_argument_placeholder on the body before render_yaml_command. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(goose): assert positive placeholder replacements in recipe prompt Per review: parse the generated recipe with yaml.safe_load and assert the prompt contains the resolved values (.specify/scripts/, 'agent goose', {{args}}), not merely that the literal tokens are absent — a wrong output that happens to omit the exact strings would otherwise pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-07-09 07:48:38 -05:00
Pascal THUET	643f73a1d7	test: isolate integration test home (#3144) ... * test: isolate integration test home Assisted-by: Codex (model: GPT-5, autonomous) * test: assert integration home isolation Assisted-by: Codex (model: GPT-5, autonomous) * test: extend integration home isolation to module-scoped setup Address Copilot review on #3144. Add a session-scoped autouse fixture so HOME/USERPROFILE/XDG are redirected for setup that runs outside a test function (e.g. the module-scoped status_* fixtures in test_integration_subcommand.py that run `specify init` before any per-test isolation applies). The function-scoped fixture still overrides HOME per test. Also assert Path.home() resolves to the isolated home, since most integrations (Hermes, catalog) read the home via that API rather than the env vars directly.	2026-07-09 07:35:38 -05:00
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.	2026-07-08 07:42:34 -05:00
Ali jawwad	10d4bca64c	fix(integrations): guard _sha256 against unreadable managed files (#3376) ... manifest.py::_sha256 does an unguarded open(). check_modified() and uninstall() both call it on a readable-but-unopenable regular file (e.g. permission denied) without catching OSError, so 'specify integration upgrade/uninstall/switch' surface a raw PermissionError traceback. Guard both call sites: in check_modified() treat an unreadable file as modified (consistent with the adjacent symlink / non-regular-file handling); in uninstall() treat it as skipped and preserve it (mirroring the existing path.unlink() OSError guard just below). The force short-circuit is unchanged. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-07-07 15:11:18 -05:00