feat(extensions): add extension deprecation support to CLI#1455
Conversation
…b#425) Add `swamp extension deprecate` and `swamp extension undeprecate` commands, plus deprecation notices in search, info, pull, outdated, update, and list output. Deprecated extensions remain pullable — this is a soft lifecycle state that signals users to migrate without breaking existing workflows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix checkExtensionVersion to check version before deprecation so updates (including security patches) are never silently skipped for deprecated extensions. Deprecation only surfaces when already up-to-date. - Add 4 missing tests: deprecated path in checkExtensionVersion (with successor, with null deprecatedAt fallthrough), deprecated+update returns update_available, buildUpdateResult counts deprecated correctly. - Add pull deprecated_warning event test. - Cache pre-fetched extension info in pull generator to avoid double registry API call. - Fix column alignment in extension info renderer (Superseded: 13 chars). - Add hasDeprecated field to outdated JSON output for script parity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Clean, well-structured PR that adds extension deprecation/undeprecation as a soft lifecycle state. The implementation closely mirrors the existing yank/unyank pattern, which makes the codebase consistent and easy to navigate.
Blocking Issues
None.
Suggestions
-
Comment positioning in
extension_outdated.ts:88-89: The explanatory comment about filtered statuses (up_to_dateandupdated) now sits directly after thedeprecatedcase's closing brace rather than at the end of the switch. It still reads correctly, but the indentation could be misleading — a reader might think it's about thedeprecatedcase specifically. Consider moving it above theswitchor after its closing}. -
checkExtensionVersionprecedence logic is well-reasoned: The decision to surfaceupdate_availableoverdeprecated(so security patches aren't skipped) with the pull warning covering the deprecated+update case is a solid design choice. The comment in the domain service explaining this is helpful.
DDD Assessment
- Proper layered architecture: CLI → libswamp generator → infrastructure deps
DeprecatedStatuscorrectly extends theExtensionUpdateStatusdiscriminated union in the domain service- Dependency injection via
ExtensionDeprecateDeps/ExtensionUndeprecateDepsinterfaces follows the established pattern - Input validation lives in the preview (application layer), generator assumes valid input — consistent with yank/unyank
buildUpdateResultcorrectly counts deprecated extensions intotalwithout incrementingupToDate,updated, orfailed
Other Notes
- All new files have AGPLv3 license headers ✓
- libswamp import boundary respected — CLI commands and renderers import from
mod.ts✓ - Both
logandjsonoutput modes supported in all renderers ✓ - 14 new tests covering generators, previews, domain service, and pull integration ✓
- API client methods inherit the existing 15s AbortSignal timeout from
this.fetch✓ mod.tsbarrel exports all new public types ✓
There was a problem hiding this comment.
CLI UX Review
Blocking
None.
Suggestions
-
extension updatelog summary silently drops deprecated extensions —src/presentation/renderers/extension_update.ts:225prints e.g.3 extension(s): 0 updated, 1 up to date, 0 failedwith no mention of the 2 deprecated ones. The individual warn lines appear above, so the data isn't lost, but the summary line can mislead users who scan only that line. Consider appending, N deprecatedwhen the count is nonzero (same patternoutdateduses withhasDeprecated). -
UpdateSummarymath doesn't add up in JSON mode —src/domain/extensions/extension_update_service.ts:totalcounts deprecated extensions but none ofupToDate / updated / faileddo, sototal > upToDate + updated + failedwhenever any extension is deprecated. (The same gap exists today fornot_found.) Scripts that validate summary math will be surprised. Adeprecatedfield inUpdateSummarywould close this, similar to howoutdatedexplicitly exposeshasDeprecated. -
deprecatedByUserIdleaks intoextension info --json—src/libswamp/extensions/info.ts:e.datais serialized wholesale in JSON mode, sodeprecatedByUserId(an internal registry user ID) appears in the output even though it is never shown in log mode. Whether that exposure is intentional is worth a quick check; if it is, it should arguably appear in log mode too (similar to howyankedAtandyankReasonare both shown).
Verdict
PASS — both new commands (deprecate / undeprecate) and all five updated surfaces (search, info, pull, outdated, list/update) handle log and JSON modes correctly. Flag names, confirmation prompts, and error messages are consistent with the existing yank/unyank pattern. Exit codes are correct (deprecated does not trigger exit 1). No blocking issues.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None found. The code follows established patterns (yank/unyank), error handling is consistent, and the logic is correct across all production paths.
Medium
-
Dead code path:
extension_list --check-updateswill never show(deprecated)—src/presentation/renderers/extension_list.ts:106and the"deprecated"union member inEnrichedExtensionListEntry.updateStatus(line 45) are wired into the renderer, but the data pipeline that feeds them can never produce the value. Two structural barriers:src/cli/commands/extension_list_freshness.ts:32—ExtensionListFreshnessDeps.getLatestVersionreturnsstring | null, discarding all deprecation fields. The wiring atextension_list.ts:149-150fetches the fullExtensionInfofrom the API client (which now includesdeprecatedAt, etc.) but only returnsinfo?.latestVersion ?? null.extension_list_freshness.ts:154—checkExtensionVersionis called without the 4thdeprecationparameter, so it can never return"deprecated".extension_list_freshness.ts:158-160— Even if those two issues were fixed, the mapping only distinguishes"update_available"from everything else (mapping the rest to"up_to_date"), so"deprecated"would still be mapped to"up_to_date".
Net effect: The design doc (lines 9-33 of the design/extension.md changes) claims
swamp extension list --check-updatesmarks deprecated extensions with(deprecated), but this doesn't work. Not a regression (the renderer and type additions are correct), but the feature is incomplete. Consider a follow-up to wiregetLatestVersion→ a richer return type that includes deprecation fields, pass them tocheckExtensionVersion, and propagate the status.
Low
extension_outdated.tsrenderer switch comment placement —src/presentation/renderers/extension_outdated.ts:88-89: the comment// up_to_date and updated are filtered before reaching the renderernow sits outside thedeprecatedcase block but still inside the switch body, which reads as if it explains whydeprecatedis the last case rather than whyup_to_date/updatedare absent. No functional issue — just potentially confusing for future readers.
Verdict
PASS — The core deprecation/undeprecation flow, update service integration, pull warnings, search/info/outdated surfacing, and exit-code semantics are all correctly implemented with good test coverage. The extension_list --check-updates gap is a missing data pipeline connection, not a correctness bug in the code that was written. The PR is safe to merge; the list enrichment can be addressed in a follow-up.
Summary
swamp extension deprecateandswamp extension undeprecateCLI commands that mark/unmark an entire extension as deprecated in the registrysearch(indicator),info(details),pull(warning),outdated(status),update(status), andlist --check-updates(indicator)POST /deprecatewith{reason, supersededBy?},POST /undeprecatewith no body)Closes swamp-club#425
Test plan
deno checkpassesdeno lintcleandeno fmtcleandeno run compilesucceedsswamp extension deprecate @namespace/ext --reason "..." --superseded-by @other/extswamp extension undeprecate @namespace/ext🤖 Generated with Claude Code