Skip to content

fix(shared-infra): refresh_shared_templates preserves recovered user files#3378

Open
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/refresh-templates-preserve-recovered
Open

fix(shared-infra): refresh_shared_templates preserves recovered user files#3378
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/refresh-templates-preserve-recovered

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Description

refresh_shared_templates decides whether to skip an existing shared template using only:

if dst.exists() and not force:
    if rel not in tracked_files or rel in modified:
        skipped_files.append(rel); continue

It never consults manifest.is_recovered(rel) — but install_shared_infra does (its _is_managed/is_recovered gate, added by #2918). A recovered file is a pre-existing user file whose on-disk hash was adopted via record_existing(recovered=True), so it is both tracked and hash-unmodified. Both predicates above are therefore false → the file is not skipped → refresh_shared_templates overwrites the user's content with the bundled body.

Reproduced on main @ 92b7cf7: a recovered spec-template.md (# USER CUSTOM CONTENT) is replaced with # BUNDLED CONTENT v2 on a non---force refresh — the exact data-loss class #2918 fixed for install_shared_infra.

Fix

Add the is_recovered check to the skip predicate, mirroring install_shared_infra. One source file, no API change.

Testing

New test_refresh_shared_templates_preserves_recovered_user_file: records a user template as recovered, points the bundled source at a different body, calls refresh_shared_templates(force=False), asserts the user content survives. Fails before (content replaced by bundled body — verified by source-stash), passes after. uvx ruff check clean.

AI Disclosure

  • I did use AI assistance (describe below)

Found and fixed with Claude Code (Claude Fable 5) under my direction. AI diffed the refresh skip-predicate against install_shared_infra's is_recovered gate; I reproduced the overwrite, verified fail-before/pass-after, and reviewed the diff.

…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 github#2918 fixed for
install_shared_infra. Add the is_recovered check to the skip predicate.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a data-loss edge case in shared template refresh logic by ensuring refresh_shared_templates() treats “recovered” (adopted) files as user-owned and does not overwrite them unless --force is used, aligning behavior with install_shared_infra from #2918.

Changes:

  • Update refresh_shared_templates() to skip overwriting templates marked as recovered in the Speckit manifest when force=False.
  • Add a regression test that records a user template as recovered and verifies refresh does not clobber its contents.
Show a summary per file
File Description
src/specify_cli/shared_infra.py Extend the non-force skip predicate to include manifest.is_recovered(rel) so recovered files are preserved.
tests/integrations/test_cli.py Add regression test covering recovered-template preservation during refresh.

Review details

Tip

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

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread src/specify_cli/shared_infra.py
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants