fix(upgrade): clear error on tarball-extract installs + scoped-name registry lookup (#184)

[mellanon]

Summary

Fixes the two bugs in arc upgrade reported in #184:

1. Clear error on tarball-extract installs. arc upgrade <name> against a registry-installed package (no .git dir) used to fail with git pull failed: fatal: not a git repository. Now surfaces an actionable error pointing to the documented workaround + this issue.
2. --check correctly resolves scoped registry entries. arc upgrade <name> --check against an installed bare-name (e.g. soma) whose registry entry is scoped (@metafactory/soma) now reports the upgrade correctly instead of silently saying "All packages are up to date."

Scope

This PR fixes both bugs as reported in #184 but ships only the clear-error variant of Bug 1 — full in-place tarball upgrade still needs the install-orchestration code in src/cli.ts factored into a reusable function (the install pipeline is currently inlined). The follow-up is tracked on #184 itself.

Bug 2 is fully fixed at the registry-resolution layer (findRegistryEntry), so every caller benefits: arc info, arc upgrade, arc upgrade --check, future callers.

Approach

Bug 1 — src/commands/upgrade.ts

Before calling git pull, check if findGitRoot(installPath) returns null. If so, the install is a tarball extract and git pull is not the right primitive. Return a structured error citing:

• the install path (so the user can inspect it)
• the documented arc remove <name> && arc install @<scope>/<name> --yes workaround
• a pointer to arc upgrade: extracted-tarball install + version-check both broken (soma#127 reproducer) #184 for the in-place fix follow-up

Bug 2 — src/lib/registry.ts

findRegistryEntry now does a two-pass lookup:

• Pass 1 — exact case-insensitive name match (preserves existing behaviour).
• Pass 2 — only when the input is unscoped (no leading @), fall back to matching against the unscoped tail of any scoped entry. So findRegistryEntry(cfg, "soma") resolves to @metafactory/soma.

Exact-scope match always beats tail-only fallback. Multiple scoped entries with the same tail resolve to the first one found, in skills → agents → prompts → tools → components → rules order — same as the original lookup order.

Test plan

• New test: resolves scoped registry entry from bare installed-name (arc#184) — failed before, passes now.

• New test: returns actionable error for tarball-extract installs (no .git) (arc#184) — failed before, passes now.

• Full suite green: bun test → 957 pass / 0 fail.

• Type-check clean: bunx tsc --noEmit.

• Manual repro against the live install:

  $ bun src/cli.ts upgrade soma
  Cannot upgrade "soma" in place: install at .../metafactory__soma is a
  registry tarball-extract, not a git checkout. Run `arc remove soma &&
  arc install @<scope>/soma --yes` to upgrade. Tracked at
  https://github.com/the-metafactory/arc/issues/184 for the in-place fix.
  

What this does NOT include

• Full in-place tarball upgrade. Lifting the resolve → download → verify (checksum + ed25519 + sigstore) → extract → swap pipeline from src/cli.ts into a reusable function. Tracked as a follow-up note on arc upgrade: extracted-tarball install + version-check both broken (soma#127 reproducer) #184.
• Changes to arc install (the bug only affected arc upgrade).
• Changes to the registry signing path.

Closes the clear-error + check-comparison parts of #184. Tracking issue stays open for the in-place upgrade follow-up.

🤖 Generated with Claude Code

[mellanon]

Code Review — PR #185 (arc#184 upgrade fixes)

Verdict: APPROVE-WITH-NITS. Both bugs are fixed correctly, the deferral of the in-place tarball upgrade is appropriate and well-documented, and the two new tests are genuine reproducers. No blockers. One HIGH-severity correctness note worth a follow-up, plus a few NITs.

Reviewed: git diff origin/main HEAD over registry.ts, upgrade.ts, upgrade.test.ts. Ran bun test test/commands/upgrade.test.ts (16 pass) and test/unit/registry.test.ts (20 pass) — no regressions in existing findRegistryEntry coverage.

---

HIGH — findRegistryEntry pass-2 tail match is cross-scope; first-match-wins can mask the wrong scope (src/lib/registry.ts:116-127)

The pass-2 fallback resolves an unscoped lookup to a scoped entry from any scope. With @metafactory/soma and a hypothetical @evil/soma both in a registry, findRegistryEntry(cfg, "soma") returns whichever appears first in section/array order — silently, with no ambiguity signal.

Security assessment — not a code-execution dependency-confusion vector, but a real version-confusion one. I traced every live caller:

• findInAllSources → checkUpgrades / info — read-only version display.
• upgradePackage itself does not install from the resolved registry entry; it git pulls the already-installed checkout. A wrong-scope match cannot redirect an upgrade to attacker code.
• addFromRegistry (the one path that would copy entry.source into a catalog) currently has no production caller — dead-ish code, but a latent hazard if arc add is wired up later.

So today the blast radius is: arc upgrade --check / arc info could display a version/description from the wrong scope, and upgradeAll could decide a package "is upgradable" off a wrong-scope version. That is misleading, not exploitable — hence HIGH, not BLOCKER. Recommended follow-up (need not block this PR): when pass-2 finds more than one tail match across distinct scopes, either return null or prefer a configured-trusted scope, rather than first-wins. At minimum, log a warning. The current docstring claims first-match-wins is intentional; please make sure arc#184's follow-up captures this as a known sharp edge.

---

LOW — empty-name input falls through to a real lookup (src/lib/registry.ts:94-95)

findRegistryEntry(cfg, "") → lower = "", inputIsScoped = false. Pass 1 can't match (no entry has an empty name). Pass 2 computes tail and compares tail === "" — this matches any scoped entry whose name ends in / (e.g. a malformed @scope/). Extremely unlikely in practice, but an empty/whitespace name should arguably short-circuit to null at the top. Cheap guard.

LOW — findGitRoot false-positive when a tarball-extract lives under a git repo (src/commands/upgrade.ts:177)

findGitRoot walks up to 10 parents. If a registry tarball-extract install path happens to sit inside an unrelated git repo (e.g. someone set the install root inside ~/Developer/something), findGitRoot returns that ancestor and the new clear-error branch is skipped — git pull --ff-only then runs in the wrong repo. Pre-existing findGitRoot behaviour, not introduced here, and the default arc install layout (~/.config/...) is not under a repo — but the new code now depends on findGitRoot === null as a positive signal for "tarball extract", which is a slightly stronger contract than before. Worth a one-line comment acknowledging the assumption, or a tighter check (presence of a sibling marker the extract pipeline writes). Not blocking.

NIT — error message scope placeholder (src/commands/upgrade.ts:185)

arc install @<scope>/${name} asks the user to know the scope. Since the registry-install that produced the tarball recorded a scoped name somewhere, consider surfacing the actual scope if it's recoverable from the DB/manifest, so the workaround is copy-pasteable rather than templated.

NIT — test relies on tmpdir() not being inside a git repo (test/commands/upgrade.test.ts)

The tarball-extract test does rm -rf .git then expects findGitRoot to return null. This holds because createTestEnv uses mkdtemp under os.tmpdir() (/var/folders/... on macOS, /tmp on Linux) — not a git repo. Correct today; a one-line comment noting the dependency would harden it against a future CI that runs tests from inside a checkout-rooted tmp.

NIT — findRegistryEntry now matches the searchRegistry section-table style (good)

The refactor to a sections array removes the six near-duplicate loops and aligns with searchRegistry above it. Consistent and a genuine readability win.

---

What's good

• Bug 2 is fixed at the right layer (findRegistryEntry), so info / upgrade / --check all benefit uniformly.
• Pass-2 is correctly gated on !inputIsScoped — a scoped lookup that misses pass 1 does not fuzzy-match, which is the right call and prevents the worst cross-scope behaviour.
• Bug 1's clear-error path is structured (UpgradeResult with success:false + actionable error), not a throw — consistent with the rest of upgradePackage's error returns.
• Deferring the in-place tarball upgrade is correctly scoped and documented in both the PR body and the code comment; the test honestly asserts the clear-error contract (not toContain("not a git repository") + toMatch(/registry|tarball|remove.*install/i)) rather than overclaiming.
• Both new tests fail on origin/main and pass on the branch — genuine reproducers.

Net: ship it. Please carry the HIGH (cross-scope tail-match ambiguity) into the arc#184 follow-up so it isn't lost.

[mellanon]

Review findings addressed — all 6 resolved

Thanks for the review. Pushed 5f8a... (see latest commit) addressing every finding:

Severity	Finding	Resolution
HIGH	findRegistryEntry pass-2 was first-match-wins across scopes	Now collects all tail matches; resolves only when exactly one exists. Ambiguous (>1 scope, same name) → returns null so the caller reports an honest "not found" instead of guessing. Exact-scope lookup still resolves.
LOW	Empty-name input could match a malformed @scope/ entry	Added lower.trim() === "" guard at the top of findRegistryEntry.
LOW	findGitRoot walk-up assumption undocumented	Added a comment in upgradePackage noting that a tarball-extract nested inside an unrelated git repo would be misdetected — safe for the default reposDir layout.
NIT	Error message used @<scope>/ placeholder	Now parses the real scoped ref from skill.repo_url (@scope/name@version) so the suggested arc install is copy-pasteable. Git installs keep the placeholder.
NIT	Tarball test relied on tmpdir not being in a git repo	Documented the assumption with a comment.
NIT (positive)	sections refactor removes duplicate loops	Kept.

On the HIGH security assessment — agreed it's not code-execution dependency confusion (no caller installs from the resolved entry; upgradePackage git-pulls the existing checkout). But rather than carry it to the arc#184 follow-up, the ambiguous-scope case is cheap to neutralise here: returning null on ambiguity is strictly safer than first-match-wins and is now covered by a test.

Tests: +3 in registry.test.ts (bare→scoped resolution, ambiguous-scope→null, empty-string→null). Full suite 960 pass / 0 fail (one known temp-dir-race flake, green on re-run). eslint --quiet + tsc --noEmit clean.

Note: the Lint and Tests CI gates on this PR will still show red — that is pre-existing breakage on main (the last two main runs failed from prior NATS/JetStream merges: lint errors in jetstream.ts/json-response.ts/nats-broker.ts, test failures in bundle/publish suites). None of those files are touched by this PR. This PR's own changed files are lint-clean and test-green.