Skip to content

SD-10171: Track install profile per asset#151

Open
inesp wants to merge 3 commits into
mainfrom
SD-10171-profile-info
Open

SD-10171: Track install profile per asset#151
inesp wants to merge 3 commits into
mainfrom
SD-10171-profile-info

Conversation

@inesp
Copy link
Copy Markdown
Contributor

@inesp inesp commented Jun 1, 2026

  • Add Profile field to the install tracker
  • Profile is set on install, repair and up-to-date runs
  • Make vault list --installed read the tracker, filter by profile <- old profile-less entries are counted towards every profile, just like they were until now
  • Filter uninstall to the current profile (empty = current)

~/.cache/sx/installed.json:
image

- Add Profile field to the install tracker
- Stamp profile on install, repair and up-to-date runs
- Make vault list --installed read the tracker, filter by profile
@inesp inesp requested review from detkin, mrdon and mzgajner as code owners June 1, 2026 14:00
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 1, 2026

SD-10171

@claude

This comment has been minimized.

- Filter uninstall to the current profile (empty = current)
@claude

This comment has been minimized.

@inesp
Copy link
Copy Markdown
Contributor Author

inesp commented Jun 1, 2026

Addressed the feedback — fixed 7 items. Skipping 3:

  • Multi-active profiles hide non-primary installs (vault.go:138): by design; aggregate view is out of scope here.
  • Untagged legacy entries leak across profiles (vault_installed.go): intentional tradeoff — keeps pre-profile installs visible/removable until re-stamped.
  • loadConfigAndVault dead code (install_context.go:217): not dead, still used by team.go:373.

@claude
Copy link
Copy Markdown

claude Bot commented Jun 1, 2026

Claude finished @inesp's task in 4m 23s —— View job


Code Review

The follow-up commit cleanly addresses the orphaned doc comment and improves discoverability with per-row repo labels. A few new issues introduced by the second commit warrant a look — none blocking.

[LOW] Two `// Step 5` comments after inserting the new profile filter step

Location: internal/commands/uninstall.go:188 and internal/commands/uninstall.go:204

Issue: The new "Step 4: Filter to the current profile" was inserted between the old Step 3 and Step 4, but the subsequent step numbers weren't shifted. The existing scope-filter block was renumbered to // Step 5 (line 188), but the next block (client filter, line 204) is also labeled // Step 5. The remaining steps (6, 7, 8…) are now off-by-one from their actual order in the function. Cosmetic, but the numbered comments only earn their keep if they stay consistent.

// Step 5: Filter plan by scope (without --all, only current repo's assets)
plan = filterUninstallPlanByScope(plan, opts.All)
...
// Step 5: Filter plan by client flag if provided   ← duplicate
if opts.ClientsFlag != "" {

Suggestion: Renumber the trailing blocks (client filter → Step 6, display → Step 7, etc.) or drop the numeric labels entirely. Fix this →

[MEDIUM] `vault list --installed --json` now emits duplicates when one asset spans multiple scopes

Location: internal/commands/vault_installed.go:39-56 and internal/commands/vault.go:540-575

Issue: The old path read lf.Assets, where one lockfile entry held a multi-scope Scopes array, so JSON output emitted one {name, version} per asset. The new path creates one lockfile.Asset per tracker entry, and each tracker entry is a single scope. If a user installs skill-X in repo A and repo B under the same profile, the JSON now contains two identical {"name":"skill-X","version":"1.0.0"} rows in skills[] (no scope field to distinguish them). Tools or scripts deduping by name will work the same; tools counting array length will double-count. This wasn't called out in the prior review because that review was looking at the text printer (where (N scopes) made the issue obvious) — the JSON path has no such hint. Mentioning since the prior reviewer suggested keeping the wide view but exposing scope per row; text now does, JSON doesn't.

Suggestion: Either (a) include the scope in the JSON output ({"name": "...", "version": "...", "repository": "...", "path": "..."}) so duplicates are distinguishable, or (b) group tracker entries by (name, version, type) before adapting, collapsing scopes back into a single lockfile.Asset (closer to the prior behavior). Worth picking one before the JSON shape ossifies. Fix this →

[LOW] `config.Load()` failure silently skips the profile filter — broader uninstall than the rest of the code suggests

Location: internal/commands/uninstall.go:179-186

Issue: When config.Load() returns an error, the code falls through with the unfiltered plan, so uninstall then proceeds to remove every tracker entry that survives the scope/client filters — including assets stamped with another profile. The inline comment ("Only a config load error skips the filter, to avoid wrongly dropping everything") explains the intent, but in practice this is the only failure mode where a multi-profile user could accidentally wipe another profile's installs from a transiently broken config (corrupt JSON, unreadable file). The other branches of this PR are very careful to leave other profiles alone; this one quietly isn't.

Suggestion: At minimum, log a warning (via out.printfErr or the styled output) when skipping the filter so the user can correlate the noisier uninstall with the config-load failure. Stronger fix: fail closed — treat config-load errors as fatal here rather than as "uninstall everything", since the user can still recover by fixing the config. Either way, surface the situation rather than masking it. Fix this →

[LOW] `install.go` profile-fallback comment misdescribes when the lookup is empty

Location: internal/commands/install.go:1080-1089

Issue: The comment says "The lookup is \"\" when the winner is the default (unnamed) profile or the asset isn't in the merge map". But assetOrigin[name] is set to pl.ProfileName, and pl.ProfileName comes from cfg.ProfileName, which GetActiveProfileName always populates with at least DefaultProfileName = "default". So the default profile is named "default" in assetOrigin, not empty. The fallback fires for the "asset not in merge map" / "nil assetOrigin from handleNothingToInstall" cases, not for the default profile. Code is correct; the comment will mislead the next reader troubleshooting why the profile field ends up empty.

Suggestion: Drop the "(default/unnamed) profile" phrasing and keep just the second half: "empty when the asset is missing from the merge map (e.g., on a repair run with nil origin) — preserve any profile already on the tracker entry rather than blanking it." Fix this →

· [Branch `SD-10171-profile-info`](https://github.com/sleuth-io/sx/tree/SD-10171-profile-info)

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.

1 participant