mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
09b99f351a6bb86f23a582c06864e350cd1533b2
13 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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 |
||
|
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> |
||
|
Marsel Safin
|
55da30c66d |
feat(templates): add py: lines to command templates' scripts frontmatter (#3403)
* feat(templates): add py: lines to command templates' scripts frontmatter Every templates/commands/*.md with a scripts: block now declares a py: variant so --script py renders a Python invocation via the existing interpreter-prefixing in process_template. Fixes #3283 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: install scripts/python for the py script type install_shared_infra mapped every non-sh script type to powershell, so --script py rendered invocations pointing at files that were never installed. Map py to the python variant dir and skip __pycache__ artifacts during the copy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: read templates with explicit utf-8 encoding Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: keep py: lines standalone, enforce script existence in tests Drop the plan/tasks py: lines that referenced scripts shipping in the core port (#3280); they move to that PR. Tests now assert every py: line points at a script the repo ships, so a dangling reference can never merge green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
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> |
||
|
Zied Jlassi
|
b042d2a843 |
feat(extensions): verify catalog archive sha256 before install (#3080)
* feat(extensions): verify catalog archive sha256 before install Extension and preset archives were downloaded over HTTPS and unpacked (with Zip-Slip protection) but their bytes were never checked against a known digest. Trust rested entirely on TLS and the integrity of the release host, so a tampered or swapped archive from a compromised third-party release would be installed silently. Maintainers do not audit extension code, so consumer-side integrity is the only available defence. Catalog entries may now pin an optional `sha256` digest. When present, the downloaded archive is verified before it is written to disk and installed; a mismatch aborts with a clear error. Entries without `sha256` keep working unchanged (a DEBUG line records that the download was unverified), so the change is backwards compatible. The check runs on both download paths (extensions and presets) via a single shared helper so the two stay in parity. - Add `verify_archive_sha256` helper in shared_infra (digest match, `sha256:` prefix, case-insensitive; DEBUG log when no digest declared) - Enforce it in ExtensionCatalog.download_extension and PresetCatalog.download_pack, before the archive is written to disk - Document the optional `sha256` field in the publishing guides - Tests: helper unit tests + matching/mismatch/no-digest on both paths Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Assisted-by: AI * fix(extensions): harden sha256 parsing and tidy download test mocks Follow-up to the review on #3080: - shared_infra.verify_archive_sha256: strip only a literal `sha256:` algorithm prefix (case-insensitive) instead of `split(':', 1)[-1]`, which silently dropped any prefix — so `md5:<64-hex>` was accepted as if it were a valid SHA-256. Validate that the declared value is exactly 64 hex characters and raise a clear error otherwise, and compare with `hmac.compare_digest` for a constant-time check. Add tests covering a malformed digest and a non-`sha256:` prefix (both previously accepted). - Download test helpers: configure the context-manager mock via `__enter__.return_value`/`__exit__.return_value` rather than assigning a `lambda s: s`, which is clearer and independent of the invocation arity. Assisted-by: AI Signed-off-by: Zied Jlassi (Architect AI) <6190550+zied-jlassi@users.noreply.github.com> * fix(extensions): reject a declared-but-empty sha256 instead of skipping verification verify_archive_sha256 skipped on any falsy expected value, so a present-but-empty digest (e.g. sha256: "" reached via ...get("sha256")) silently disabled the integrity check instead of surfacing the authoring error. Guard on expected is None so only an absent digest skips; blank/whitespace/bare-prefix values fall through to the 64-hex validation and are rejected. Adds a regression test. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> * docs(shared_infra): clarify _SHA256_HEX_RE accepts and normalizes uppercase The comment described the regex as matching '64 lowercase' hex characters, but verify_archive_sha256 lowercases the declared value (raw.lower()) before matching, so an uppercase digest is accepted and normalized rather than rejected. Clarify the comment to avoid misleading future readers. Addresses Copilot review feedback on shared_infra.py. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> * test(presets): cover the no-sha256 backwards-compatible path Address Copilot review: download_pack's optional sha256 verification was tested for match/mismatch but not the backwards-compatible path where a catalog entry has no sha256 (pack_info.get("sha256") is None). Add a no-sha256 test mirroring the extensions coverage so the helper never silently becomes mandatory for presets. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> --------- Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com> Signed-off-by: Zied Jlassi (Architect AI) <6190550+zied-jlassi@users.noreply.github.com> |
||
|
José Villaseñor Montfort
|
e5a03bffc8 |
fix(shared-infra): remove stale managed scripts the core no longer ships (#3076) (#3098)
* fix(shared-infra): remove stale managed scripts the core no longer ships (#3076) install_shared_infra never removed shared scripts a prior (pre-refactor) install recorded but the current core no longer ships — e.g. the legacy scripts/<variant>/update-agent-context.sh, superseded by the bundled agent-context extension. On a legacy project the orphan lingers and crashes when it sources a refreshed common.sh (HAS_GIT unbound under set -u). Apply the stale-removal that integration_upgrade already performs to install_shared_infra: manifest-tracked scripts the current bundle no longer produces are removed, but only managed copies (hash matches the manifest); user-customized files, symlinks, and recovered entries are preserved. Guarded so a missing/empty source can't trigger mass deletion, and the safe-destination check prevents unlinking through a symlinked ancestor. Add IntegrationManifest.remove(); drop the stale update-agent-context.sh reference in CONTRIBUTING.md. AI assistance: implemented with Claude Code (Anthropic); reviewed and validated locally (ruff clean, full suite 4176 passed, manual CLI repro). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(shared-infra): harden stale-cleanup per review (empty source + orphan manifest) - Set scripts_scanned only after a real source file is seen, so an empty variant source can't trigger mass deletion of tracked scripts. - Prune a stale manifest entry even when its file is already gone from disk, keeping the manifest consistent (previously left tracked forever). - Add a test for each edge case. Addresses the Copilot review comments on #3098. AI assistance: Claude Code (Anthropic), reviewed/validated locally (ruff clean, full suite 4178 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(shared-infra): guard unsafe manifest keys in stale-cleanup (review) - Skip absolute / '..' manifest keys before any filesystem access in stale-cleanup, so a corrupted/hand-edited manifest can't make it touch paths outside the project root (mirrors IntegrationManifest.check_modified / uninstall). - Clarify the scripts_scanned comment: the safety hinge is that flag, not seen_rels (which also holds template paths). - Add a containment test: a traversal manifest key is skipped, its target untouched. Addresses the second round of Copilot review on #3098. AI assistance: Claude Code (Anthropic); validated locally (ruff clean, full suite 4179 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(manifest): make remove() reject absolute/.. keys like its siblings (review) IntegrationManifest.remove() now applies the same lexical validation and normalization as record_existing() / is_recovered(): absolute paths and '..' segments are rejected (return False) instead of being used verbatim as a key. Keeps the manifest API consistent. Adds tests (valid drop + no-op, absolute rejected, traversal rejected). Addresses the third round of Copilot review on #3098. AI assistance: Claude Code (Anthropic); validated locally (ruff clean, full suite 4182 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(shared-infra): validate stale-cleanup keys for containment, not just lexically (review) The stale-script cleanup guarded manifest keys with a lexical check only (is_absolute() / ".." segments). On Windows a drive-relative key such as "C:tmp\\file" is not is_absolute(), yet joining it onto the project path discards the root — so cleanup could stat/unlink outside the project before _ensure_safe_shared_destination raised, and a corrupted manifest key turned into an install-time hard failure (ValueError) instead of being skipped. Reuse the canonical containment helper (_validate_rel_path, the same one IntegrationManifest.is_recovered / remove use): after the fast lexical reject, resolve the join and confirm it stays within the project root; a key that still escapes is skipped, never unlinked, never fatal. Adds a regression test that forces _validate_rel_path to reject a managed key (portably simulating the Windows drive-relative escape) and asserts the install skips it without failing and still installs the real scripts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Manfred Riem
|
5ae7ff53d0 |
fix: skip recovered files during refresh_managed overwrite check (#2918) (#2919)
_is_managed() in install_shared_infra now consults manifest.is_recovered() before treating a hash-matching file as managed. Files marked recovered (pre-existing on disk, not installed by Spec Kit) are no longer overwritten by integration use/switch even when their hash matches the manifest entry. This closes the gap documented in the manifest API: callers using refresh_managed MUST check is_recovered first. Co-authored-by: Manfred Riem <mnriem@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
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 |
||
|
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 (
|
||
|
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> |
||
|
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. |
||
|
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> |
||
|
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 |