Feat/bootstrap official registry - #10
Merged
Merged
Conversation
Search and shorthand specs previously required manually registering a registry before they could work. Manager.Bootstrap registers the built-in official registry when registries.json does not yet exist, and never overwrites an existing, empty, or corrupt file. OfficialAlias and DefaultRegistryURL expose the built-in for reuse and tests. 💘 Generated with Crush Assisted-by: Crush:deepseek-v4-flash-free
ResolveShorthand now returns AliasNotRegisteredError carrying the missing alias, so callers can detect the failure structurally instead of parsing message text. The registry package stays transport-agnostic and leaves user-facing hints to the CLI. 💘 Generated with Crush Assisted-by: Crush:deepseek-v4-flash-free
Search and <alias>/<id> specs in new and add now bootstrap the official registry on first run, so the out-of-box experience needs no manual setup. The CLI enriches the typed alias error with a re-add hint for the built-in registry, while registry list remains a pure read that never bootstraps. 💘 Generated with Crush Assisted-by: Crush:deepseek-v4-flash-free
The search guide now explains that the official registry is registered automatically on first run and that removing it later is respected. 💘 Generated with Crush Assisted-by: Crush:deepseek-v4-flash-free
Contributor
📝 WalkthroughWalkthroughThe registry manager now installs the official registry on first use. The add, new, and search commands invoke this bootstrap. Shorthand resolution returns structured missing-alias errors, and CLI commands add guidance for restoring the official alias. ChangesOfficial registry bootstrap
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLICommand
participant RegistryManager
participant ShorthandResolver
User->>CLICommand: Run add, new, or search
CLICommand->>RegistryManager: Bootstrap official registry
RegistryManager-->>CLICommand: Return bootstrap status
CLICommand->>ShorthandResolver: Resolve shorthand
ShorthandResolver-->>CLICommand: Return result or AliasNotRegisteredError
CLICommand-->>User: Show result or re-add guidance
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/bootstrap.go`:
- Around line 17-24: Update the err != nil branch in maybeBootstrapOfficial to
emit a direct, action-oriented user warning without the “could not bootstrap
official registry:” category prefix; preserve the detailed err context only
through internal diagnostics if that mechanism already exists.
In `@docs/2.guide/4.search.md`:
- Around line 13-15: Update the additional-registry example following the
introductory text to use a distinct non-official alias, such as community,
instead of official; keep the command’s other arguments and surrounding
documentation unchanged.
In `@internal/registry/manager_test.go`:
- Around line 432-448: Update TestManager_BootstrapNoopWhenConfigured in
internal/registry/manager_test.go: call withFixtureOfficialURL(t) before
Bootstrap. In cmd/bootstrap_test.go lines 79-87, assign
registry.DefaultRegistryURL to a valid local writeMiniRegistry fixture and
restore the original value with t.Cleanup so both no-op tests avoid production
network access and expose unexpected bootstrap calls.
In `@internal/registry/manager.go`:
- Around line 365-375: Serialize the first-run initialization in
Manager.Bootstrap by acquiring a cross-process lock or atomic initialization
claim before the RegistriesPath existence check, and hold it through m.Add.
Recheck the path after acquiring the guard so only one process registers
OfficialAlias, while later callers return false without concurrent cloning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be37c2c4-d86a-4855-a2af-1c5a3ce77ef3
📒 Files selected for processing (11)
cmd/add.gocmd/bootstrap.gocmd/bootstrap_test.gocmd/new.gocmd/search.godocs/2.guide/4.search.mdinternal/registry/index_resolve_test.gointernal/registry/manager.gointernal/registry/manager_test.gointernal/registry/official.gointernal/registry/resolve.go
Comment on lines
+13
to
+15
| On first run, `spin search`, `spin new <alias>/<id>`, and `spin add <alias>/<id>` automatically register the built-in official registry (`official`), so search works out of the box. Removing it later (`spin registry remove official`) is respected: it is never re-added while `registries.json` exists. | ||
|
|
||
| To register additional registries: |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a non-official alias in the additional-registry example.
The new text introduces additional registries, but the following command uses official. After automatic bootstrap, that command can fail because the alias already exists. Change the example alias to a distinct value such as community.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/2.guide/4.search.md` around lines 13 - 15, Update the
additional-registry example following the introductory text to use a distinct
non-official alias, such as community, instead of official; keep the command’s
other arguments and surrounding documentation unchanged.
Comment on lines
+432
to
+448
| func TestManager_BootstrapNoopWhenConfigured(t *testing.T) { | ||
| mgr := newTestManager(t) | ||
| if err := os.WriteFile(mgr.RegistriesPath(), | ||
| []byte(`{"registries":[{"alias":"custom","source":"/tmp/x","kind":"local"}]}`), 0o644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| did, err := mgr.Bootstrap(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("Bootstrap: %v", err) | ||
| } | ||
| if did { | ||
| t.Error("Bootstrap reported did=true for an existing registries.json") | ||
| } | ||
| if _, ok := mgr.Get(context.Background(), "official"); ok { | ||
| t.Error("official registry must not be added when registries.json already exists") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use local registry fixtures in both no-op tests.
Both tests leave DefaultRegistryURL pointed at the production Git URL. This makes regressions network-dependent. In cmd/bootstrap_test.go, suppressed bootstrap errors can also hide an unexpected call.
internal/registry/manager_test.go#L432-L448: CallwithFixtureOfficialURL(t)beforeBootstrap.cmd/bootstrap_test.go#L79-L87: Setregistry.DefaultRegistryURLto a valid localwriteMiniRegistryfixture and restore it witht.Cleanup.
📍 Affects 2 files
internal/registry/manager_test.go#L432-L448(this comment)cmd/bootstrap_test.go#L79-L87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/registry/manager_test.go` around lines 432 - 448, Update
TestManager_BootstrapNoopWhenConfigured in internal/registry/manager_test.go:
call withFixtureOfficialURL(t) before Bootstrap. In cmd/bootstrap_test.go lines
79-87, assign registry.DefaultRegistryURL to a valid local writeMiniRegistry
fixture and restore the original value with t.Cleanup so both no-op tests avoid
production network access and expose unexpected bootstrap calls.
- Serialize first-run bootstrap with a cross-process lock. - Reclaim stale bootstrap locks after a timeout. - Simplify user-facing bootstrap warnings. - Add tests covering lock contention and stale lock recovery.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/registry/manager_test.go (1)
515-539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the stale-lock test by asserting the lock is released.
TestManager_BootstrapReclaimsStaleLockverifiesdid == trueand that the official registry is registered, but it does not assert thatlockPathno longer exists after the successful reclaim. Adding that check verifies the release path (Line 450releaseBootstrapLock) and the reclaim mechanism together, which is directly relevant to the false-reclaim risk discussed onmanager.goLines 402-445.♻️ Proposed test addition
if _, ok := mgr.Get(context.Background(), "official"); !ok { t.Error("official registry should be registered after reclaiming a stale lock") } + if _, err := os.Stat(lockPath); !os.IsNotExist(err) { + t.Errorf("bootstrap lock should be released after successful reclaim, stat err: %v", err) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/registry/manager_test.go` around lines 515 - 539, Extend TestManager_BootstrapReclaimsStaleLock after the successful Bootstrap call to assert that lockPath no longer exists, using an existence check that fails for unexpected filesystem errors. Keep the existing did and official-registry assertions, and verify releaseBootstrapLock completes as part of stale-lock reclamation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/registry/manager.go`:
- Around line 402-445: Replace the mtime/TTL-based stale-claim logic in
acquireBootstrapLock with a cross-platform advisory file-lock mechanism that
keeps ownership held for the entire bootstrap operation, including m.Add. Update
the caller and cleanup flow so the lock is explicitly released by its owner, and
remove the Stat/Rename/.stale reclaim behavior and bootstrapLockTTL dependency.
---
Nitpick comments:
In `@internal/registry/manager_test.go`:
- Around line 515-539: Extend TestManager_BootstrapReclaimsStaleLock after the
successful Bootstrap call to assert that lockPath no longer exists, using an
existence check that fails for unexpected filesystem errors. Keep the existing
did and official-registry assertions, and verify releaseBootstrapLock completes
as part of stale-lock reclamation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f58c807e-273e-40d2-aafb-6a7165cd5fad
📒 Files selected for processing (3)
cmd/bootstrap.gointernal/registry/manager.gointernal/registry/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/bootstrap.go
Comment on lines
+402
to
+445
| // bootstrapLockTTL is how long a .bootstrap.lock claim may stand | ||
| // before a later process treats it as stale. The lock only guards | ||
| // first-run registration; a process that crashes mid-bootstrap leaves | ||
| // a claim behind, and the TTL lets a later command reclaim it instead | ||
| // of deferring forever. It is generous so a slow clone is never | ||
| // misread as stale. | ||
| const bootstrapLockTTL = 2 * time.Minute | ||
|
|
||
| // acquireBootstrapLock atomically claims lockPath with O_CREATE|O_EXCL | ||
| // and reports whether this process won the claim. A fresh claim held | ||
| // by another process yields false (the caller defers to it). A stale | ||
| // claim, left by a crashed process, is moved aside via an atomic | ||
| // rename and re-claimed, so a crash cannot block first-run bootstrap | ||
| // permanently. | ||
| func acquireBootstrapLock(lockPath string) (bool, error) { | ||
| for { | ||
| lock, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL, 0o600) | ||
| if err == nil { | ||
| _ = lock.Close() | ||
| return true, nil | ||
| } | ||
| if !os.IsExist(err) { | ||
| return false, err | ||
| } | ||
| info, err := os.Stat(lockPath) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| continue // released between OpenFile and Stat | ||
| } | ||
| return false, err | ||
| } | ||
| if time.Since(info.ModTime()) < bootstrapLockTTL { | ||
| return false, nil | ||
| } | ||
| // Stale claim: move it aside atomically, then retry. If a | ||
| // concurrent process wins the rename first, the retry sees its | ||
| // fresh claim and defers. | ||
| stalePath := lockPath + ".stale" | ||
| if err := os.Rename(lockPath, stalePath); err != nil && !os.IsNotExist(err) { | ||
| return false, err | ||
| } | ||
| _ = os.Remove(stalePath) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for an existing cross-process file-locking dependency.
grep -nE 'gofrs/flock|golang.org/x/sys' go.mod go.sum 2>/dev/null
cat go.mod 2>/dev/null | head -40Repository: N1xev/spin
Length of output: 1975
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- manager.go outline ---'
ast-grep outline internal/registry/manager.go --lang go 2>/dev/null || true
printf '%s\n' '--- manager.go lines 350-470 ---'
cat -n internal/registry/manager.go | sed -n '350,470p'
printf '%s\n' '--- lock and bootstrap references ---'
rg -n -C 3 'acquireBootstrapLock|bootstrapLockTTL|bootstrap\.lock|RegistriesPath|func \(m \*?Manager\) Add|func .*Add' internal/registry
printf '%s\n' '--- relevant tests ---'
rg -n -C 4 'bootstrap|concurr|lock|RegistriesPath|Add\(' --glob '*_test.go' internal/registryRepository: N1xev/spin
Length of output: 44535
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Add and clone paths ---'
cat -n internal/registry/manager.go | sed -n '130,295p'
printf '%s\n' '--- registry writes and atomic-write helper ---'
cat -n internal/registry/manager.go | sed -n '540,590p'
rg -n -C 5 'func atomicWriteJSON|func \(m Manager\) writeRegistries|func .*upsert|os\.MkdirAll|RegistriesDir' internal/registry
printf '%s\n' '--- platform and dependency context ---'
rg -n 'GOOS|windows|unix|x/sys|flock|lock' go.mod go.sum internal --glob '*.go' --glob '*.mod' --glob '*.sum'
printf '%s\n' '--- standalone rename race primitive ---'
python3 - <<'PY'
import os
import tempfile
with tempfile.TemporaryDirectory() as d:
lock = os.path.join(d, ".bootstrap.lock")
stale = lock + ".stale"
with open(lock, "w") as f:
f.write("live-claim")
with open(stale, "w") as f:
f.write("old-stale")
os.rename(lock, stale)
print("lock_exists_after_rename:", os.path.exists(lock))
print("stale_contents_after_rename:", open(stale).read())
os.remove(stale)
print("live_claim_removed_by_cleanup:", not os.path.exists(stale))
PYRepository: N1xev/spin
Length of output: 26559
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- copyDir implementation ---'
cat -n internal/registry/manager.go | sed -n '1,130p'
rg -n -C 5 'func copyDir|copyDir\(' internal/registry
printf '%s\n' '--- atomic JSON write implementation ---'
cat -n internal/registry/write.go | sed -n '1,100p'
printf '%s\n' '--- standalone directory-install behavior ---'
python3 - <<'PY'
import os
import tempfile
with tempfile.TemporaryDirectory() as d:
dest = os.path.join(d, "official")
first = os.path.join(d, "official.clone-first")
second = os.path.join(d, "official.clone-second")
os.mkdir(first)
open(os.path.join(first, "registry.toml"), "w").close()
os.mkdir(second)
open(os.path.join(second, "registry.toml"), "w").close()
os.rename(first, dest)
try:
os.rename(second, dest)
except OSError as e:
print("second_install:", type(e).__name__, e.errno)
else:
print("second_install: succeeded")
print("installed_contents:", sorted(os.listdir(dest)))
print("second_temp_remains:", os.path.exists(second))
PYRepository: N1xev/spin
Length of output: 10067
Replace the mtime-based bootstrap lock with an ownership-safe lock.
acquireBootstrapLock never refreshes the mtime during m.Add. A clone that exceeds two minutes can therefore be reclaimed by another process. Both processes can then run m.Add; for Git sources, this causes duplicate clones and one installation to fail because os.Rename(tmp, dest) cannot replace the non-empty destination.
The Stat/Rename gap can also remove a fresh claim after the original holder releases it. Use a cross-platform advisory file lock instead of the TTL heuristic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/registry/manager.go` around lines 402 - 445, Replace the
mtime/TTL-based stale-claim logic in acquireBootstrapLock with a cross-platform
advisory file-lock mechanism that keeps ownership held for the entire bootstrap
operation, including m.Add. Update the caller and cleanup flow so the lock is
explicitly released by its owner, and remove the Stat/Rename/.stale reclaim
behavior and bootstrapLockTTL dependency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auto-bootstrap the official registry on first use when no registry configuration exists. Respect explicit user removal by never recreating official once registries.json has been created. Keep registry resolution transport-agnostic by returning typed errors and enrich CLI messages only in the command layer.
Summary by CodeRabbit