fix(shared-infra): record skipped files in speckit.manifest.json

[eldar702]

Summary

In install_shared_infra (src/specify_cli/shared_infra.py), the skip branches in both the scripts loop and the templates loop now record each skipped file in speckit.manifest.json, so a fresh-manifest run against an already-populated .specify/ tree no longer writes an empty files field.

Fixes #2107

Problem

When install_shared_infra ran with force=False against a project that already had files under .specify/scripts/ and .specify/templates/ — but a fresh, empty manifest — every iteration hit the if dst.exists() and not force: skipped_files.append(...); continue branch. planned_copies and planned_templates stayed empty, the post-loop record loop had nothing to record, and manifest.save() serialised files: {}. The integration then believed nothing had been installed.

This bites users who delete or lose speckit.manifest.json, who extract .specify/ out-of-band, or who hit a code path where the manifest can't be loaded but the directory tree is intact.

Solution

Call manifest.record_existing(rel_skip) from inside both skip branches, but only when the path is not already tracked:

if dst_path.exists() and not force:
    rel_skip = dst_path.relative_to(project_path).as_posix()
    skipped_files.append(rel_skip)
    if rel_skip not in manifest.files:
        manifest.record_existing(rel_skip)
    continue

The guard matters: record_existing always re-hashes the on-disk content, so without it a customized template would have its manifest hash overwritten with the customized hash, defeating the user-modification detection that integration use relies on (test_use_preserves_modified_templates_unless_forced is the canonical regression test for that flow).

Symmetric change in both the scripts loop and the templates loop.

Test plan

• New regression test TestSpeckitManifestRecordsSkippedFiles::test_install_shared_infra_records_skipped_files — populates .specify/ via a normal first run, deletes the manifest, runs install_shared_infra again with force=False (every file goes down the skip branch), asserts the saved manifest is non-empty and is a superset of the first run's files. Fails on main, passes after the fix.
• Existing test_use_preserves_modified_templates_unless_forced continues to pass thanks to the if rel_skip not in manifest.files: guard — customized templates keep their original hash, so integration use still skips overwriting them.
• pytest full suite: 2787 passed, 34 skipped — zero regressions.

Notes

This change was AI-assisted. The fix was selected after a 3-agent solution-design debate where two designers independently identified the same root cause (skip branch in install_shared_infra failing to record), giving high convergent confidence. A third designer initially proposed a fix in claude/__init__.py, but that targets the integration manifest (claude.manifest.json), not the shared-infrastructure manifest (speckit.manifest.json) the issue describes — convergent analysis correctly localized the right file.

A first attempt at the fix omitted the if rel_skip not in manifest.files: guard and broke test_use_preserves_modified_templates_unless_forced (customized templates were silently re-hashed). Adding the guard fixed the regression while still recovering from a lost-manifest scenario.

[Copilot]

Pull request overview

This PR fixes a shared-infrastructure tracking bug where install_shared_infra(..., force=False) could skip all existing .specify/scripts/ and .specify/templates/ files without recording them, resulting in speckit.manifest.json being saved with an empty files mapping (issue #2107).

Changes:

• Record skipped (already-existing) shared scripts/templates into speckit.manifest.json during install_shared_infra when they are not already tracked.
• Add a regression test ensuring a “lost manifest” reinstall reconstructs a non-empty manifest that includes all previously tracked files.

Show a summary per file

File	Description
src/specify_cli/shared_infra.py	Records skipped existing shared infra files into the shared manifest during install.
tests/integrations/test_integration_claude.py	Adds a regression test covering the “lost manifest + skip branch” scenario.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

src/specify_cli/shared_infra.py:303

• Same issue as the scripts loop: calling manifest.record_existing(rel_skip) in the skip branch will raise if dst exists but is not a regular file (e.g., a directory). Add an dst.is_file() guard or raise an explicit error so a non-file collision doesn’t fail with an opaque hashing/open error.

                # ``.specify/`` tree does not silently drop it (#2107).
                # Skip if already tracked — preserving the original hash
                # keeps user-modification detection working downstream.
                if rel_skip not in manifest.files:
                    manifest.record_existing(rel_skip)

• Files reviewed: 2/2 changed files
• Comments generated: 2

[mnriem]

Please address Copilot feedback

[eldar702]

Thanks for the review! Pushed 6790654 to address all three Copilot findings.

What changed

1. Hardened IntegrationManifest.record_existing (the choke point)

The function's docstring already promised "an already-existing file", but didn't enforce it. It now raises ValueError if the path is a symlink or is not a regular file — mirroring the existing _ensure_safe_manifest_destination precedent in the same module. The is_symlink() check runs on the un-resolved path because _validate_rel_path calls resolve(), which would otherwise silently follow the symlink and record the target.

2. Guarded both call sites in install_shared_infra (the bug Copilot flagged at lines 277 + 303)

if dst_path.is_file() and rel_skip not in manifest.files:
    try:
        manifest.record_existing(rel_skip)
    except (OSError, ValueError) as exc:
        console.print(
            f"[yellow]⚠[/yellow]  could not record {rel_skip} in manifest: {exc}"
        )

Belt-and-suspenders: the is_file() guard short-circuits the common case (directory collision), and the try/except absorbs the rare TOCTOU race or permission error so a single weird path can't abort the whole install. The path still surfaces via the existing skipped_files warning at the bottom of the function.

3. Tightened the test helper (Copilot's low-confidence finding)

_read_manifest_files no longer falls back to data.get("_files"). It now asserts "files" in data and that the value is a dict, with descriptive failure messages. A future schema regression that renames the public key now fails red instead of being silently masked.

4. New regression tests

Two new methods in TestSpeckitManifestRecordsSkippedFiles plant a directory at the expected destination of a script and a template, then assert (a) install completes, (b) the path is NOT recorded in the manifest, (c) the path still appears in the user-visible warning.

5. Cosmetic

The user-facing warning string "shared infrastructure file(s) already exist" → "shared infrastructure path(s) already exist", since non-file entries can now legitimately land in skipped_files.

Verification

• All 1950 integrations tests + the new tests pass: pytest tests/integrations/ → passed
• Full repo: pytest tests/ → 2789 passed, 34 skipped
• Adversarial unit-checks confirm record_existing raises ValueError for directory, symlink, and out-of-root path; succeeds for a regular file.

Out of scope (intentionally)

• refresh_shared_templates (line 212 area) — its record_existing call runs after _write_shared_text, never on the skip branch, so it is not affected by this bug class.
• Pre-existing behavior when force=True and a directory occupies a destination — outside the scope of [Bug]: speckit install creates manifest with empty files — no skills available #2107.

[Copilot]

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 3

[mnriem]

Please address Copilot feedback and resolve conflicts

[eldar702]

Thanks for the bump @mnriem! Pushed 5a6e25b (rebased on main) addressing both your asks.

1. Rebased onto current main

The conflict was the new _decide_overwrite / _safe_dest_or_bucket / _ensure_or_bucket_dir helpers from #2375 (the refresh-managed refactor). I integrated my skip-branch manifest-recording logic into the new if not write: ... else: skipped_files.append(rel) path of both loops, so the fix still applies even when refresh_managed=True decides not to overwrite a file. mergeStateStatus is now CLEAN.

2. Copilot re-review (review id 4266902103, 2026-05-11)

All three findings addressed:

(a) Lexical pre-check in record_existing — manifest.py. The old order did (self.project_root / rel).is_symlink() before _validate_rel_path(), so an absolute path or one with .. segments triggered a filesystem stat outside the project root before the path was rejected. Added a cheap lexical pre-check up front that delegates to _validate_rel_path for the canonical error messages; the symlink stat now only ever runs on paths that are already lexically inside the project root.

(b) Focused unit tests for the new error cases — tests/integrations/test_manifest.py. Added TestManifestRecordExistingErrors with five cases:

• 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

(c) Avoid repeated dict(self._files) copies in the skip branches — shared_infra.py. Reused prior_hashes — the function-scope snapshot already taken at the top of install_shared_infra for the refresh logic — for the membership check, so manifest.files (which copies on every access) is no longer called in the hot loops. This shares the existing snapshot rather than introducing a parallel cache.

Verification

• Full test suite: 2925 passed, 35 skipped, 0 failed (43.8s).
• Identity: eldar702 <eldarshlomi7@gmail.com> on the new commit.

🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).

[Copilot]

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 4

[eldar702]

Thanks @copilot-pull-request-reviewer for the additional pass. Pushed 00710c8 addressing all four findings.

1 & 2. Recovery semantics (shared_infra.py:371, shared_infra.py:412)

Both call sites now pass recovered=True. The flag flows through to a new top-level recovered_files: list[str] in the manifest JSON — additively, so the existing files: dict[str, str] schema is unchanged and the load-validator at line 336 still asserts isinstance(v, str) without ambiguity. Future refresh_managed can call manifest.is_recovered(rel) and decline to treat the recorded hash as a managed baseline until the user opts in with --refresh-shared-infra, defending against silent overwrite of customizations after manifest loss.

I deliberately chose this over hash-validation against the bundled bytes: processed templates undergo resolve_command_refs before being written, so on-disk bytes can never byte-equal the raw bundled bytes, and re-computing the processed bytes at recovery time requires user context that isn't recoverable. The recovered_files channel is honest: "we observed this file, but we cannot vouch it is the managed version."

3. Symlinked-ancestor walk (manifest.py:172)

Replaced the leaf-only is_symlink() check with a component-walk:

_walk = self.project_root
for part in rel.parts:
    _walk = _walk / part
    if _walk.is_symlink():
        raise ValueError(
            f"Refusing to record symlinked manifest path: {rel} "
            f"(symlinked at {_walk.relative_to(self.project_root).as_posix()})"
        )

This mirrors the pattern already in _ensure_safe_manifest_directory (manifest.py:67-72). New test test_rejects_symlinked_ancestor plants linked_dir -> real_dir and asserts record_existing("linked_dir/file.txt") raises before any resolution can follow the symlink.

4. .. rejection message (manifest.py:168)

You offered two options; I took the second — kept the strict rejection but replaced the misleading "escapes project root" message with: "Manifest paths must be canonical; '..' segments are not allowed (got {rel})". Manifest keys must be canonical so check_modified/uninstall don't key the same file under two paths; the old message was correct in intent but wrong in wording for dir/../file.txt-style paths.

Verification

• Full suite: 2932 passed, 35 skipped, 0 failed.
• New tests: 7 across TestManifestRecoveredFiles and TestRecordExistingNewGuards.
• Diff: 3 files, +145/-11 LOC.
• Identity: eldar702 <eldarshlomi7@gmail.com>.

🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).

[Copilot]

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 1

[mnriem]

Please address Copilot feedback

[eldar702]

Thanks @copilot-pull-request-reviewer / @mnriem. Pushed 25051ff addressing the round-5 finding.

The fix

is_recovered now normalizes its input through the same _validate_rel_path pipeline that record_existing uses for its stored keys — taking Copilot's verbatim option A. ValueError from the validator is caught and converted to False so query semantics stay exception-free (Python __contains__ convention: a not-in-set query never raises).

def is_recovered(self, rel_path: str | Path) -> bool:
    try:
        abs_path = _validate_rel_path(Path(rel_path), self.project_root)
        normalized = abs_path.relative_to(self.project_root).as_posix()
    except ValueError:
        return False
    return normalized in self._recovered_files

2 new tests in TestManifestRecoveredFiles: test_is_recovered_absolute_path_returns_false and test_is_recovered_escaping_path_returns_false.

Verification

• Manifest tests: 43 passed (7 existing for the recovery feature + 2 new + 34 unrelated)
• Full suite: 2934 passed, 35 skipped, 0 failed
• Identity: eldar702 <eldarshlomi7@gmail.com>
• Diff: 2 files, +35/-3 LOC

Question for @mnriem

This PR has now had 5 review rounds since it was opened on 2026-05-07 — your initial review, then 3 successive Copilot rounds each finding adjacent-but-distinct issues, then today's. Each fix has been small and tests stay green, but Copilot has no "approve" state, only "more findings" or silence. Could you confirm whether your "Please address Copilot feedback" comment is the merge gate, or whether Copilot signoff is itself required? Happy to iterate further if needed — just want to make sure we're converging.

🤖 AI disclosure: drafted with assistance from Claude (Opus 4.7).

[mnriem]

As a general rule of thumb Copilot feedback must be addressed until it no longer has anything to report. This keeps the corner cases covered. Note that everyone including maintainers have to go through that same cycle. I realize this takes a while and often looks like it just keeps on giving, but overall I think this is the best for the entire codebase

[Copilot]

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 2

[mnriem]

Please address Copilot feedback. If not applicable, please explain why

[eldar702]

Thanks @copilot-pull-request-reviewer / @mnriem. Pushed 1dbf0c2 addressing both Round-7 findings.

Finding A — recovered marker now clears on managed re-record

record_existing(recovered=False) now discards the normalized key from _recovered_files, and record_file does the same. The intent of _recovered_files is "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. record_file's docstring also gained a one-line note about the side effect.

Finding B — is_recovered now matches record_existing's canonicalization

Added a lexical guard to is_recovered that rejects absolute paths AND .. segments up-front (returns False, no raise — preserving the Python __contains__ convention). This matches record_existing's canonical-key guard exactly, so the docstring's equivalence claim is now true. _validate_rel_path still runs after the lexical guard for defense-in-depth on paths that escape the root after resolution.

Tests

3 new in TestManifestRecoveredFiles:

• test_record_existing_clears_recovered_when_false
• test_record_file_clears_recovered
• test_is_recovered_rejects_dotdot_segment

Verification

• Manifest tests: pytest tests/integrations/test_manifest.py -v — 46/46 green (including the 3 new)
• Full suite: 2937 passed, 35 skipped, 0 failed
• Diff: 2 files, +52/-7 LOC
• Identity: eldar702 <eldarshlomi7@gmail.com>

🤖 AI disclosure: drafted with Claude (Opus 4.7).

[Copilot]

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 1

[mnriem]

Please address Copilot feedback

[eldar702]

Thanks @copilot-pull-request-reviewer / @mnriem. Pushed 7666030 — pure comment fix on the two is_recovered tests so they describe the actual Round-7 code path (early lexical is_absolute() / '..' in rel.parts guards) rather than the prior "_validate_rel_path raises; we catch" wording. No assertion or behavior change; same suite still green.

Pre-empted the parallel comment in test_is_recovered_escaping_path_returns_false while in the area — same comment-class drift would have been Round-9 material otherwise.

🤖 AI disclosure: drafted with Claude (Opus 4.7).