Skip to content

Commit b1a9694

Browse files
committed
refactor: address second round of PR review feedback
- Remove dead cache_file/cache_metadata_file attributes from IntegrationCatalog - Deduplicate non-default catalog warning (show once per process) - Anchor version regex to reject partial matches like 1.0.0beta - Fix 'Preserved modified' message to 'Skipped' for accuracy - Make upgrade transactional: install new files first, then remove stale old-only files, so a failed setup leaves old integration intact - Update CONTRIBUTING.md: speckit_version validates presence only
1 parent eeac3b2 commit b1a9694

3 files changed

Lines changed: 34 additions & 23 deletions

File tree

integrations/CONTRIBUTING.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ provides:
8080
| `schema_version` | Must be `"1.0"` |
8181
| `integration.id` | Lowercase alphanumeric + hyphens (`^[a-z0-9-]+$`) |
8282
| `integration.version` | Valid semantic version |
83-
| `requires.speckit_version` | PEP 440 version specifier |
83+
| `requires.speckit_version` | Required field; current validation checks presence only |
8484
| `provides` | Must include at least one command or script |
8585
| `provides.commands[].name` | String identifier |
8686
| `provides.commands[].file` | Relative path to template file |

src/specify_cli/__init__.py

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2286,15 +2286,8 @@ def integration_upgrade(
22862286

22872287
selected_script = _resolve_script_type(project_root, script)
22882288

2289-
# Phase 1: Teardown old files
2289+
# Phase 1: Install new files (overwrites existing; old-only files remain)
22902290
console.print(f"Upgrading integration: [cyan]{key}[/cyan]")
2291-
removed, skipped = old_manifest.uninstall(project_root, force=force)
2292-
if removed:
2293-
console.print(f" Removed {len(removed)} old file(s)")
2294-
if skipped:
2295-
console.print(f" [yellow]Preserved {len(skipped)} modified file(s)[/yellow]")
2296-
2297-
# Phase 2: Reinstall
22982291
new_manifest = IntegrationManifest(key, project_root, version=get_speckit_version())
22992292

23002293
parsed_options: dict[str, Any] | None = None
@@ -2319,15 +2312,33 @@ def integration_upgrade(
23192312
console.print(
23202313
f"[yellow]Warning:[/yellow] Teardown during rollback also failed: {teardown_exc}"
23212314
)
2322-
# Attempt to restore the old manifest so the project is not left broken
2323-
try:
2324-
old_manifest.save()
2325-
_write_integration_json(project_root, key, selected_script)
2326-
except Exception:
2327-
pass # Best-effort restoration; original error is more important
23282315
console.print(f"[red]Error:[/red] Failed to upgrade integration: {exc}")
23292316
raise typer.Exit(1)
23302317

2318+
# Phase 2: Remove stale files from old manifest that are not in the new one
2319+
old_files = set(old_manifest.files.keys())
2320+
new_files = set(new_manifest.files.keys())
2321+
stale_files = old_files - new_files
2322+
stale_removed = 0
2323+
for rel in stale_files:
2324+
path = project_root / rel
2325+
if path.exists() and path.is_file():
2326+
try:
2327+
path.unlink()
2328+
stale_removed += 1
2329+
# Clean up empty parent directories up to project root
2330+
parent = path.parent
2331+
while parent != project_root:
2332+
try:
2333+
parent.rmdir()
2334+
except OSError:
2335+
break
2336+
parent = parent.parent
2337+
except OSError:
2338+
pass
2339+
if stale_removed:
2340+
console.print(f" Removed {stale_removed} stale file(s) from previous install")
2341+
23312342
name = (integration.config or {}).get("name", key)
23322343
console.print(f"\n[green]✓[/green] Integration '{name}' upgraded successfully")
23332344

src/specify_cli/integrations/catalog.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,6 @@ class IntegrationCatalog:
6666
def __init__(self, project_root: Path) -> None:
6767
self.project_root = project_root
6868
self.cache_dir = project_root / ".specify" / "integrations" / ".cache"
69-
self.cache_file = self.cache_dir / "catalog.json"
70-
self.cache_metadata_file = self.cache_dir / "catalog-metadata.json"
7169

7270
# -- URL validation ---------------------------------------------------
7371

@@ -176,11 +174,13 @@ def get_active_catalogs(self) -> List[IntegrationCatalogEntry]:
176174
if env_value:
177175
self._validate_catalog_url(env_value)
178176
if env_value != self.DEFAULT_CATALOG_URL:
179-
print(
180-
"Warning: Using non-default integration catalog. "
181-
"Only use catalogs from sources you trust.",
182-
file=sys.stderr,
183-
)
177+
if not getattr(self, "_non_default_catalog_warning_shown", False):
178+
print(
179+
"Warning: Using non-default integration catalog. "
180+
"Only use catalogs from sources you trust.",
181+
file=sys.stderr,
182+
)
183+
self._non_default_catalog_warning_shown = True
184184
return [
185185
IntegrationCatalogEntry(
186186
url=env_value,
@@ -459,7 +459,7 @@ def _validate(self) -> None:
459459
"must be lowercase alphanumeric with hyphens only"
460460
)
461461

462-
if not re.match(r"^\d+\.\d+\.\d+", integ["version"]):
462+
if not re.match(r"^\d+\.\d+\.\d+$", integ["version"]):
463463
raise IntegrationDescriptorError(
464464
f"Invalid version '{integ['version']}': must use semantic versioning (e.g., 1.0.0)"
465465
)

0 commit comments

Comments
 (0)