Files
github-spec-kit/tests/integrations/test_manifest.py
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

543 lines
23 KiB
Python

"""Tests for IntegrationManifest — record, hash, save, load, uninstall, modified detection."""
import hashlib
import json
import sys
import pytest
from specify_cli.integrations.manifest import IntegrationManifest, _sha256
class TestManifestRecordFile:
def test_record_file_writes_and_hashes(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
content = "hello world"
abs_path = m.record_file("a/b.txt", content)
assert abs_path == tmp_path / "a" / "b.txt"
assert abs_path.read_text(encoding="utf-8") == content
expected_hash = hashlib.sha256(content.encode()).hexdigest()
assert m.files["a/b.txt"] == expected_hash
def test_record_file_bytes(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
data = b"\x00\x01\x02"
abs_path = m.record_file("bin.dat", data)
assert abs_path.read_bytes() == data
assert m.files["bin.dat"] == hashlib.sha256(data).hexdigest()
def test_record_existing(self, tmp_path):
f = tmp_path / "existing.txt"
f.write_text("content", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("existing.txt")
assert m.files["existing.txt"] == _sha256(f)
class TestManifestRecordExistingErrors:
"""Error-case coverage for ``record_existing`` symlink + non-file guards.
Added in #2483 — Copilot review flagged these as un-tested regressions
after the ``is_symlink``/``is_file`` guards were introduced.
"""
def test_rejects_symlink_target(self, tmp_path):
target = tmp_path / "target.txt"
target.write_text("target content", encoding="utf-8")
link = tmp_path / "link.txt"
link.symlink_to(target)
m = IntegrationManifest("test", tmp_path)
with pytest.raises(ValueError, match="symlinked"):
m.record_existing("link.txt")
def test_rejects_dangling_symlink(self, tmp_path):
# A symlink pointing nowhere should still be rejected before the
# ``is_file()`` check (which would itself be False on a dangler).
link = tmp_path / "dangler.txt"
link.symlink_to(tmp_path / "no-such-target.txt")
m = IntegrationManifest("test", tmp_path)
with pytest.raises(ValueError, match="symlinked"):
m.record_existing("dangler.txt")
def test_rejects_directory_path(self, tmp_path):
(tmp_path / "a_dir").mkdir()
m = IntegrationManifest("test", tmp_path)
with pytest.raises(ValueError, match="not a regular file"):
m.record_existing("a_dir")
def test_rejects_missing_path(self, tmp_path):
# ``is_file()`` is False for non-existent paths too; the same error
# surface keeps callers from having to distinguish "missing" from
# "wrong kind" — both mean "cannot hash this".
m = IntegrationManifest("test", tmp_path)
with pytest.raises(ValueError, match="not a regular file"):
m.record_existing("never-existed.txt")
def test_lexical_prevalidation_for_absolute_path(self, tmp_path):
# ``record_existing`` must reject absolute paths via the lexical
# pre-check, NOT via the filesystem-touching ``is_symlink()`` call.
# Verified by passing an absolute path that points to a directory
# outside the project root — the canonical "Absolute paths" error
# must surface before any stat on the absolute path.
m = IntegrationManifest("test", tmp_path)
abs_path = "C:\\tmp\\escape.txt" if sys.platform == "win32" else "/tmp/escape.txt"
with pytest.raises(ValueError, match="Absolute paths"):
m.record_existing(abs_path)
class TestManifestPathTraversal:
def test_record_file_rejects_parent_traversal(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
with pytest.raises(ValueError, match="outside"):
m.record_file("../escape.txt", "bad")
def test_record_file_rejects_absolute_path(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
abs_path = "C:\\tmp\\escape.txt" if sys.platform == "win32" else "/tmp/escape.txt"
with pytest.raises(ValueError, match="Absolute paths"):
m.record_file(abs_path, "bad")
def test_record_existing_rejects_parent_traversal(self, tmp_path):
escape = tmp_path.parent / "escape.txt"
escape.write_text("evil", encoding="utf-8")
try:
m = IntegrationManifest("test", tmp_path)
with pytest.raises(ValueError, match="outside"):
m.record_existing("../escape.txt")
finally:
escape.unlink(missing_ok=True)
def test_uninstall_skips_traversal_paths(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("safe.txt", "good")
m._files["../outside.txt"] = "fakehash"
m.save()
removed, skipped = m.uninstall()
assert len(removed) == 1
assert removed[0].name == "safe.txt"
def test_remove_drops_entry_and_is_noop_second_time(self, tmp_path):
(tmp_path / "f.txt").write_text("x", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("f.txt")
assert "f.txt" in m.files
assert m.remove("f.txt") is True
assert "f.txt" not in m.files
assert m.remove("f.txt") is False # already gone → no-op
def test_remove_rejects_absolute_path(self, tmp_path):
# Matches record_existing/is_recovered: an absolute key can never be a
# canonical manifest key, so remove() rejects it lexically and leaves
# the tracked entry untouched.
(tmp_path / "f.txt").write_text("x", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("f.txt")
import sys
abs_input = "C:\\tmp\\f.txt" if sys.platform == "win32" else "/tmp/f.txt"
assert m.remove(abs_input) is False
assert "f.txt" in m.files
def test_remove_rejects_parent_traversal(self, tmp_path):
(tmp_path / "f.txt").write_text("x", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("f.txt")
assert m.remove("../f.txt") is False
assert "f.txt" in m.files
class TestManifestCheckModified:
def test_unmodified_file(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "original")
assert m.check_modified() == []
def test_modified_file(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "original")
(tmp_path / "f.txt").write_text("changed", encoding="utf-8")
assert m.check_modified() == ["f.txt"]
def test_deleted_file_not_reported(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "original")
(tmp_path / "f.txt").unlink()
assert m.check_modified() == []
def test_symlink_treated_as_modified(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "original")
target = tmp_path / "target.txt"
target.write_text("target", encoding="utf-8")
(tmp_path / "f.txt").unlink()
(tmp_path / "f.txt").symlink_to(target)
assert m.check_modified() == ["f.txt"]
class TestManifestUninstall:
def test_removes_unmodified(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("d/f.txt", "content")
m.save()
removed, skipped = m.uninstall()
assert len(removed) == 1
assert not (tmp_path / "d" / "f.txt").exists()
assert not (tmp_path / "d").exists()
assert skipped == []
def test_skips_modified(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "original")
m.save()
(tmp_path / "f.txt").write_text("modified", encoding="utf-8")
removed, skipped = m.uninstall()
assert removed == []
assert len(skipped) == 1
assert (tmp_path / "f.txt").exists()
def test_force_removes_modified(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "original")
m.save()
(tmp_path / "f.txt").write_text("modified", encoding="utf-8")
removed, skipped = m.uninstall(force=True)
assert len(removed) == 1
assert skipped == []
def test_already_deleted_file(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "content")
m.save()
(tmp_path / "f.txt").unlink()
removed, skipped = m.uninstall()
assert removed == []
assert skipped == []
def test_removes_manifest_file(self, tmp_path):
m = IntegrationManifest("test", tmp_path, version="1.0")
m.record_file("f.txt", "content")
m.save()
assert m.manifest_path.exists()
m.uninstall()
assert not m.manifest_path.exists()
def test_remove_manifest_false_preserves_manifest_file(self, tmp_path):
"""Regression (review #3415, 4724160183): a partial cleanup must not
delete ``{key}.manifest.json``.
The upgrade stale-file pass builds a throwaway manifest sharing the
integration's key over a subset of files and uninstalls it. With
``remove_manifest=False`` the tracked files are still removed but the
real, freshly-saved manifest for that key survives — otherwise a
layout-shrinking upgrade (e.g. Bob migrating legacy commands → skills)
would leave the integration untracked and un-upgradeable.
"""
m = IntegrationManifest("test", tmp_path, version="1.0")
m.record_file("f.txt", "content")
m.save()
assert m.manifest_path.exists()
removed, skipped = m.uninstall(remove_manifest=False)
assert len(removed) == 1
assert not (tmp_path / "f.txt").exists()
assert m.manifest_path.exists(), (
"remove_manifest=False must keep the manifest file on disk"
)
def test_cleans_empty_parent_dirs(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("a/b/c/f.txt", "content")
m.save()
m.uninstall()
assert not (tmp_path / "a").exists()
def test_preserves_nonempty_parent_dirs(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("a/b/tracked.txt", "content")
(tmp_path / "a" / "b" / "other.txt").write_text("keep", encoding="utf-8")
m.save()
m.uninstall()
assert not (tmp_path / "a" / "b" / "tracked.txt").exists()
assert (tmp_path / "a" / "b" / "other.txt").exists()
def test_symlink_skipped_without_force(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "original")
m.save()
target = tmp_path / "target.txt"
target.write_text("target", encoding="utf-8")
(tmp_path / "f.txt").unlink()
(tmp_path / "f.txt").symlink_to(target)
removed, skipped = m.uninstall()
assert removed == []
assert len(skipped) == 1
def test_symlink_removed_with_force(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "original")
m.save()
target = tmp_path / "target.txt"
target.write_text("target", encoding="utf-8")
(tmp_path / "f.txt").unlink()
(tmp_path / "f.txt").symlink_to(target)
removed, skipped = m.uninstall(force=True)
assert len(removed) == 1
assert target.exists()
class TestManifestPersistence:
def test_save_and_load_roundtrip(self, tmp_path):
m = IntegrationManifest("myagent", tmp_path, version="2.0.1")
m.record_file("dir/file.md", "# Hello")
m.save()
loaded = IntegrationManifest.load("myagent", tmp_path)
assert loaded.key == "myagent"
assert loaded.version == "2.0.1"
assert loaded.files == m.files
def test_manifest_path(self, tmp_path):
m = IntegrationManifest("copilot", tmp_path)
assert m.manifest_path == tmp_path / ".specify" / "integrations" / "copilot.manifest.json"
def test_load_missing_raises(self, tmp_path):
with pytest.raises(FileNotFoundError):
IntegrationManifest.load("nonexistent", tmp_path)
def test_save_creates_directories(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "content")
path = m.save()
assert path.exists()
data = json.loads(path.read_text(encoding="utf-8"))
assert data["integration"] == "test"
def test_save_preserves_installed_at(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "content")
m.save()
first_ts = m._installed_at
m.save()
assert m._installed_at == first_ts
class TestManifestLoadValidation:
def test_load_non_dict_raises(self, tmp_path):
path = tmp_path / ".specify" / "integrations" / "bad.manifest.json"
path.parent.mkdir(parents=True)
path.write_text('"just a string"', encoding="utf-8")
with pytest.raises(ValueError, match="JSON object"):
IntegrationManifest.load("bad", tmp_path)
def test_load_bad_files_type_raises(self, tmp_path):
path = tmp_path / ".specify" / "integrations" / "bad.manifest.json"
path.parent.mkdir(parents=True)
path.write_text(json.dumps({"files": ["not", "a", "dict"]}), encoding="utf-8")
with pytest.raises(ValueError, match="mapping"):
IntegrationManifest.load("bad", tmp_path)
def test_load_bad_files_values_raises(self, tmp_path):
path = tmp_path / ".specify" / "integrations" / "bad.manifest.json"
path.parent.mkdir(parents=True)
path.write_text(json.dumps({"files": {"a.txt": 123}}), encoding="utf-8")
with pytest.raises(ValueError, match="mapping"):
IntegrationManifest.load("bad", tmp_path)
def test_load_invalid_json_raises(self, tmp_path):
path = tmp_path / ".specify" / "integrations" / "bad.manifest.json"
path.parent.mkdir(parents=True)
path.write_text("{not valid json", encoding="utf-8")
with pytest.raises(ValueError, match="invalid JSON"):
IntegrationManifest.load("bad", tmp_path)
def test_load_filters_recovered_files_not_in_files(self, tmp_path):
# Finding B (Round-9): a recovered_files entry referencing a path
# not present in files indicates an internally-inconsistent manifest
# (e.g. external edit). load() filters those entries silently so the
# manifest self-heals on next save(); is_recovered then returns the
# truthful False for the orphan.
path = tmp_path / ".specify" / "integrations" / "test.manifest.json"
path.parent.mkdir(parents=True)
path.write_text(json.dumps({
"integration": "test",
"files": {"kept.txt": "abc123"},
"recovered_files": ["kept.txt", "orphan.txt"],
}), encoding="utf-8")
m = IntegrationManifest.load("test", tmp_path)
assert m.recovered_files == {"kept.txt"}
assert m.is_recovered("kept.txt") is True
assert m.is_recovered("orphan.txt") is False
class TestManifestRecoveredFiles:
"""Coverage for the ``recovered_files`` channel added in #2483.
When ``shared_infra`` skips an existing file (because the user already has
it on disk) it now records the file with ``recovered=True``. The path
appears in ``manifest.recovered_files`` and ``is_recovered(path)`` returns
True. ``refresh_managed`` (out of scope for this PR) consults this list
before treating the recorded hash as a managed baseline, defending against
silent overwrite of user customizations after manifest loss.
"""
def test_record_existing_default_is_not_recovered(self, tmp_path):
(tmp_path / "f.txt").write_text("x", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("f.txt")
assert m.is_recovered("f.txt") is False
assert m.recovered_files == set()
def test_record_existing_with_recovered_flag(self, tmp_path):
(tmp_path / "f.txt").write_text("x", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("f.txt", recovered=True)
assert m.is_recovered("f.txt") is True
assert m.recovered_files == {"f.txt"}
# File still hashed normally so check_modified/uninstall keep working
assert m.files["f.txt"] == _sha256(tmp_path / "f.txt")
def test_recovered_files_round_trips_through_save_load(self, tmp_path):
(tmp_path / "a.txt").write_text("aaa", encoding="utf-8")
(tmp_path / "b.txt").write_text("bbb", encoding="utf-8")
m = IntegrationManifest("test", tmp_path, version="9.9")
m.record_existing("a.txt", recovered=True)
m.record_existing("b.txt") # not recovered
m.save()
loaded = IntegrationManifest.load("test", tmp_path)
assert loaded.is_recovered("a.txt") is True
assert loaded.is_recovered("b.txt") is False
assert loaded.recovered_files == {"a.txt"}
def test_save_omits_empty_recovered_files(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("f.txt", "x")
path = m.save()
data = json.loads(path.read_text(encoding="utf-8"))
assert "recovered_files" not in data
def test_load_rejects_non_list_recovered_files(self, tmp_path):
path = tmp_path / ".specify" / "integrations" / "bad.manifest.json"
path.parent.mkdir(parents=True)
path.write_text(
json.dumps({"files": {}, "recovered_files": "not-a-list"}),
encoding="utf-8",
)
with pytest.raises(ValueError, match="recovered_files"):
IntegrationManifest.load("bad", tmp_path)
def test_is_recovered_absolute_path_returns_false(self, tmp_path):
# Copilot round-5 finding: passing an absolute path silently returned
# False because the stored keys are relative POSIX strings. Round-7
# made this explicit: ``is_recovered`` now rejects absolute paths
# up front via a lexical ``rel.is_absolute()`` guard and returns
# False without calling ``_validate_rel_path`` at all — matching
# ``record_existing``'s canonical-key guard so the two methods
# agree on which inputs can ever be stored keys.
(tmp_path / "f.txt").write_text("x", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("f.txt", recovered=True)
import sys
abs_input = "C:\\tmp\\f.txt" if sys.platform == "win32" else "/tmp/f.txt"
assert m.is_recovered(abs_input) is False
def test_is_recovered_escaping_path_returns_false(self, tmp_path):
# A relative path containing ``..`` segments cannot be a stored key:
# Round-7 added the same lexical ``".." in rel.parts`` guard to
# ``is_recovered`` that ``record_existing`` already enforces, so the
# method returns False immediately without reaching
# ``_validate_rel_path``. The try/except around ``_validate_rel_path``
# remains as defense-in-depth for paths that pass the lexical guard
# but still resolve outside the project root via a symlinked
# ancestor.
m = IntegrationManifest("test", tmp_path)
# Don't record anything — the path is impossible to record anyway.
assert m.is_recovered("../escape.txt") is False
def test_record_existing_clears_recovered_when_false(self, tmp_path):
# Finding A: re-recording the same path with recovered=False must
# drop the prior recovered marker (transition to managed baseline).
f = tmp_path / "x.txt"
f.write_text("v1", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("x.txt", recovered=True)
assert m.is_recovered("x.txt") is True
m.record_existing("x.txt", recovered=False)
assert m.is_recovered("x.txt") is False
def test_record_file_clears_recovered(self, tmp_path):
# Finding A: record_file writes produced content; the path can no
# longer be considered "merely observed" once we wrote bytes.
(tmp_path / "y.txt").write_text("observed", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("y.txt", recovered=True)
assert m.is_recovered("y.txt") is True
m.record_file("y.txt", "produced")
assert m.is_recovered("y.txt") is False
def test_is_recovered_rejects_dotdot_segment(self, tmp_path):
# Finding B: record_existing rejects ``..`` segments via the lexical
# pre-check; is_recovered must match that behavior and return False
# without raising, mirroring the canonicalization guard.
(tmp_path / "z.txt").write_text("v1", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
m.record_existing("z.txt", recovered=True)
# Same file via dotdot-normalizing path — must be False, not raise.
assert m.is_recovered("subdir/../z.txt") is False
class TestRecordExistingNewGuards:
"""Coverage for the two new guards added by Copilot's 2026-05-18 review."""
def test_rejects_symlinked_ancestor(self, tmp_path):
real_dir = tmp_path / "real_dir"
real_dir.mkdir()
(real_dir / "file.txt").write_text("payload", encoding="utf-8")
(tmp_path / "linked_dir").symlink_to(real_dir, target_is_directory=True)
m = IntegrationManifest("test", tmp_path)
with pytest.raises(ValueError, match="symlinked"):
m.record_existing("linked_dir/file.txt")
def test_rejects_inside_root_dotdot_with_explicit_message(self, tmp_path):
# ``dir/../file.txt`` normalizes inside root, so the old "escapes
# project root" message was misleading. The new message names the
# actual reason: canonicalization.
(tmp_path / "dir").mkdir()
(tmp_path / "file.txt").write_text("x", encoding="utf-8")
m = IntegrationManifest("test", tmp_path)
with pytest.raises(ValueError, match=r"canonical|'\.\.' segments"):
m.record_existing("dir/../file.txt")
class TestManifestUnreadableFile:
"""A managed file that is unreadable (e.g. PermissionError) must not crash
check_modified()/uninstall() — the CLI handlers surfaced a raw traceback."""
def _mk(self, tmp_path):
m = IntegrationManifest("test", tmp_path)
m.record_file("sub/f.md", "content")
return m
def test_check_modified_treats_unreadable_as_modified(self, tmp_path, monkeypatch):
m = self._mk(tmp_path)
def raise_perm(_path):
raise PermissionError("unreadable")
monkeypatch.setattr(
"specify_cli.integrations.manifest._sha256", raise_perm
)
# Before the fix this raised PermissionError.
assert m.check_modified() == ["sub/f.md"]
def test_uninstall_preserves_unreadable_file(self, tmp_path, monkeypatch):
m = self._mk(tmp_path)
def raise_perm(_path):
raise PermissionError("unreadable")
monkeypatch.setattr(
"specify_cli.integrations.manifest._sha256", raise_perm
)
removed, skipped = m.uninstall(force=False)
# Can't verify ownership => preserve, don't crash and don't delete.
assert removed == []
assert (tmp_path / "sub" / "f.md") in skipped
assert (tmp_path / "sub" / "f.md").exists()