From 4b71848fddcc76359b8ee927386121e68868ef43 Mon Sep 17 00:00:00 2001 From: "eric.wien" Date: Tue, 9 Jun 2026 19:48:08 +0200 Subject: [PATCH] fix: address dual-reviewer code review findings Cross-PDS / resolution: - resolvePDS now uses indigo's identity directory (handles did:plc, did:web, and handle resolution) instead of hardcoding plc.directory and bsky.social - ResolveRepo/ListRepos page through all records and fall back to the public cross-PDS path; repo clone's knot lookup reuses ResolveRepo - ListOwnPulls/ListOwnIssues paginate via Paginate Correctness / robustness: - repo delete now rolls back (restores the PDS record) if the knot delete fails, instead of orphaning the bare repo - non-DPoP knot/PDS requests use an http.Client with a 30s timeout - pr create resolves the repo's default branch from the knot instead of hardcoding "main" - patch blob uploaded as application/gzip (matches the appview) instead of text/x-patch - AddAccount preserves IsDefault on re-login (no longer demotes default) - merge populates AuthorName/CommitMessage on MergeInput - record rkeys minted from a single monotonic TID clock - SSH remote regex accepts repo names containing dots Security: - sanitize the OAuth state before using it as an auth-request filename (prevents path traversal via the localhost callback) Build / release: - fix .goreleaser.yml ldflags module path (knot.eric.wien -> tangled.org) so release binaries carry real version metadata - factory passes context.Background() to ResumeSession instead of nil Cleanup / dedup: - shared internal/browser.Open and cmdutil.ParseOwnerRepo replace per-file copies; drop dead CloneSSH/CloneHTTPS/WebURL and the parseRecordValue wrapper; bytes.NewReader avoids a body copy Tests: regression coverage for ParseOwnerRepo, account-default preservation, auth-request path traversal, and dotted SSH repo names. Co-Authored-By: Claude Opus 4.8 --- .goreleaser.yml | 6 +- README.md | 2 +- api/client.go | 131 ++++++++++++++++++---------- api/issue.go | 22 +++-- api/pull.go | 40 +++++---- api/repo.go | 25 ++++-- api/tid.go | 18 ++++ git/remote.go | 14 +-- git/remote_test.go | 21 ++--- internal/authflow/flow.go | 20 +---- internal/browser/browser.go | 24 +++++ internal/config/auth_config.go | 20 ++++- internal/config/auth_config_test.go | 48 ++++++++++ internal/tngcmd/cmd.go | 8 +- pkg/cmd/auth/login/login.go | 4 +- pkg/cmd/browse/browse.go | 20 +---- pkg/cmd/factory/default.go | 5 +- pkg/cmd/issue/comment/comment.go | 4 +- pkg/cmd/issue/list/list.go | 14 +-- pkg/cmd/pr/comment/comment.go | 4 +- pkg/cmd/pr/create/create.go | 53 +++++++---- pkg/cmd/pr/list/list.go | 14 +-- pkg/cmd/pr/merge/merge.go | 23 +++-- pkg/cmd/repo/clone/clone.go | 51 ++--------- pkg/cmd/repo/clone/clone_test.go | 35 -------- pkg/cmd/repo/delete/delete.go | 17 +--- pkg/cmd/repo/list/list.go | 6 +- pkg/cmd/repo/view/view.go | 17 +--- pkg/cmdutil/args.go | 15 ++++ pkg/cmdutil/args_test.go | 39 +++++++++ pkg/cmdutil/factory.go | 2 +- pkg/iostreams/iostreams.go | 8 +- 32 files changed, 409 insertions(+), 321 deletions(-) create mode 100644 api/tid.go create mode 100644 internal/browser/browser.go create mode 100644 pkg/cmdutil/args_test.go diff --git a/.goreleaser.yml b/.goreleaser.yml index f3634f9..b4c6311 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -7,9 +7,9 @@ builds: goarch: [amd64, arm64] ldflags: &ldflags - -s -w - - -X knot.eric.wien/tng-cli/internal/build.Version={{.Version}} - - -X knot.eric.wien/tng-cli/internal/build.Date={{.Date}} - - -X knot.eric.wien/tng-cli/internal/build.Commit={{.ShortCommit}} + - -X tangled.org/eric.wien/tng-cli/internal/build.Version={{.Version}} + - -X tangled.org/eric.wien/tng-cli/internal/build.Date={{.Date}} + - -X tangled.org/eric.wien/tng-cli/internal/build.Commit={{.ShortCommit}} main: ./cmd/tng binary: tng diff --git a/README.md b/README.md index 1639651..f6fca1d 100644 --- a/README.md +++ b/README.md @@ -149,7 +149,7 @@ tng pr comment --body "..." # Comment on a PR Flags for `pr create`: - `-t, --title` -- PR title (required) - `-B, --body` -- PR description -- `-b, --base` -- Target branch (default: `main`) +- `-b, --base` -- Target branch (default: the repo's default branch, falling back to `main`) - `-H, --head` -- Source branch (default: current branch) Flags for `pr list`: diff --git a/api/client.go b/api/client.go index 18b297d..0d607d0 100644 --- a/api/client.go +++ b/api/client.go @@ -1,6 +1,7 @@ package api import ( + "bytes" "context" "encoding/json" "fmt" @@ -13,9 +14,14 @@ import ( "github.com/bluesky-social/indigo/atproto/atclient" "github.com/bluesky-social/indigo/atproto/auth/oauth" + "github.com/bluesky-social/indigo/atproto/identity" "github.com/bluesky-social/indigo/atproto/syntax" ) +// defaultHTTPTimeout bounds knot/PDS reads made outside the DPoP client so a +// slow or unresponsive server can't hang the CLI indefinitely. +const defaultHTTPTimeout = 30 * time.Second + // Client wraps an authenticated session for making Tangled API calls. // // Tangled uses AT Protocol service auth: the CLI authenticates to the @@ -27,6 +33,8 @@ type Client struct { pdsClient *atclient.APIClient // authenticated against the PDS (DPoP) baseURL string // default appview URL (unused for XRPC, kept for web URLs) did syntax.DID + dir identity.Directory // resolves handles/DIDs to PDS endpoints + http *http.Client // for non-DPoP knot/PDS requests (bounded timeout) } // NewClient creates a new API client from an OAuth session. @@ -40,6 +48,8 @@ func NewClient(session *oauth.ClientSession, baseURL string) *Client { pdsClient: session.APIClient(), baseURL: baseURL, did: did, + dir: identity.DefaultDirectory(), + http: &http.Client{Timeout: defaultHTTPTimeout}, } } @@ -106,7 +116,7 @@ func (c *Client) PostToKnot(ctx context.Context, knotURL, nsid string, input int if err != nil { return fmt.Errorf("failed to marshal request body: %w", err) } - body = strings.NewReader(string(data)) + body = bytes.NewReader(data) } req, err := http.NewRequestWithContext(ctx, "POST", u, body) @@ -243,51 +253,30 @@ func (c *Client) ListPublicRecords(ctx context.Context, repo, collection, cursor } // resolvePDS resolves a handle or DID to the PDS service endpoint URL and DID. +// It uses indigo's identity directory, which handles handle resolution +// (DNS/well-known) and all DID methods — did:plc via plc.directory and did:web +// via the domain's /.well-known/did.json — rather than a hardcoded resolver. func (c *Client) resolvePDS(ctx context.Context, handleOrDID string) (string, string, error) { - did := handleOrDID - - // If it's a handle, resolve to DID first. - if !strings.HasPrefix(handleOrDID, "did:") { - u := "https://bsky.social/xrpc/com.atproto.identity.resolveHandle?handle=" + url.QueryEscape(handleOrDID) - req, err := http.NewRequestWithContext(ctx, "GET", u, nil) - if err != nil { - return "", "", err - } - req.Header.Set("Accept", "application/json") - - var out struct { - DID string `json:"did"` - } - if err := c.doPlainRequest(req, "com.atproto.identity.resolveHandle", &out); err != nil { - return "", "", fmt.Errorf("failed to resolve handle %s: %w", handleOrDID, err) - } - did = out.DID + dir := c.dir + if dir == nil { + dir = identity.DefaultDirectory() } - // Resolve DID document to find PDS endpoint. - u := "https://plc.directory/" + did - req, err := http.NewRequestWithContext(ctx, "GET", u, nil) + id, err := syntax.ParseAtIdentifier(handleOrDID) if err != nil { - return "", "", err + return "", "", fmt.Errorf("invalid handle or DID %q: %w", handleOrDID, err) } - req.Header.Set("Accept", "application/json") - var doc struct { - Service []struct { - ID string `json:"id"` - ServiceEndpoint string `json:"serviceEndpoint"` - } `json:"service"` - } - if err := c.doPlainRequest(req, "plc.directory", &doc); err != nil { - return "", "", fmt.Errorf("failed to resolve DID document for %s: %w", did, err) + ident, err := dir.Lookup(ctx, id) + if err != nil { + return "", "", fmt.Errorf("failed to resolve identity for %s: %w", handleOrDID, err) } - for _, svc := range doc.Service { - if svc.ID == "#atproto_pds" { - return svc.ServiceEndpoint, did, nil - } + pds := ident.PDSEndpoint() + if pds == "" { + return "", "", fmt.Errorf("no PDS endpoint in DID document for %s", handleOrDID) } - return "", "", fmt.Errorf("no PDS service found in DID document for %s", did) + return pds, ident.DID.String(), nil } // RecordList is the response from com.atproto.repo.listRecords. @@ -306,7 +295,11 @@ type RecordEntry struct { // doPlainRequest executes an HTTP request without DPoP (used for knot requests // with Bearer service auth or unauthenticated reads). func (c *Client) doPlainRequest(req *http.Request, nsid string, out interface{}) error { - resp, err := http.DefaultClient.Do(req) + httpClient := c.http + if httpClient == nil { + httpClient = http.DefaultClient + } + resp, err := httpClient.Do(req) if err != nil { return fmt.Errorf("request failed: %w", err) } @@ -352,20 +345,66 @@ type ResolvedRepo struct { Rkey string } +// listRecordsAll walks every record of a collection for an owner, following +// cursors. It tries the authenticated user's PDS first and, if that PDS doesn't +// host the owner, falls back to the public cross-PDS path for the whole walk. +// limit <= 0 fetches all records. +func (c *Client) listRecordsAll(ctx context.Context, owner, collection string, limit int) ([]RecordEntry, error) { + var all []RecordEntry + cursor := "" + usePublic := false + + for { + pageLimit := 100 + if limit > 0 { + remaining := limit - len(all) + if remaining <= 0 { + break + } + if remaining < pageLimit { + pageLimit = remaining + } + } + + var ( + rl *RecordList + err error + ) + if usePublic { + rl, err = c.ListPublicRecords(ctx, owner, collection, cursor, pageLimit) + } else { + rl, err = c.ListRecords(ctx, owner, collection, cursor, pageLimit) + if err != nil && cursor == "" { + // The user's PDS may not host this owner — try the public API. + usePublic = true + rl, err = c.ListPublicRecords(ctx, owner, collection, cursor, pageLimit) + } + } + if err != nil { + return nil, err + } + + all = append(all, rl.Records...) + if rl.Cursor == "" || len(rl.Records) == 0 { + break + } + cursor = rl.Cursor + } + + return all, nil +} + // ResolveRepo looks up a repository by owner and name from AT Protocol records. // It first tries the authenticated user's PDS, then falls back to the public API -// for cross-PDS lookups (e.g. resolving repos owned by users on a different PDS). +// for cross-PDS lookups (e.g. resolving repos owned by users on a different PDS), +// paging through all of the owner's repo records. func (c *Client) ResolveRepo(ctx context.Context, owner, name string) (*ResolvedRepo, error) { - records, err := c.ListRecords(ctx, owner, RepoRecordNSID, "", 100) + records, err := c.listRecordsAll(ctx, owner, RepoRecordNSID, 0) if err != nil { - // The user's PDS may not host this owner — try the public API. - records, err = c.ListPublicRecords(ctx, owner, RepoRecordNSID, "", 100) - if err != nil { - return nil, fmt.Errorf("failed to look up repos for %s: %w", owner, err) - } + return nil, fmt.Errorf("failed to look up repos for %s: %w", owner, err) } - for _, rec := range records.Records { + for _, rec := range records { var r struct { Name string `json:"name"` Knot string `json:"knot"` diff --git a/api/issue.go b/api/issue.go index da03b76..d927c78 100644 --- a/api/issue.go +++ b/api/issue.go @@ -5,8 +5,6 @@ import ( "encoding/json" "fmt" "time" - - "github.com/bluesky-social/indigo/atproto/syntax" ) const ( @@ -24,7 +22,7 @@ const ( // issueRecord is written to the user's PDS when creating an issue. type issueRecord struct { Type string `json:"$type"` - Repo string `json:"repo"` // AT URI of the target repo + Repo string `json:"repo"` // AT URI of the target repo Title string `json:"title"` Body *string `json:"body,omitempty"` CreatedAt string `json:"createdAt"` @@ -61,7 +59,7 @@ type IssueEntry struct { // repoURI is the AT URI of the target repository (at://did/sh.tangled.repo/rkey). // Returns the rkey of the created issue. func (c *Client) CreateIssue(ctx context.Context, repoURI, title, body string) (string, error) { - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() var bodyPtr *string if body != "" { @@ -86,13 +84,19 @@ func (c *Client) CreateIssue(ctx context.Context, repoURI, title, body string) ( // ListOwnIssues lists issues created by the current user. If repoURI is // non-empty, only issues targeting that repo are returned. func (c *Client) ListOwnIssues(ctx context.Context, repoURI string) ([]IssueEntry, error) { - records, err := c.ListRecords(ctx, c.did.String(), IssueRecordNSID, "", 100) + records, err := Paginate(ctx, func(cursor string) ([]RecordEntry, string, error) { + rl, err := c.ListRecords(ctx, c.did.String(), IssueRecordNSID, cursor, 100) + if err != nil { + return nil, "", err + } + return rl.Records, rl.Cursor, nil + }) if err != nil { return nil, err } var issues []IssueEntry - for _, rec := range records.Records { + for _, rec := range records { var r struct { Repo string `json:"repo"` Title string `json:"title"` @@ -161,7 +165,7 @@ func (c *Client) GetIssueRecord(ctx context.Context, authorDID, rkey string) (*I // CloseIssue writes an issue state record to close the issue. // issueURI is the AT URI of the issue (at://did/sh.tangled.repo.issue/rkey). func (c *Client) CloseIssue(ctx context.Context, issueURI string) error { - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() record := &issueStateRecord{ Type: IssueStateNSID, @@ -175,7 +179,7 @@ func (c *Client) CloseIssue(ctx context.Context, issueURI string) error { // ReopenIssue writes an issue state record to reopen the issue. func (c *Client) ReopenIssue(ctx context.Context, issueURI string) error { - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() record := &issueStateRecord{ Type: IssueStateNSID, @@ -190,7 +194,7 @@ func (c *Client) ReopenIssue(ctx context.Context, issueURI string) error { // CreateIssueComment adds a comment to an issue. // issueURI is the AT URI of the issue. func (c *Client) CreateIssueComment(ctx context.Context, issueURI, body string) error { - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() record := &issueCommentRecord{ Type: IssueCommentNSID, diff --git a/api/pull.go b/api/pull.go index 47a9a1e..1e9ad28 100644 --- a/api/pull.go +++ b/api/pull.go @@ -31,13 +31,13 @@ const ( // pullRecord is written to the user's PDS when creating a PR. type pullRecord struct { - Type string `json:"$type"` - Title string `json:"title"` - Body *string `json:"body,omitempty"` - Target *pullTarget `json:"target"` - Source *pullSource `json:"source,omitempty"` - PatchBlob json.RawMessage `json:"patchBlob"` - CreatedAt string `json:"createdAt"` + Type string `json:"$type"` + Title string `json:"title"` + Body *string `json:"body,omitempty"` + Target *pullTarget `json:"target"` + Source *pullSource `json:"source,omitempty"` + PatchBlob json.RawMessage `json:"patchBlob"` + CreatedAt string `json:"createdAt"` } type pullTarget struct { @@ -47,7 +47,7 @@ type pullTarget struct { type pullSource struct { Branch string `json:"branch"` - Sha string `json:"sha"` // 40-char commit hash + Sha string `json:"sha"` // 40-char commit hash Repo *string `json:"repo,omitempty"` // AT URI (for forks, same as target for same-repo) } @@ -137,13 +137,15 @@ func (c *Client) CreatePull(ctx context.Context, params *CreatePullParams) (stri return "", fmt.Errorf("failed to finalize patch compression: %w", err) } - // Upload blob to PDS - blobRef, err := c.UploadBlob(ctx, &compressed, "text/x-patch") + // Upload blob to PDS as application/gzip — the blob bytes are gzip and the + // decompressed payload is a text-based git format-patch. This matches the + // Tangled appview, which uploads the patch blob with the same content type. + blobRef, err := c.UploadBlob(ctx, &compressed, "application/gzip") if err != nil { return "", err } - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() var bodyPtr *string if params.Body != "" { @@ -178,13 +180,19 @@ func (c *Client) CreatePull(ctx context.Context, params *CreatePullParams) (stri // ListOwnPulls lists pull requests created by the current user. If repoURI // is non-empty, only PRs targeting that repo are returned. func (c *Client) ListOwnPulls(ctx context.Context, repoURI string) ([]PullEntry, error) { - records, err := c.ListRecords(ctx, c.did.String(), PullRecordNSID, "", 100) + records, err := Paginate(ctx, func(cursor string) ([]RecordEntry, string, error) { + rl, err := c.ListRecords(ctx, c.did.String(), PullRecordNSID, cursor, 100) + if err != nil { + return nil, "", err + } + return rl.Records, rl.Cursor, nil + }) if err != nil { return nil, err } var pulls []PullEntry - for _, rec := range records.Records { + for _, rec := range records { entry, err := parsePullRecord(rec, c.did.String()) if err != nil { continue @@ -248,7 +256,7 @@ func parsePullRecord(rec RecordEntry, authorDID string) (*PullEntry, error) { // ClosePull writes a PR status record to close the PR. func (c *Client) ClosePull(ctx context.Context, pullURI string) error { - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() record := &pullStatusRecord{ Type: PullStatusNSID, @@ -262,7 +270,7 @@ func (c *Client) ClosePull(ctx context.Context, pullURI string) error { // ReopenPull writes a PR status record to reopen the PR. func (c *Client) ReopenPull(ctx context.Context, pullURI string) error { - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() record := &pullStatusRecord{ Type: PullStatusNSID, @@ -276,7 +284,7 @@ func (c *Client) ReopenPull(ctx context.Context, pullURI string) error { // CreatePullComment adds a comment to a pull request. func (c *Client) CreatePullComment(ctx context.Context, pullURI, body string) error { - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() record := &pullCommentRecord{ Type: PullCommentNSID, diff --git a/api/repo.go b/api/repo.go index 6651a0e..ee10ea9 100644 --- a/api/repo.go +++ b/api/repo.go @@ -2,12 +2,11 @@ package api import ( "context" + "encoding/json" "fmt" "net/url" "strconv" "time" - - "github.com/bluesky-social/indigo/atproto/syntax" ) const ( @@ -119,7 +118,7 @@ func (c *Client) CreateRepo(ctx context.Context, params *CreateRepoParams) error knotHost := extractHost(params.KnotURL) // Generate TID for the record key - rkey := syntax.NewTIDClock(0).Next().String() + rkey := newTID() // Step 1: Create AT Protocol record on PDS var desc *string @@ -161,8 +160,11 @@ func (c *Client) CreateRepo(ctx context.Context, params *CreateRepoParams) error } // DeleteRepo deletes a repository from a knot and removes the PDS record. -// The PDS record must be deleted first — the knot verifies it's gone before proceeding. -func (c *Client) DeleteRepo(ctx context.Context, knotURL string, did, name, rkey string) error { +// The PDS record must be deleted first — the knot verifies it's gone before +// proceeding. If the knot deletion then fails, the PDS record is restored from +// the supplied value so the repo stays resolvable and the delete can be retried +// (rather than leaving an orphaned bare repo on the knot). +func (c *Client) DeleteRepo(ctx context.Context, knotURL string, did, name, rkey string, record json.RawMessage) error { // Step 1: Delete the PDS record first (knot checks it's gone) if err := c.DeleteRecord(ctx, RepoRecordNSID, rkey); err != nil { return fmt.Errorf("failed to delete PDS record: %w", err) @@ -180,6 +182,13 @@ func (c *Client) DeleteRepo(ctx context.Context, knotURL string, did, name, rkey } if err := c.PostToKnot(ctx, knotURL, "sh.tangled.repo.delete", &input, nil); err != nil { + // Roll back: restore the PDS record so the repo remains resolvable. + if len(record) > 0 { + var raw map[string]interface{} + if jsonErr := json.Unmarshal(record, &raw); jsonErr == nil { + _, _ = c.PutRecord(ctx, RepoRecordNSID, rkey, raw) + } + } return err } @@ -243,8 +252,10 @@ func (c *Client) SetDefaultBranch(ctx context.Context, knotURL string, input *Se } // ListRepos lists repositories for a given owner by reading AT Protocol records. -func (c *Client) ListRepos(ctx context.Context, owner, cursor string, limit int) (*RecordList, error) { - return c.ListRecords(ctx, owner, RepoRecordNSID, cursor, limit) +// It pages through results (up to limit; limit <= 0 means all) and falls back to +// the public cross-PDS path when the owner isn't hosted on the user's own PDS. +func (c *Client) ListRepos(ctx context.Context, owner string, limit int) ([]RecordEntry, error) { + return c.listRecordsAll(ctx, owner, RepoRecordNSID, limit) } // GetLanguages fetches language statistics for a repository from a knot. diff --git a/api/tid.go b/api/tid.go new file mode 100644 index 0000000..a1618fc --- /dev/null +++ b/api/tid.go @@ -0,0 +1,18 @@ +package api + +import ( + "math/rand/v2" + + "github.com/bluesky-social/indigo/atproto/syntax" +) + +// sharedTIDClock mints record keys for the lifetime of the process. A single +// clock guarantees strictly increasing (monotonic) TIDs, so two records created +// in the same microsecond can't collide; the random clock id reduces the chance +// of collisions across concurrent processes. +var sharedTIDClock = syntax.NewTIDClock(uint(rand.IntN(1 << 10))) + +// newTID returns a fresh, monotonic TID string for use as a record rkey. +func newTID() string { + return sharedTIDClock.Next().String() +} diff --git a/git/remote.go b/git/remote.go index 11bd064..da49039 100644 --- a/git/remote.go +++ b/git/remote.go @@ -22,22 +22,10 @@ func (r TangledRepo) FullName() string { return r.Owner + "/" + r.Name } -func (r TangledRepo) CloneSSH() string { - return "git@tangled.sh:" + r.Owner + "/" + r.Name -} - -func (r TangledRepo) CloneHTTPS() string { - return "https://tangled.sh/" + r.Owner + "/" + r.Name -} - -func (r TangledRepo) WebURL() string { - return "https://tangled.sh/" + r.Owner + "/" + r.Name -} - var ( // git@:handle/repo-name(.git)? // Matches tangled.sh and any knot host (knot.*, *.example.com, etc.) - sshPattern = regexp.MustCompile(`^git@([^:]+):([^/]+)/([^/.]+?)(?:\.git)?$`) + sshPattern = regexp.MustCompile(`^git@([^:]+):([^/]+)/([^/]+?)(?:\.git)?$`) // https:///handle/repo-name(.git)? httpsPattern = regexp.MustCompile(`^https?://([^/]+)/([^/]+)/([^/]+?)(?:\.git)?$`) ) diff --git a/git/remote_test.go b/git/remote_test.go index b49932e..ba27f16 100644 --- a/git/remote_test.go +++ b/git/remote_test.go @@ -50,6 +50,16 @@ func TestParseTangledURL(t *testing.T) { input: "https://knot.example.com/alice.bsky.social/my-project", want: &TangledRepo{Owner: "alice.bsky.social", Name: "my-project"}, }, + { + name: "ssh repo name with dots", + input: "git@tangled.sh:alice.bsky.social/my.project", + want: &TangledRepo{Owner: "alice.bsky.social", Name: "my.project"}, + }, + { + name: "ssh repo name with dots and .git", + input: "git@tangled.sh:alice.bsky.social/my.project.git", + want: &TangledRepo{Owner: "alice.bsky.social", Name: "my.project"}, + }, { name: "empty", input: "", @@ -77,21 +87,12 @@ func TestParseTangledURL(t *testing.T) { } } -func TestTangledRepo_URLs(t *testing.T) { +func TestTangledRepo_FullName(t *testing.T) { repo := TangledRepo{Owner: "alice.bsky.social", Name: "my-project"} if got := repo.FullName(); got != "alice.bsky.social/my-project" { t.Errorf("FullName() = %q", got) } - if got := repo.CloneSSH(); got != "git@tangled.sh:alice.bsky.social/my-project" { - t.Errorf("CloneSSH() = %q", got) - } - if got := repo.CloneHTTPS(); got != "https://tangled.sh/alice.bsky.social/my-project" { - t.Errorf("CloneHTTPS() = %q", got) - } - if got := repo.WebURL(); got != "https://tangled.sh/alice.bsky.social/my-project" { - t.Errorf("WebURL() = %q", got) - } } func TestParseRemotes(t *testing.T) { diff --git a/internal/authflow/flow.go b/internal/authflow/flow.go index 07ed4a3..5b4b493 100644 --- a/internal/authflow/flow.go +++ b/internal/authflow/flow.go @@ -5,12 +5,11 @@ import ( "fmt" "net" "net/http" - "os/exec" - "runtime" "time" "github.com/bluesky-social/indigo/atproto/auth/oauth" "github.com/bluesky-social/indigo/atproto/syntax" + "tangled.org/eric.wien/tng-cli/internal/browser" "tangled.org/eric.wien/tng-cli/internal/build" "tangled.org/eric.wien/tng-cli/internal/config" ) @@ -94,7 +93,7 @@ func Login(ctx context.Context, identifier string) (*LoginResult, error) { }() // Open browser - if err := openBrowser(redirectURL); err != nil { + if err := browser.Open(redirectURL); err != nil { fmt.Printf("Could not open browser automatically.\nOpen this URL in your browser:\n\n %s\n\n", redirectURL) } @@ -217,18 +216,3 @@ func resolveHandle(ctx context.Context, app *oauth.ClientApp, did syntax.DID) st } return ident.Handle.String() } - -func openBrowser(url string) error { - var cmd *exec.Cmd - switch runtime.GOOS { - case "darwin": - cmd = exec.Command("open", url) - case "linux": - cmd = exec.Command("xdg-open", url) - case "windows": - cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", url) - default: - return fmt.Errorf("unsupported platform") - } - return cmd.Start() -} diff --git a/internal/browser/browser.go b/internal/browser/browser.go new file mode 100644 index 0000000..e562c45 --- /dev/null +++ b/internal/browser/browser.go @@ -0,0 +1,24 @@ +// Package browser opens URLs in the user's default web browser. +package browser + +import ( + "fmt" + "os/exec" + "runtime" +) + +// Open launches the user's default browser pointed at url. +func Open(url string) error { + var cmd *exec.Cmd + switch runtime.GOOS { + case "darwin": + cmd = exec.Command("open", url) + case "linux": + cmd = exec.Command("xdg-open", url) + case "windows": + cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", url) + default: + return fmt.Errorf("unsupported platform") + } + return cmd.Start() +} diff --git a/internal/config/auth_config.go b/internal/config/auth_config.go index 264483e..2a83016 100644 --- a/internal/config/auth_config.go +++ b/internal/config/auth_config.go @@ -80,6 +80,9 @@ func AddAccount(info AccountInfo) error { found := false for i := range accounts { if accounts[i].DID == info.DID { + // Preserve default status across re-login (callers pass IsDefault + // unset), so re-authenticating doesn't silently demote the account. + info.IsDefault = accounts[i].IsDefault || info.IsDefault accounts[i] = info found = true break @@ -129,8 +132,23 @@ func SessionFilePath(did string) string { } // AuthRequestFilePath returns the path for an in-flight auth request by state. +// The OAuth state is attacker-influenceable via the localhost callback, so it is +// sanitized before being used as a filename to prevent path traversal. func AuthRequestFilePath(state string) string { - return filepath.Join(AuthRequestsDir(), state+".json") + return filepath.Join(AuthRequestsDir(), sanitizeFilename(state)+".json") +} + +// sanitizeFilename reduces an arbitrary string to a safe single path component, +// replacing anything outside [A-Za-z0-9_-] (including '/', '.', and separators). +func sanitizeFilename(s string) string { + return strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-', r == '_': + return r + default: + return '_' + } + }, s) } // ValidateToken checks if a TNG_TOKEN env var is set (for CI/script usage). diff --git a/internal/config/auth_config_test.go b/internal/config/auth_config_test.go index cbe70d8..8236d58 100644 --- a/internal/config/auth_config_test.go +++ b/internal/config/auth_config_test.go @@ -1,6 +1,7 @@ package config import ( + "path/filepath" "testing" ) @@ -160,3 +161,50 @@ func TestAddAccount_Update(t *testing.T) { t.Errorf("handle = %q, want 'alice-new'", accounts[0].Handle) } } + +// TestAddAccount_PreservesDefaultOnReLogin guards against re-login silently +// demoting the default account (callers pass IsDefault unset). +func TestAddAccount_PreservesDefaultOnReLogin(t *testing.T) { + t.Setenv("TNG_CONFIG_DIR", t.TempDir()) + + // bob first (becomes default), then alice. + if err := AddAccount(AccountInfo{DID: "did:plc:bbb", Handle: "bob", SessionID: "s1"}); err != nil { + t.Fatal(err) + } + if err := AddAccount(AccountInfo{DID: "did:plc:aaa", Handle: "alice", SessionID: "s2"}); err != nil { + t.Fatal(err) + } + + // Make alice the explicit default. + accounts, _ := LoadAccounts() + for i := range accounts { + accounts[i].IsDefault = accounts[i].DID == "did:plc:aaa" + } + if err := SaveAccounts(accounts); err != nil { + t.Fatal(err) + } + + // Re-login as alice (IsDefault unset, as the login flow does). + if err := AddAccount(AccountInfo{DID: "did:plc:aaa", Handle: "alice", SessionID: "s3"}); err != nil { + t.Fatal(err) + } + + active, _ := ActiveAccount() + if active == nil || active.DID != "did:plc:aaa" { + t.Fatalf("active account = %+v, want alice to remain default after re-login", active) + } +} + +// TestAuthRequestFilePath_NoTraversal guards against path traversal via the +// attacker-influenceable OAuth state. +func TestAuthRequestFilePath_NoTraversal(t *testing.T) { + t.Setenv("TNG_CONFIG_DIR", "/tmp/tng-test") + + dir := AuthRequestsDir() + for _, state := range []string{"../../etc/passwd", "..", "a/b/c", "normal-state_123"} { + got := AuthRequestFilePath(state) + if filepath.Dir(got) != dir { + t.Errorf("AuthRequestFilePath(%q) = %q escaped %q", state, got, dir) + } + } +} diff --git a/internal/tngcmd/cmd.go b/internal/tngcmd/cmd.go index f7766b5..a9c8636 100644 --- a/internal/tngcmd/cmd.go +++ b/internal/tngcmd/cmd.go @@ -9,10 +9,10 @@ import ( ) const ( - exitOK = 0 - exitError = 1 - exitCancel = 2 - exitAuth = 4 + exitOK = 0 + exitError = 1 + exitCancel = 2 + exitAuth = 4 ) func Main() int { diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 10f4c04..4023e8a 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -10,8 +10,8 @@ import ( ) type Options struct { - IO *iostreams.IOStreams - Handle string + IO *iostreams.IOStreams + Handle string } func NewCmdLogin(f *cmdutil.Factory) *cobra.Command { diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index 66cbf33..1ac3090 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -2,11 +2,10 @@ package browse import ( "fmt" - "os/exec" - "runtime" "github.com/spf13/cobra" gitpkg "tangled.org/eric.wien/tng-cli/git" + "tangled.org/eric.wien/tng-cli/internal/browser" "tangled.org/eric.wien/tng-cli/pkg/cmdutil" "tangled.org/eric.wien/tng-cli/pkg/iostreams" ) @@ -76,20 +75,5 @@ func browseRun(opts *Options, path string) error { } fmt.Fprintf(opts.IO.ErrOut, "Opening %s in browser...\n", url) - return openBrowser(url) -} - -func openBrowser(url string) error { - var cmd *exec.Cmd - switch runtime.GOOS { - case "darwin": - cmd = exec.Command("open", url) - case "linux": - cmd = exec.Command("xdg-open", url) - case "windows": - cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", url) - default: - return fmt.Errorf("unsupported platform") - } - return cmd.Start() + return browser.Open(url) } diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index d57bb34..2044b80 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -1,6 +1,7 @@ package factory import ( + "context" "fmt" "sync" @@ -19,7 +20,7 @@ func New() *cmdutil.Factory { gc := gitpkg.NewClient() f := &cmdutil.Factory{ - IOStreams: ios, + IOStreams: ios, GitClient: gc, } @@ -46,7 +47,7 @@ func New() *cmdutil.Factory { return } - sess, err := authflow.ResumeSession(nil) // TODO: pass context + sess, err := authflow.ResumeSession(context.Background()) if err != nil { clientErr = &cmdutil.AuthError{Err: err} return diff --git a/pkg/cmd/issue/comment/comment.go b/pkg/cmd/issue/comment/comment.go index e8950fe..c24ea4f 100644 --- a/pkg/cmd/issue/comment/comment.go +++ b/pkg/cmd/issue/comment/comment.go @@ -24,8 +24,8 @@ func NewCmdComment(f *cmdutil.Factory) *cobra.Command { } cmd := &cobra.Command{ - Use: "comment ", - Short: "Comment on an issue", + Use: "comment ", + Short: "Comment on an issue", Example: ` tng issue comment 3jxn7q7bklr2r --body "Thanks for reporting."`, Args: cmdutil.ExactArgs(1, "issue rkey is required"), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/pkg/cmd/issue/list/list.go b/pkg/cmd/issue/list/list.go index f25a883..4132659 100644 --- a/pkg/cmd/issue/list/list.go +++ b/pkg/cmd/issue/list/list.go @@ -76,9 +76,8 @@ func listRun(cmd *cobra.Command, opts *Options) error { // Optionally resolve to AT URI for filtering var repoURI string if repoRef != "" { - parts := splitRepoRef(repoRef) - if len(parts) == 2 { - if resolved, err := client.ResolveRepo(cmd.Context(), parts[0], parts[1]); err == nil { + if owner, name, err := cmdutil.ParseOwnerRepo(repoRef); err == nil { + if resolved, err := client.ResolveRepo(cmd.Context(), owner, name); err == nil { repoURI = resolved.URI } } @@ -107,12 +106,3 @@ func listRun(cmd *cobra.Command, opts *Options) error { return nil } - -func splitRepoRef(ref string) []string { - for i := range ref { - if ref[i] == '/' { - return []string{ref[:i], ref[i+1:]} - } - } - return []string{ref} -} diff --git a/pkg/cmd/pr/comment/comment.go b/pkg/cmd/pr/comment/comment.go index 51cabb9..1630af7 100644 --- a/pkg/cmd/pr/comment/comment.go +++ b/pkg/cmd/pr/comment/comment.go @@ -24,8 +24,8 @@ func NewCmdComment(f *cmdutil.Factory) *cobra.Command { } cmd := &cobra.Command{ - Use: "comment ", - Short: "Comment on a pull request", + Use: "comment ", + Short: "Comment on a pull request", Example: ` tng pr comment 3jxn7q7bklr2r --body "Looks good!"`, Args: cmdutil.ExactArgs(1, "pull request rkey is required"), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 07bde9d..095c72d 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -1,6 +1,7 @@ package create import ( + "context" "fmt" "github.com/spf13/cobra" @@ -48,7 +49,7 @@ as an AT Protocol record, targeting the repository detected from git remotes.`, cmd.Flags().StringVarP(&opts.Title, "title", "t", "", "Title of the pull request (required)") cmd.Flags().StringVarP(&opts.Body, "body", "B", "", "Body of the pull request") - cmd.Flags().StringVarP(&opts.Base, "base", "b", "", "Target branch (defaults to main)") + cmd.Flags().StringVarP(&opts.Base, "base", "b", "", "Target branch (defaults to the repo's default branch)") cmd.Flags().StringVarP(&opts.Head, "head", "H", "", "Source branch (defaults to current branch)") cmd.MarkFlagRequired("title") @@ -58,19 +59,35 @@ as an AT Protocol record, targeting the repository detected from git remotes.`, func createRun(cmd *cobra.Command, opts *Options) error { cs := iostreams.NewColorScheme(opts.IO.ColorEnabled()) + // Resolve target repo + repo, err := opts.BaseRepo() + if err != nil { + return err + } + + client, err := opts.ApiClient() + if err != nil { + return err + } + + resolved, err := client.ResolveRepo(cmd.Context(), repo.Owner, repo.Name) + if err != nil { + return fmt.Errorf("failed to resolve repository: %w", err) + } + // Resolve source branch head := opts.Head if head == "" { - var err error head, err = opts.Branch() if err != nil { return fmt.Errorf("could not determine current branch: %w", err) } } + // Resolve target branch: explicit flag, else the repo's default branch. base := opts.Base if base == "" { - base = "main" + base = defaultBranch(cmd.Context(), client, resolved) } // Get HEAD SHA @@ -88,22 +105,6 @@ func createRun(cmd *cobra.Command, opts *Options) error { return fmt.Errorf("no changes between %s and HEAD", base) } - // Resolve target repo - repo, err := opts.BaseRepo() - if err != nil { - return err - } - - client, err := opts.ApiClient() - if err != nil { - return err - } - - resolved, err := client.ResolveRepo(cmd.Context(), repo.Owner, repo.Name) - if err != nil { - return fmt.Errorf("failed to resolve repository: %w", err) - } - // Create the PR params := &api.CreatePullParams{ Title: opts.Title, @@ -126,3 +127,17 @@ func createRun(cmd *cobra.Command, opts *Options) error { return nil } + +// defaultBranch returns the repository's default branch as reported by its knot, +// falling back to "main" when it can't be determined. +func defaultBranch(ctx context.Context, client *api.Client, resolved *api.ResolvedRepo) string { + knotRepoRef := resolved.OwnerDID + "/" + resolved.Name + if br, err := client.GetBranches(ctx, resolved.KnotURL, knotRepoRef, "", 100); err == nil { + for _, b := range br.Branches { + if b.IsDefault { + return b.Name + } + } + } + return "main" +} diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index 6da2c0b..4ee5d0a 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -74,9 +74,8 @@ func listRun(cmd *cobra.Command, opts *Options) error { var repoURI string if repoRef != "" { - parts := splitRepoRef(repoRef) - if len(parts) == 2 { - if resolved, err := client.ResolveRepo(cmd.Context(), parts[0], parts[1]); err == nil { + if owner, name, err := cmdutil.ParseOwnerRepo(repoRef); err == nil { + if resolved, err := client.ResolveRepo(cmd.Context(), owner, name); err == nil { repoURI = resolved.URI } } @@ -110,12 +109,3 @@ func listRun(cmd *cobra.Command, opts *Options) error { return nil } - -func splitRepoRef(ref string) []string { - for i := range ref { - if ref[i] == '/' { - return []string{ref[:i], ref[i+1:]} - } - } - return []string{ref} -} diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 7674920..0827bca 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -7,6 +7,7 @@ import ( "github.com/spf13/cobra" "tangled.org/eric.wien/tng-cli/api" gitpkg "tangled.org/eric.wien/tng-cli/git" + "tangled.org/eric.wien/tng-cli/internal/config" "tangled.org/eric.wien/tng-cli/pkg/cmdutil" "tangled.org/eric.wien/tng-cli/pkg/iostreams" ) @@ -85,16 +86,24 @@ func mergeRun(cmd *cobra.Command, opts *Options) error { Name string `json:"name"` Knot string `json:"knot"` } - if err := parseRecordValue(targetRec.Value, &repoInfo); err != nil { + if err := json.Unmarshal(targetRec.Value, &repoInfo); err != nil { return fmt.Errorf("failed to parse repository record: %w", err) } + // Identify the merge commit's author from the active account. + authorName := client.DID().String() + if acct, _ := config.ActiveAccount(); acct != nil && acct.Handle != "" { + authorName = acct.Handle + } + knotURL := "https://" + repoInfo.Knot input := &api.MergeInput{ - DID: targetDID, - Name: repoInfo.Name, - Branch: pr.TargetRef, - Patch: patch, + DID: targetDID, + Name: repoInfo.Name, + Branch: pr.TargetRef, + Patch: patch, + AuthorName: authorName, + CommitMessage: fmt.Sprintf("Merge pull request: %s", pr.Title), } if err := client.MergePull(cmd.Context(), knotURL, input); err != nil { @@ -104,7 +113,3 @@ func mergeRun(cmd *cobra.Command, opts *Options) error { fmt.Fprintf(opts.IO.ErrOut, "%s Merged pull request %s\n", cs.SuccessIcon(), opts.Rkey) return nil } - -func parseRecordValue(data []byte, v interface{}) error { - return json.Unmarshal(data, v) -} diff --git a/pkg/cmd/repo/clone/clone.go b/pkg/cmd/repo/clone/clone.go index 95be369..f628823 100644 --- a/pkg/cmd/repo/clone/clone.go +++ b/pkg/cmd/repo/clone/clone.go @@ -1,23 +1,20 @@ package clone import ( - "encoding/json" "fmt" "strings" "github.com/spf13/cobra" "tangled.org/eric.wien/tng-cli/api" - "tangled.org/eric.wien/tng-cli/internal/config" + gitpkg "tangled.org/eric.wien/tng-cli/git" "tangled.org/eric.wien/tng-cli/pkg/cmdutil" "tangled.org/eric.wien/tng-cli/pkg/iostreams" - gitpkg "tangled.org/eric.wien/tng-cli/git" ) type Options struct { IO *iostreams.IOStreams GitClient *gitpkg.Client ApiClient func() (*api.Client, error) - Config func() (*config.Config, error) Repo string Dir string Knot string @@ -28,7 +25,6 @@ func NewCmdClone(f *cmdutil.Factory) *cobra.Command { IO: f.IOStreams, GitClient: f.GitClient, ApiClient: f.ApiClient, - Config: f.Config, } cmd := &cobra.Command{ @@ -67,7 +63,7 @@ func cloneRun(cmd *cobra.Command, opts *Options) error { } // Expect owner/repo format - owner, repoName, err := parseOwnerRepo(repo) + owner, repoName, err := cmdutil.ParseOwnerRepo(repo) if err != nil { return err } @@ -88,43 +84,19 @@ func cloneRun(cmd *cobra.Command, opts *Options) error { return doClone(opts, cloneURL, cs) } -// resolveKnot looks up the knot hostname for a repo by reading the owner's -// AT Protocol records. +// resolveKnot looks up the knot hostname for a repo by resolving the owner's +// AT Protocol records (handling cross-PDS owners and pagination via ResolveRepo). func resolveKnot(cmd *cobra.Command, opts *Options, owner, repoName string) (string, error) { client, err := opts.ApiClient() if err != nil { return "", fmt.Errorf("authentication required to resolve knot (or use --knot): %w", err) } - // If owner matches the current user's handle, use their DID for the lookup - ownerID := owner - account, _ := config.ActiveAccount() - if account != nil && account.Handle == owner { - ownerID = account.DID - } - - records, err := client.ListRepos(cmd.Context(), ownerID, "", 100) + resolved, err := client.ResolveRepo(cmd.Context(), owner, repoName) if err != nil { - return "", fmt.Errorf("failed to look up repos for %s: %w", owner, err) - } - - for _, rec := range records.Records { - var r struct { - Name string `json:"name"` - Knot string `json:"knot"` - } - if err := json.Unmarshal(rec.Value, &r); err != nil { - continue - } - if r.Name == repoName { - if r.Knot == "" { - return "", fmt.Errorf("repo %s/%s has no knot assigned", owner, repoName) - } - return r.Knot, nil - } + return "", err } - - return "", fmt.Errorf("repo %s/%s not found in AT Protocol records", owner, repoName) + return resolved.Knot, nil } func doClone(opts *Options, cloneURL string, cs *iostreams.ColorScheme) error { @@ -144,15 +116,6 @@ func doClone(opts *Options, cloneURL string, cs *iostreams.ColorScheme) error { return nil } -// parseOwnerRepo splits "owner/repo" into its parts. -func parseOwnerRepo(ref string) (string, string, error) { - parts := strings.SplitN(ref, "/", 2) - if len(parts) != 2 || parts[0] == "" || parts[1] == "" { - return "", "", fmt.Errorf("expected owner/repo format, got %q", ref) - } - return parts[0], parts[1], nil -} - // isGitURL returns true if the string looks like a full git URL. func isGitURL(s string) bool { return strings.HasPrefix(s, "git@") || diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index 4415d17..d7293b0 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -4,41 +4,6 @@ import ( "testing" ) -func TestParseOwnerRepo(t *testing.T) { - tests := []struct { - input string - wantOwner string - wantName string - wantErr bool - }{ - {"alice.bsky.social/my-project", "alice.bsky.social", "my-project", false}, - {"did:plc:abc123/repo-name", "did:plc:abc123", "repo-name", false}, - {"just-a-name", "", "", true}, - {"/missing-owner", "", "", true}, - {"missing-name/", "", "", true}, - {"", "", "", true}, - } - - for _, tt := range tests { - t.Run(tt.input, func(t *testing.T) { - owner, name, err := parseOwnerRepo(tt.input) - if tt.wantErr { - if err == nil { - t.Errorf("parseOwnerRepo(%q) expected error", tt.input) - } - return - } - if err != nil { - t.Fatalf("parseOwnerRepo(%q) unexpected error: %v", tt.input, err) - } - if owner != tt.wantOwner || name != tt.wantName { - t.Errorf("parseOwnerRepo(%q) = (%q, %q), want (%q, %q)", - tt.input, owner, name, tt.wantOwner, tt.wantName) - } - }) - } -} - func TestIsGitURL(t *testing.T) { tests := []struct { input string diff --git a/pkg/cmd/repo/delete/delete.go b/pkg/cmd/repo/delete/delete.go index d0c2239..001a948 100644 --- a/pkg/cmd/repo/delete/delete.go +++ b/pkg/cmd/repo/delete/delete.go @@ -46,7 +46,7 @@ This action is irreversible. You must confirm with --yes unless running interact func deleteRun(cmd *cobra.Command, opts *Options) error { cs := iostreams.NewColorScheme(opts.IO.ColorEnabled()) - owner, name, err := parseOwnerRepo(opts.Repo) + owner, name, err := cmdutil.ParseOwnerRepo(opts.Repo) if err != nil { return err } @@ -92,7 +92,7 @@ func deleteRun(cmd *cobra.Command, opts *Options) error { return fmt.Errorf("failed to parse repo record: %w", err) } - if err := client.DeleteRepo(cmd.Context(), resolved.KnotURL, resolved.OwnerDID, repoVal.Name, resolved.Rkey); err != nil { + if err := client.DeleteRepo(cmd.Context(), resolved.KnotURL, resolved.OwnerDID, repoVal.Name, resolved.Rkey, rec.Value); err != nil { return fmt.Errorf("failed to delete repository: %w", err) } @@ -101,16 +101,3 @@ func deleteRun(cmd *cobra.Command, opts *Options) error { return nil } - -func parseOwnerRepo(ref string) (string, string, error) { - for i := range ref { - if ref[i] == '/' { - owner, name := ref[:i], ref[i+1:] - if owner == "" || name == "" { - break - } - return owner, name, nil - } - } - return "", "", fmt.Errorf("expected owner/name format, got %q", ref) -} diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index 24c496a..8d7c55f 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -58,17 +58,17 @@ func listRun(cmd *cobra.Command, opts *Options) error { owner = client.DID().String() } - records, err := client.ListRepos(cmd.Context(), owner, "", opts.Limit) + records, err := client.ListRepos(cmd.Context(), owner, opts.Limit) if err != nil { return fmt.Errorf("failed to list repositories: %w", err) } - if len(records.Records) == 0 { + if len(records) == 0 { fmt.Fprintln(opts.IO.ErrOut, "No repositories found.") return nil } - for _, rec := range records.Records { + for _, rec := range records { var repo struct { Name string `json:"name"` Knot string `json:"knot"` diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index 3487492..d7f44b4 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -52,11 +52,11 @@ func viewRun(cmd *cobra.Command, opts *Options) error { // Resolve repo var owner, name string if opts.Repo != "" { - parts := splitRepoRef(opts.Repo) - if len(parts) != 2 { - return fmt.Errorf("expected owner/name format, got %q", opts.Repo) + var err error + owner, name, err = cmdutil.ParseOwnerRepo(opts.Repo) + if err != nil { + return err } - owner, name = parts[0], parts[1] } else { repo, err := opts.BaseRepo() if err != nil { @@ -130,12 +130,3 @@ func viewRun(cmd *cobra.Command, opts *Options) error { return nil } - -func splitRepoRef(ref string) []string { - for i := range ref { - if ref[i] == '/' { - return []string{ref[:i], ref[i+1:]} - } - } - return []string{ref} -} diff --git a/pkg/cmdutil/args.go b/pkg/cmdutil/args.go index 002e6e7..c8bf5c5 100644 --- a/pkg/cmdutil/args.go +++ b/pkg/cmdutil/args.go @@ -33,3 +33,18 @@ func MinimumArgs(n int, msg string) cobra.PositionalArgs { return nil } } + +// ParseOwnerRepo splits an "owner/name" reference into its parts, splitting on +// the first slash. It returns an error if either part is empty. +func ParseOwnerRepo(ref string) (owner, name string, err error) { + for i := 0; i < len(ref); i++ { + if ref[i] == '/' { + owner, name = ref[:i], ref[i+1:] + if owner == "" || name == "" { + break + } + return owner, name, nil + } + } + return "", "", fmt.Errorf("expected owner/name format, got %q", ref) +} diff --git a/pkg/cmdutil/args_test.go b/pkg/cmdutil/args_test.go new file mode 100644 index 0000000..a051785 --- /dev/null +++ b/pkg/cmdutil/args_test.go @@ -0,0 +1,39 @@ +package cmdutil + +import "testing" + +func TestParseOwnerRepo(t *testing.T) { + tests := []struct { + input string + wantOwner string + wantName string + wantErr bool + }{ + {"alice.bsky.social/my-project", "alice.bsky.social", "my-project", false}, + {"did:plc:abc123/repo-name", "did:plc:abc123", "repo-name", false}, + {"alice/nested/path", "alice", "nested/path", false}, + {"just-a-name", "", "", true}, + {"/missing-owner", "", "", true}, + {"missing-name/", "", "", true}, + {"", "", "", true}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + owner, name, err := ParseOwnerRepo(tt.input) + if tt.wantErr { + if err == nil { + t.Errorf("ParseOwnerRepo(%q) expected error", tt.input) + } + return + } + if err != nil { + t.Fatalf("ParseOwnerRepo(%q) unexpected error: %v", tt.input, err) + } + if owner != tt.wantOwner || name != tt.wantName { + t.Errorf("ParseOwnerRepo(%q) = (%q, %q), want (%q, %q)", + tt.input, owner, name, tt.wantOwner, tt.wantName) + } + }) + } +} diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index ce68300..e28c23f 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -12,7 +12,7 @@ import ( // Fields that are functions are lazily initialized -- commands only pay // for what they actually use. type Factory struct { - IOStreams *iostreams.IOStreams + IOStreams *iostreams.IOStreams GitClient *gitpkg.Client // Lazy-initialized dependencies diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 1bf79fc..d9b456b 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -14,11 +14,11 @@ type IOStreams struct { Out io.Writer ErrOut io.Writer - colorEnabled bool - is256Color bool - isTrueColor bool + colorEnabled bool + is256Color bool + isTrueColor bool isInteractive bool - termWidth int + termWidth int } func System() *IOStreams { -- 2.51.2