6 Commits

Author	SHA1	Message	Date
Pascal THUET	4028c50af8	fix: render script command hints with active agent separator (#2649) ... * fix script command hints for agent separators * Address command hint review feedback * chore: remove whitespace-only PR churn * test: fix PowerShell command hint invocation * fix: preserve hyphens in script command hints * fix: render managed script command hints	2026-06-03 16:24:13 -05:00
Eldar Shlomi	39921ddd3b	fix(shared-infra): record skipped files in speckit.manifest.json (#2483) ... * fix(shared-infra): record skipped files in speckit.manifest.json `install_shared_infra` skipped files that already existed on disk when `force=False`, but the skip branches in both the scripts loop and the templates loop only appended to `skipped_files` without calling `manifest.record_existing`. So when the function ran with a fresh manifest against an already-populated `.specify/` tree (e.g. after the manifest was deleted, corrupted, or extracted out of band), every file went down the skip path, `planned_copies` / `planned_templates` stayed empty, and `manifest.save()` wrote an empty `files` field — leaving the integration believing nothing was installed. Record every skipped file in the manifest, but only when it is not already tracked. This preserves the original hash for files that were previously recorded so `check_modified()` (used by `integration use` to decide whether a user has customized a template) keeps working correctly. Add `TestSpeckitManifestRecordsSkippedFiles` in `tests/integrations/test_integration_claude.py` covering both the fresh-skip path and the recover-after-lost-manifest path. Fixes #2107 * fix(shared-infra): guard manifest.record_existing against non-file dst Address Copilot review feedback on PR #2483. The previous fix called ``manifest.record_existing(rel_skip)`` from the skip branch of both loops in ``install_shared_infra``, which would crash with ``IsADirectoryError`` (or another ``OSError``) if a directory or other non-regular-file happened to exist at the expected destination path — since ``record_existing`` opens the file to compute its SHA-256. Three coordinated fixes: 1. ``IntegrationManifest.record_existing`` now validates its precondition: it raises ``ValueError`` if the path is a symlink or is not a regular file. The docstring already promised "an already-existing file"; this enforces it. The symlink check runs on the un-resolved path because ``_validate_rel_path`` calls ``resolve()``, which would silently follow the symlink. Mirrors the existing ``_ensure_safe_manifest_destination`` precedent in the same module. 2. In ``install_shared_infra``'s scripts and templates skip branches, guard the ``record_existing`` call with ``dst.is_file()`` and wrap it in ``try/except (OSError, ValueError)``. A directory collision, permission error, or TOCTOU race no longer aborts the whole install — the user gets a per-path warning, the path still surfaces in ``skipped_files``, and the rest of the install continues. 3. ``_read_manifest_files`` in the regression test no longer falls back to ``data.get("_files")`` (Copilot's low-confidence finding): the silent fallback could mask a schema regression where the public ``files`` key is renamed. It now asserts ``"files" in data`` and that the value is a dict. Add two regression tests in ``TestSpeckitManifestRecordsSkippedFiles`` covering the directory-at-destination edge case for both the scripts loop and the templates loop. Both verify (a) install does not crash, (b) the non-file path is not recorded in the manifest, and (c) the path still surfaces in the user-visible warning. The "shared infrastructure file(s)" warning text is changed to "path(s)" so it remains accurate when non-file entries appear in the list. Refs #2107 * fix(manifest): lexical pre-check for record_existing + add error-case tests Address Copilot review (2026-05-11, review id 4266902103): 1. `record_existing` was calling `(self.project_root / rel).is_symlink()` BEFORE validating containment. For absolute paths or paths containing `..`, this performed a filesystem stat outside the project root before `_validate_rel_path()` raised. Add a cheap lexical pre-check that delegates to `_validate_rel_path()` for the canonical error messages, so the symlink stat only ever runs on paths that are already lexically inside the project root. 2. Add focused unit tests in `tests/integrations/test_manifest.py` for the symlink and non-regular-file error paths, including: - symlink target rejection - dangling symlink rejection (caught by the symlink guard before the is_file check) - directory path rejection (is_file == False) - missing-path rejection (is_file == False) - absolute-path lexical pre-check The Copilot reviewer noted these guards had no focused coverage in `test_manifest.py`, only via the `test_integration_claude.py` regression test. 3. The third Copilot finding (repeated `dict(self._files)` copies via `manifest.files` in the skip branches) is already resolved on this branch by using `prior_hashes` — the function-scope snapshot taken at the top of `install_shared_infra` — for the membership check, instead of `manifest.files`. AI disclosure: drafted with assistance from Claude (Opus 4.7). * fix(manifest): track recovered files separately + symlink-ancestor + canonical-path guards Address Copilot review id 4309888722 (2026-05-18) on PR #2483: 1. Recovery semantics (shared_infra.py:371, 412) — install_shared_infra now passes ``recovered=True`` when re-recording a skipped existing file. This flag funnels into a new ``recovered_files`` array in the manifest JSON, so a future ``refresh_managed`` run can distinguish "hash I produced" from "hash I observed on a file that may be a user customization" and avoid silent overwrite without ``--refresh-shared-infra``. Schema is purely additive: ``files: dict[str, str]`` is unchanged; the new ``recovered_files: list[str]`` is omitted when empty. 2. Symlinked ancestor (manifest.py:172) — ``record_existing`` now walks every component of the rel path and rejects any symlinked ancestor, not just a symlinked leaf. Catches ``linked_dir/file.txt`` where ``linked_dir`` is a symlink, which previously slipped past the leaf-only ``is_symlink()`` check and was resolved through by ``_validate_rel_path``. Mirrors the component-walk pattern in ``_ensure_safe_manifest_directory``. 3. Misleading "escapes project root" message (manifest.py:168) — paths like ``dir/../file.txt`` normalize inside the project, so the old message lied about what was wrong. New message: "Manifest paths must be canonical; '..' segments are not allowed". Still rejects (canonical keys are required so ``check_modified``/``uninstall`` cannot key the same file under two paths). Tests: 7 new test methods across TestManifestRecoveredFiles and TestRecordExistingNewGuards covering all 4 Copilot findings. Full suite passes locally. 🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7). * fix(manifest): normalize is_recovered input through _validate_rel_path Address Copilot review comment id 4309888722 round-5 (2026-05-21) on PR #2483: ``is_recovered()`` previously checked ``self._recovered_files`` membership with bare ``Path(rel).as_posix()``, while ``record_existing()`` stores keys via ``_validate_rel_path(rel, root).relative_to(root).as_posix()``. The two normalizations disagreed on absolute paths and paths that escape the project root — ``is_recovered`` would silently return False for inputs that ``record_existing`` would have refused entirely. The fix routes ``is_recovered`` through the same ``_validate_rel_path`` pipeline; ``ValueError`` from the validator is caught and converted to False so query semantics stay exception-free (Python ``__contains__`` convention). Tests: 2 new methods in ``TestManifestRecoveredFiles``: - ``test_is_recovered_absolute_path_returns_false`` - ``test_is_recovered_escaping_path_returns_false`` 🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7). * fix(manifest): clear recovered marker on managed re-record + reject '..' in is_recovered Address Copilot Round-7 review comments on PR #2483: 1. record_existing(recovered=False) and record_file now BOTH discard the path from _recovered_files. The marker is meant to flag "we observed this file but cannot vouch it's a managed baseline" — once the same path is re-recorded as managed (either explicitly or by writing fresh bytes), the marker is stale and must clear so refresh_managed and future is_recovered queries return the truthful answer. 2. is_recovered now applies the same canonical-key guard as record_existing (rejects absolute paths and '..' segments lexically before delegating to _validate_rel_path). Such paths can never be stored keys, so the query correctly returns False without depending on _validate_rel_path semantics that diverged from record_existing's stricter contract. record_file docstring updated to mention the side-effect on recovered markers. Tests: 3 new methods in TestManifestRecoveredFiles covering record_existing(false) clearing, record_file clearing, and is_recovered dotdot rejection. * test(manifest): update is_recovered comments to reflect Round-7 lexical guard Round 8 — addresses Copilot review comment on tests/integrations/test_manifest.py:362. After Round-7 (1dbf0c2), is_recovered() rejects absolute paths and '..' segments up front via a lexical guard, returning False without calling _validate_rel_path at all. The test comments still described the prior "_validate_rel_path raises; we catch" code path, which is misleading for readers. Updated comments in both: - test_is_recovered_absolute_path_returns_false (Copilot's exact target) - test_is_recovered_escaping_path_returns_false (same comment-class issue; fixed preemptively to avoid a Round-9 finding on the same drift) Pure documentation change. Test assertions and behavior unchanged; all manifest tests still green. * fix(manifest): document OS errors on record_existing + filter orphan recovered_files on load Round 9 — addresses Copilot review on PR #2483: 1. record_existing's docstring now documents OSError/PermissionError as possible raises (in addition to ValueError) — the implementation has always been able to raise them from is_symlink, is_file, or the file-read used to hash, but the contract did not reflect that. Callers should be prepared for both surfaces. 2. load() now filters recovered_files entries that don't correspond to keys in files. An externally-edited or partially-corrupted manifest can deserialize with orphan recovered paths; rather than reject the whole manifest (too strict on the upgrade path), we drop the orphans and let the inconsistency self-correct on the next save(). is_recovered then returns the truthful False for the orphan. Tests: new test_load_filters_recovered_files_not_in_files asserting an orphan recovered entry is dropped on load.	2026-06-02 08:06:31 -05:00
WOLIKIMCHENG	57a518a583	Fix shared script command hints for integration separators (#2627) ... * fix shared script command refs for integration separators * Fix integration use shared infra refresh hint * Clarify shared infrastructure force wording --------- Co-authored-by: root <1647273252@qq.com> Co-authored-by: root <kinsonnee@gmail.com>	2026-05-28 10:02:27 -05:00
Manfred Riem	e54653efcc	fix: create skills directory on demand during extension/preset install (#2711) ... * fix: create skills directory on demand during extension/preset install _get_skills_dir() in both extensions.py and presets.py returned None when the skills directory did not yet exist on disk, even though skills were enabled in init-options. This caused extension skill registration to silently produce an empty registered_skills list and skip writing SKILL.md files. Replace the is_dir() bail-out with mkdir(parents=True, exist_ok=True) so the directory is created on demand when ai_skills is enabled. Update the existing test expectation and add a parametrized regression test (claude + codex) that installs an extension before the skills directory exists and asserts SKILL.md files and registry entries are created. Fixes #2682 * test: assert skills dir is NOT created when skills are disabled Strengthen negative tests to verify _get_skills_dir does not create the directory on disk when ai_skills is false or init-options.json is absent. * fix: add symlink/containment check and preserve Kimi existence gate Address PR review feedback: - Use _ensure_safe_shared_directory() instead of raw mkdir() to prevent symlink-following writes outside the project root. - Restore the is_dir() existence gate for the Kimi native-skills fallback (ai_skills=false): only create the directory on demand when ai_skills is explicitly enabled. - Update docstrings to reflect the on-demand vs existence-gate behavior. - Reuse resolve_skills_dir helper in tests instead of manually reconstructing paths from AGENT_CONFIG. * refactor: extract resolve_active_skills_dir shared helper Deduplicate the _get_skills_dir logic that was nearly identical in ExtensionManager and PresetManager into a single module-level resolve_active_skills_dir() function in __init__.py. The shared helper wraps _ensure_safe_shared_directory errors with skills-specific messages so users see 'agent skills directory' instead of 'shared infrastructure directory' in error output. Both class methods now delegate to the shared helper. * fix: preserve original error reason in skills dir safety check Include the original exception message from _ensure_safe_shared_directory in the re-raised ValueError so the user sees the specific reason (symlink, not-a-directory, path escape, etc.) instead of a generic message. * fix: handle skills dir safety errors gracefully during install Catch ValueError/OSError from _get_skills_dir() inside _register_extension_skills() so a symlink or permission error logs a warning and returns [] instead of aborting mid-install and leaving a partially-installed extension without a registry entry. Also document OSError in resolve_active_skills_dir() docstring. * fix: catch errors in _get_skills_dir and use _print_cli_warning Move the ValueError/OSError catch from _register_extension_skills into _get_skills_dir itself so all callers (install, uninstall, reconcile) are protected from unsafe-path exceptions. Replace logging.getLogger().warning with _print_cli_warning for consistent Rich-formatted user output. * fix: use context-aware error messages for skills directory safety Add a 'context' parameter to _ensure_safe_shared_directory (defaults to 'shared infrastructure directory' for backward compat). The skills dir caller passes context='agent skills directory' so error messages say e.g. 'Refusing to use symlinked agent skills directory' instead of 'Refusing to use symlinked shared infrastructure directory'. Simplify resolve_active_skills_dir by removing the now-unnecessary try/except wrapper. * fix: validate Kimi native-skills directory for symlink/containment The Kimi fallback path (ai_skills=false) used is_dir() which follows symlinks, so a symlinked .kimi/skills could cause writes outside the project root. Now validates with _ensure_safe_shared_directory(create= False) before returning the directory.	2026-05-26 15:10:59 -05:00
Quratulain-bilal	947b4398c7	fix(integration): refresh shared infra on integration switch (#2375) ... * fix(integration): refresh shared infra on integration switch * fix(integration): address Copilot review on switch shared-infra refresh - Clarify install_shared_infra docstring: force overwrites regular files but always preserves symlinks (safe-destination check refuses to follow). - Print refresh_hint only for preserved_user_files; skipped_files keeps the generic remediation. Avoids misleading guidance when files were merely skipped (not detected as customized). - Catch ValueError from the safe-destination check and bucket the path under a new symlinked_files warning instead of aborting the switch. - Restore templates/constitution-template.md to upstream (drop accidental leading blank lines). * fix(integration): narrow symlink bucketing to dedicated exception Address Copilot feedback on shared_infra.py:305 — _safe_dest_or_bucket caught any ValueError as 'symlinked', which masked genuine safety errors (path escape, parent-not-a-directory). - Introduce SymlinkedSharedPathError(ValueError) raised only by the symlink-specific branches in _ensure_safe_shared_*(). - _safe_dest_or_bucket() now catches only SymlinkedSharedPathError; other ValueErrors propagate so the operation aborts with the real cause instead of being silently bucketed. - Wrap top-level dest_scripts/dest_variant/dest_templates mkdir calls in the same bucket helper so a symlinked .specify/scripts or .specify/templates is preserved with a warning rather than aborting the switch (matches the documented 'preserve customizations' behavior). - Update tests to expect the new bucket+warn behavior for leaf-level symlinked destinations. * fix(integration): tailor shared-infra warnings and rename preflight test Address Copilot review on PR #2375: - skipped_files hint now uses refresh_hint when refresh_managed=True so integration switch suggests --refresh-shared-infra instead of the generic init/upgrade flags. - symlinked-files warning header says "path(s)" rather than "file(s)" since symlinked directories (e.g. .specify/scripts/bash) are also bucketed there. - Rename test_shared_infra_install_preflights_before_writing to test_shared_infra_install_buckets_unsafe_destinations_and_continues to match the new bucket-and-continue semantics. * test: rename symlink bucketing tests to reflect bucket-and-continue behavior The two file-bucketing tests at line 300/320 were named *_refuses_*, but the new behavior buckets symlinked file destinations with a warning while safe destinations in the same install still complete. Rename to *_buckets_* and update docstrings to match. The remaining *_refuses_* tests (line 342/362/381) genuinely raise on symlinked dirs/manifests and keep their names. --------- Co-authored-by: Quratulain-bilal <quratulain.bilal@users.noreply.github.com>	2026-05-11 10:49:48 -05:00
Pascal THUET	38fd1f6cc2	Support controlled multi-install for safe AI agent integrations (#2389) ... * support controlled multi-install integrations * fix: harden multi-install integration state * refactor: isolate integration runtime helpers * fix: address copilot review feedback * fix: address follow-up copilot feedback * fix: tighten integration switch semantics * fix: address final copilot review feedback * fix: harden integration manifest read errors * fix: refuse symlinked shared infra paths * test: filter expected self-test preset warning * test: address copilot review nits * refactor: centralize safe shared infra writes * fix: use no-follow writes for shared infra * fix: keep default integration atomic on template refresh * fix: harden shared infra error paths * fix: preflight shared infra and future state schemas * fix: support nested shared scripts during preflight * test: tolerate wrapped schema error output * fix: use safe default mode for shared text writes * fix: use posix paths in shared skip output * fix: share project guard for integration use * fix: centralize spec-kit project guards * fix: use posix project paths in cli output * fix: harden shared manifest and upgrade refresh	2026-05-01 11:54:41 -05:00