feat(hub): zero-infrastructure private-git-repo carrier (AD-1 first slice)#96
Merged
Conversation
…ct store Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lice) hub: git+ssh://… syncs through any private git repo the user can already push to — no bucket, no new credential plane. GitCarrierHub composes the proven R2Hub keying/semantics over a filesystem S3Client in a local clone, adding only the git transport: fetch/reset reads, one-commit-per-batch writes, push-ref CAS as the linearization point (non-fast-forward retries re-apply idempotent path-disjoint mutations), RFC3339Nano timestamp sidecars for object freshness, an orphan-squash + force-with-lease on hub compact to bound carrier history, and a marker that refuses non-hub repositories. Conformance, concurrency-race, compact-squash, and foreign-repo-refusal suites run against a hermetic local bare repo; a two-device e2e converges through git+file://. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a private git-repo “git carrier” hub backend, wires CLI hub selection to accept git URI forms, extends git push rejection handling, adds conformance and adversarial tests, and updates docs/specs plus an end-to-end sync script for the new backend. ChangesGit carrier hub implementation and wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant GitCarrierHub
participant fsObjectStore
participant GitRemote
CLI->>GitCarrierHub: selectBackendHub(remote, branch, workspaceID)
GitCarrierHub->>GitRemote: fetch + reset
GitCarrierHub->>GitCarrierHub: validate marker / workspace
CLI->>GitCarrierHub: Push(events)
GitCarrierHub->>fsObjectStore: write objects
GitCarrierHub->>GitRemote: commit + push
GitRemote-->>GitCarrierHub: non-fast-forward rejection
GitCarrierHub->>GitRemote: refetch + retry
GitCarrierHub-->>CLI: success
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/hub/gitcarrier.go (3)
151-184: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
lock()ignores context cancellation.
lock()takes nocontext.Contextand polls for up togitLockWait(2 min) regardless of the caller's ctx deadline/cancellation. EverywriteLoop/readRefresh/CompactEventsBelowcall passes actxfor the git-command execution but the lock-acquisition wait itself can't be cancelled early.♻️ Suggested fix: thread ctx into lock()
-func (g *GitCarrierHub) lock() (func(), error) { +func (g *GitCarrierHub) lock(ctx context.Context) (func(), error) { g.mu.Lock() if err := os.MkdirAll(filepath.Dir(g.lockPath), 0o700); err != nil { g.mu.Unlock() return nil, fmt.Errorf("create git hub cache dir: %w", err) } deadline := time.Now().Add(gitLockWait) for { f, err := os.OpenFile(g.lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) if err == nil { _ = f.Close() return func() { _ = os.Remove(g.lockPath) g.mu.Unlock() }, nil } if !os.IsExist(err) { g.mu.Unlock() return nil, fmt.Errorf("acquire git hub lock %s: %w", g.lockPath, err) } if info, serr := os.Stat(g.lockPath); serr == nil && time.Since(info.ModTime()) > gitLockStale { _ = os.Remove(g.lockPath) // abandoned by a crashed process continue } if time.Now().After(deadline) { g.mu.Unlock() return nil, fmt.Errorf("acquire git hub lock %s: timed out (another devstrap process is using this hub?)", g.lockPath) } + if ctx.Err() != nil { + g.mu.Unlock() + return nil, ctx.Err() + } g.sleep(50 * time.Millisecond) } }🤖 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/hub/gitcarrier.go` around lines 151 - 184, The lock acquisition in GitCarrierHub.lock ignores caller cancellation and can block for the full gitLockWait even when the operation context is done. Thread a context into lock() and use it in the wait loop so writeLoop, readRefresh, and CompactEventsBelow can abort early on ctx cancellation or deadline, while keeping the existing stale-lock cleanup and release behavior intact.
893-902: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate credential-redaction logic.
redactedRemotereimplements URL-credential stripping thatinternal/gitalready has (redactGitText/urlCredentials, unexported). Since this PR already exportsSafeBranchNamefrom an internal helper for exactly this kind of cross-package reuse, consider exporting a similar redaction helper frominternal/gitinstead of maintaining a second, simpler regex/index-based implementation here that can drift out of sync.🤖 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/hub/gitcarrier.go` around lines 893 - 902, The redactedRemote helper duplicates credential-stripping logic already present in internal/git, so replace this local implementation with a shared helper exported from internal/git. Update redactedRemote to delegate to the existing redaction path (or a new exported equivalent of redactGitText/urlCredentials) so all URL credential masking stays consistent and maintained in one place.
839-854: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: inconsistent lint suppression and blanket error handling in
PutObjectIfMatch.
os.ReadFile(path)here lacks the//nolint:gosecannotation that the equivalent read inGetObject(line 690) has, despite going through the samekeyPath-validated path. Separately,rerr != niltreats any read failure (including real I/O errors, not just "doesn't exist") identically to an ETag mismatch, which would mask a genuine disk/permission problem asErrPreconditionFailed.🤖 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/hub/gitcarrier.go` around lines 839 - 854, PutObjectIfMatch currently mixes all read failures into ErrPreconditionFailed and is missing the same gosec suppression used by GetObject. Update PutObjectIfMatch to keep the os.ReadFile(path) lint annotation consistent with keyPath-validated access, and change the current read handling so only a true ETag mismatch returns ErrPreconditionFailed while genuine read errors from os.ReadFile are surfaced directly. Use the existing GetObject and PutObjectIfMatch flow as the reference points when adjusting the error path.
🤖 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/cli/hub.go`:
- Around line 391-401: `parseGitCarrierURI` currently only blocks credentials
when a password is present, so `git+https://<token>@...` can still pass through
and get persisted as the remote. Update the userinfo check in
`parseGitCarrierURI` to reject any `u.User` when the scheme resolves to `https`,
while still allowing `git@`-style SSH URIs. Keep the existing scheme handling
around `realScheme` and the credential validation logic, but make the `https`
path fail whenever userinfo is present.
---
Nitpick comments:
In `@internal/hub/gitcarrier.go`:
- Around line 151-184: The lock acquisition in GitCarrierHub.lock ignores caller
cancellation and can block for the full gitLockWait even when the operation
context is done. Thread a context into lock() and use it in the wait loop so
writeLoop, readRefresh, and CompactEventsBelow can abort early on ctx
cancellation or deadline, while keeping the existing stale-lock cleanup and
release behavior intact.
- Around line 893-902: The redactedRemote helper duplicates credential-stripping
logic already present in internal/git, so replace this local implementation with
a shared helper exported from internal/git. Update redactedRemote to delegate to
the existing redaction path (or a new exported equivalent of
redactGitText/urlCredentials) so all URL credential masking stays consistent and
maintained in one place.
- Around line 839-854: PutObjectIfMatch currently mixes all read failures into
ErrPreconditionFailed and is missing the same gosec suppression used by
GetObject. Update PutObjectIfMatch to keep the os.ReadFile(path) lint annotation
consistent with keyPath-validated access, and change the current read handling
so only a true ETag mismatch returns ErrPreconditionFailed while genuine read
errors from os.ReadFile are surfaced directly. Use the existing GetObject and
PutObjectIfMatch flow as the reference points when adjusting the error path.
🪄 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: 8da519fb-d6a7-475a-8733-348b8a3e66c9
📒 Files selected for processing (19)
cmd/devstrap/testdata/script/sync_git_hub.txtarinternal/cli/hub.gointernal/cli/hub_git_test.gointernal/git/git.gointernal/hub/gitcarrier.gointernal/hub/gitcarrier_smoke_test.gointernal/hub/gitcarrier_test.gointernal/hub/r2_test.gospec/00_START_HERE.mdspec/01_ARCHITECTURE_DECISION.mdspec/02_PRODUCT_REQUIREMENTS.mdspec/03_SYSTEM_ARCHITECTURE.mdspec/13_CLI_DAEMON_API.mdspec/14_MVP_ROADMAP_AND_BACKLOG.mdspec/15_SECURITY_THREAT_MODEL.mdspec/16_TEST_PLAN.mdspec/17_REFERENCES.mdspec/18_WORK_LOG.mdspec/19_CLOUD_PROVISIONING_GUIDE.md
Reject any https userinfo in carrier URIs (tokens ride the username slot and the remote URL persists into .git/config) without echoing the URI; route dry-run migrate-events through the read path so it writes nothing; floor reported blob times at a per-clone observation index so a skewed writer clock can never age a live blob past another device's gc grace window, and clamp the sweep-lock time down to the same floor so a dead holder's future-dated clock cannot wedge the TTL break; heartbeat the cross-process clone lock so the stale-breaker only fires on dead holders. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reederey87
force-pushed
the
feat/ad1-git-carrier-hub
branch
from
ea2612c to
9b74e5d
Compare
CI runners have no init.defaultBranch, so the fixture's HEAD pointed at master while everything pushes to main — the tests' verification clones checked out an unborn HEAD and read an empty tree. The carrier itself is unaffected (it names refs/heads/<branch> explicitly, never remote HEAD). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hub/gitcarrier_adversarial_test.go`:
- Around line 135-151: The non-dry-run branch in
TestGitCarrierDryRunMigrateWritesNothing only checks for errors and does not
verify that MigrateLegacyEvents(ctx, false) leaves the carrier unchanged. Add a
post-call assertion using gitLsRemote, mirroring the dry-run check, so the test
confirms the remote remains empty after the real no-op migrate and catches
unintended marker or empty commits.
🪄 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: 8061b173-7a48-4cbb-9f3a-d403808eba24
📒 Files selected for processing (7)
internal/cli/hub.gointernal/cli/hub_git_test.gointernal/hub/gitcarrier.gointernal/hub/gitcarrier_adversarial_test.gointernal/hub/gitcarrier_smoke_test.gospec/15_SECURITY_THREAT_MODEL.mdspec/18_WORK_LOG.md
✅ Files skipped from review due to trivial changes (1)
- spec/18_WORK_LOG.md
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cli/hub_git_test.go
- internal/cli/hub.go
- spec/15_SECURITY_THREAT_MODEL.md
- internal/hub/gitcarrier.go
Comment on lines
+135
to
+151
| func TestGitCarrierDryRunMigrateWritesNothing(t *testing.T) { | ||
| ctx := context.Background() | ||
| remote := newBareCarrier(t) | ||
| h := newGitCarrierTestHub(t, remote, "dryrun") | ||
| if _, _, err := h.MigrateLegacyEvents(ctx, true); err != nil { | ||
| t.Fatalf("dry-run migrate: %v", err) | ||
| } | ||
| out := gitLsRemote(t, remote) | ||
| if strings.TrimSpace(out) != "" { | ||
| t.Fatalf("dry-run migrate wrote to the carrier: %q", out) | ||
| } | ||
| // The non-dry run against the same empty carrier is also a no-op commit | ||
| // (no legacy layout, no mutations) and must not fail. | ||
| if _, _, err := h.MigrateLegacyEvents(ctx, false); err != nil { | ||
| t.Fatalf("real migrate: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Real-migrate no-op is not actually verified.
The comment on line 146-147 claims the non-dry-run call is also a no-op ("no legacy layout, no mutations") but only the error is checked at line 148-150 — there's no follow-up gitLsRemote assertion confirming the remote stayed empty, unlike the dry-run check at lines 142-145. As written, a regression that causes the real migrate to push an empty/marker commit on a no-op batch would not be caught.
✅ Proposed fix to verify the real-run no-op as well
// The non-dry run against the same empty carrier is also a no-op commit
// (no legacy layout, no mutations) and must not fail.
if _, _, err := h.MigrateLegacyEvents(ctx, false); err != nil {
t.Fatalf("real migrate: %v", err)
}
+ out = gitLsRemote(t, remote)
+ if strings.TrimSpace(out) != "" {
+ t.Fatalf("no-op real migrate wrote to the carrier: %q", out)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestGitCarrierDryRunMigrateWritesNothing(t *testing.T) { | |
| ctx := context.Background() | |
| remote := newBareCarrier(t) | |
| h := newGitCarrierTestHub(t, remote, "dryrun") | |
| if _, _, err := h.MigrateLegacyEvents(ctx, true); err != nil { | |
| t.Fatalf("dry-run migrate: %v", err) | |
| } | |
| out := gitLsRemote(t, remote) | |
| if strings.TrimSpace(out) != "" { | |
| t.Fatalf("dry-run migrate wrote to the carrier: %q", out) | |
| } | |
| // The non-dry run against the same empty carrier is also a no-op commit | |
| // (no legacy layout, no mutations) and must not fail. | |
| if _, _, err := h.MigrateLegacyEvents(ctx, false); err != nil { | |
| t.Fatalf("real migrate: %v", err) | |
| } | |
| } | |
| func TestGitCarrierDryRunMigrateWritesNothing(t *testing.T) { | |
| ctx := context.Background() | |
| remote := newBareCarrier(t) | |
| h := newGitCarrierTestHub(t, remote, "dryrun") | |
| if _, _, err := h.MigrateLegacyEvents(ctx, true); err != nil { | |
| t.Fatalf("dry-run migrate: %v", err) | |
| } | |
| out := gitLsRemote(t, remote) | |
| if strings.TrimSpace(out) != "" { | |
| t.Fatalf("dry-run migrate wrote to the carrier: %q", out) | |
| } | |
| // The non-dry run against the same empty carrier is also a no-op commit | |
| // (no legacy layout, no mutations) and must not fail. | |
| if _, _, err := h.MigrateLegacyEvents(ctx, false); err != nil { | |
| t.Fatalf("real migrate: %v", err) | |
| } | |
| out = gitLsRemote(t, remote) | |
| if strings.TrimSpace(out) != "" { | |
| t.Fatalf("no-op real migrate wrote to the carrier: %q", out) | |
| } | |
| } |
🤖 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/hub/gitcarrier_adversarial_test.go` around lines 135 - 151, The
non-dry-run branch in TestGitCarrierDryRunMigrateWritesNothing only checks for
errors and does not verify that MigrateLegacyEvents(ctx, false) leaves the
carrier unchanged. Add a post-call assertion using gitLsRemote, mirroring the
dry-run check, so the test confirms the remote remains empty after the real
no-op migrate and catches unintended marker or empty commits.
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.
Summary
hub: git+ssh://git@host/you/devstrap-hub.git(alsogit+https://,git+file://for tests, scp-likegit@host:path.git, optional?branch=, defaultmain) syncs the workspace through any private git repository the user can already push to — no provisioned bucket, no new credential plane (existing ssh agent / git credential helpers; non-interactive, with embedded URI passwords rejected). This is the first slice of AD-1, the top first-run adoption friction named in spec/02/03/14/19.internal/hub/gitcarrier.go): instead of re-implementing the 24-method Hub contract,GitCarrierHubcomposes the provenR2Hubsemantics over a plain-filesystemS3Client(fsObjectStore) rooted in a local clone (~/.devstrap/hub-git/<hash>/repo), adding only the git transport. Reads fetch + hard-reset to the remote head; writes apply idempotent file mutations (every key content-addressed or(device,seq)-unique → devices never touch the same path, nogit mergeever runs), commit once per batch, push. The atomic push-ref CAS replaces S3 conditional PUT; a non-fast-forward rejection refetches and re-applies with capped backoff, andErrRetentionConflict/ErrSweepLockHeldre-evaluate against the race winner's state. Object freshness (gc grace, sweep TTL) rides RFC3339Nano timestamp sidecars under.devstrap-meta/times/(outside every listing prefix — commit times neither survive squashes nor register dedup re-puts).CompactEventsBelowdeletes cold events then rewrites the branch to a single parentless commit pushed--force-with-lease— the only operation that actually shrinks a git carrier. Adevstrap-hub.jsonmarker refuses non-hub repositories and foreign workspace ids.internal/git: newErrNonFastForwardclassification + exportedSafeBranchName.internal/cli/hub.go:selectBackendHub/hubConfiguredgit-carrier dispatch (parseGitCarrierURI), hub idgit:<workspace_id>.assertHubRoundTripgeneralized todssync.Huband run against a hermetic local bare repo, plus ack/retention-CAS/snapshot/sweep-lock contract mirrors, concurrent-push-both-land, one-CAS-winner + one-lock-holder races, compact-squash (rev-list --count == 1, stale clone recovers), foreign-repo refusal, env-gated real-remote conformance (DEVSTRAP_HUB_GIT_TEST_REMOTE), URI accept/reject matrix, and a two-devicesync_git_hub.txtare2e.hub initbootstrap, folder carrier).Validation
gofmtclean;golangci-lint run0 issues;spec-driftpass (20 specs, 19 changed files);go test -race ./...pass (one unrelatedinternal/gittimeout-test flake under heavy parallel load re-ran clean twice).git+file://carrier.🤖 Generated with Claude Code
Summary by CodeRabbit
devstrap synccan use a private Git repo (supports multiple URL forms and?branch=selection).