diff --git a/CHANGELOG.md b/CHANGELOG.md index 80f169b..7f3a9f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased +- Reject mismatched repositories in pasted thread references before reads, syncs, enrichment, local overrides, or TUI jumps, and reject empty selections or list entries that could turn a targeted sync into an unrestricted one. - Skip unchanged issue-comment downloads on issues and PRs using parent timestamps, comment counts, and completed saved observations; keep PR review and detail data live, and add `sync`/`refresh --force` for a full selected refresh. Thanks @vlsi for the report. - Order captured comments chronologically when source timestamps have different fractional-second precision, preserving deterministic ties by kind and stable ID. diff --git a/docs/governance.md b/docs/governance.md index 814bb9d..44d9846 100644 --- a/docs/governance.md +++ b/docs/governance.md @@ -77,8 +77,9 @@ The chosen `--number` must already be a member of the cluster. The TUI's right-c All governance `--number` flags accept the same thread-reference forms as sync: bare numbers, `#123`, `issues/123`, `pull/123`, `owner/repo#123`, and full GitHub issue or pull request URLs. The command still applies only to the -`owner/repo` argument you pass to gitcrawl; URL input is accepted so copied -GitHub links can be pasted directly. +`owner/repo` argument you pass to gitcrawl. Qualified references must match that +repository (case-insensitively); a mismatched link is rejected before any local +override is written. ## Reopen and undo diff --git a/docs/sync.md b/docs/sync.md index f821544..2bc0d83 100644 --- a/docs/sync.md +++ b/docs/sync.md @@ -75,6 +75,13 @@ gitcrawl sync owner/repo --numbers https://github.com/owner/repo/issues/123 --wi `123`, `#123`, `issues/123`, `pull/123`, `owner/repo#123`, and full GitHub issue or pull request URLs. +Repository-qualified references must match the `owner/repo` argument +(case-insensitively). Mismatched repositories, unsupported URL forms, empty +comma-separated entries, and an explicitly empty `--numbers` value are rejected +before synchronization starts. Omit `--numbers` to select a repository-wide sync. +The same rule applies to `threads --numbers` and to `embed` or `summarize +--number`: an explicitly empty selection is an error, not an omitted filter. + ## Hydration depth | Flag | What it adds | diff --git a/internal/cli/app_test.go b/internal/cli/app_test.go index a9644b6..3b730c7 100644 --- a/internal/cli/app_test.go +++ b/internal/cli/app_test.go @@ -5438,13 +5438,13 @@ func TestAppOutputModesAndUsageBranches(t *testing.T) { if owner, repo, err := parseOwnerRepo("https://github.com/openclaw/openclaw/issues/78601"); err != nil || owner != "openclaw" || repo != "openclaw" { t.Fatalf("full issue URL owner/repo = %q/%q err=%v", owner, repo, err) } - if got, err := parseOptionalThreadNumber("https://github.com/openclaw/openclaw/issues/78601"); err != nil || got != 78601 { + if got, err := parseOptionalThreadNumber("https://github.com/openclaw/openclaw/issues/78601", "openclaw/openclaw"); err != nil || got != 78601 { t.Fatalf("full issue URL number = %d err=%v", got, err) } - if got, err := parseOptionalThreadNumber("https://github.com/openclaw/openclaw/pull/78602#issuecomment-1"); err != nil || got != 78602 { + if got, err := parseOptionalThreadNumber("https://github.com/openclaw/openclaw/pull/78602#issuecomment-1", "openclaw/openclaw"); err != nil || got != 78602 { t.Fatalf("full pull URL number = %d err=%v", got, err) } - if got, err := parseOptionalThreadNumberList("https://github.com/openclaw/openclaw/issues/78601, openclaw/openclaw#78602, pull/78603, #78604"); err != nil || len(got) != 4 || got[0] != 78601 || got[1] != 78602 || got[2] != 78603 || got[3] != 78604 { + if got, err := parseOptionalThreadNumberList("https://github.com/openclaw/openclaw/issues/78601, openclaw/openclaw#78602, pull/78603, #78604", "openclaw/openclaw"); err != nil || len(got) != 4 || got[0] != 78601 || got[1] != 78602 || got[2] != 78603 || got[3] != 78604 { t.Fatalf("thread ref list = %#v err=%v", got, err) } if _, _, _, err := parseClusterShapeOptions("test", "bad", "1", "0.5"); err == nil { diff --git a/internal/cli/embed.go b/internal/cli/embed.go index 025c175..78247b7 100644 --- a/internal/cli/embed.go +++ b/internal/cli/embed.go @@ -60,10 +60,13 @@ func (a *App) runEmbed(ctx context.Context, args []string) error { if err != nil { return usageErr(err) } - number, err := parseOptionalThreadNumber(*numberRaw) + number, err := parseOptionalThreadNumber(*numberRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } + if flagWasSet(fs, "number") && number == 0 { + return usageErr(fmt.Errorf("--number requires a thread reference")) + } limit, err := parseOptionalPositiveInt(*limitRaw) if err != nil { return usageErr(err) diff --git a/internal/cli/governance.go b/internal/cli/governance.go index df58205..5cdcb0c 100644 --- a/internal/cli/governance.go +++ b/internal/cli/governance.go @@ -25,7 +25,7 @@ func (a *App) runCloseThread(ctx context.Context, args []string) error { if err != nil { return usageErr(err) } - number, err := parseOptionalThreadNumber(*numberRaw) + number, err := parseOptionalThreadNumber(*numberRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } @@ -70,7 +70,7 @@ func (a *App) runReopenThread(ctx context.Context, args []string) error { if err != nil { return usageErr(err) } - number, err := parseOptionalThreadNumber(*numberRaw) + number, err := parseOptionalThreadNumber(*numberRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } @@ -206,7 +206,7 @@ func (a *App) runExcludeClusterMember(ctx context.Context, args []string) error if err != nil { return usageErr(err) } - clusterID, number, err := parseClusterMemberCommandIDs("exclude-cluster-member", *idRaw, *numberRaw) + clusterID, number, err := parseClusterMemberCommandIDs("exclude-cluster-member", *idRaw, *numberRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } @@ -248,7 +248,7 @@ func (a *App) runIncludeClusterMember(ctx context.Context, args []string) error if err != nil { return usageErr(err) } - clusterID, number, err := parseClusterMemberCommandIDs("include-cluster-member", *idRaw, *numberRaw) + clusterID, number, err := parseClusterMemberCommandIDs("include-cluster-member", *idRaw, *numberRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } @@ -290,7 +290,7 @@ func (a *App) runSetClusterCanonical(ctx context.Context, args []string) error { if err != nil { return usageErr(err) } - clusterID, number, err := parseClusterMemberCommandIDs("set-cluster-canonical", *idRaw, *numberRaw) + clusterID, number, err := parseClusterMemberCommandIDs("set-cluster-canonical", *idRaw, *numberRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } diff --git a/internal/cli/inspect.go b/internal/cli/inspect.go index e10d458..3f06693 100644 --- a/internal/cli/inspect.go +++ b/internal/cli/inspect.go @@ -241,10 +241,13 @@ func (a *App) runThreads(ctx context.Context, args []string) error { if err != nil { return usageErr(err) } - numbers, err := parseOptionalThreadNumberList(*numbersRaw) + numbers, err := parseOptionalThreadNumberList(*numbersRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } + if flagWasSet(fs, "numbers") && len(numbers) == 0 { + return usageErr(fmt.Errorf("--numbers requires at least one thread reference")) + } limit, err := parseOptionalPositiveInt(*limitRaw) if err != nil { return usageErr(err) diff --git a/internal/cli/neighbors.go b/internal/cli/neighbors.go index 81e122d..5a7e734 100644 --- a/internal/cli/neighbors.go +++ b/internal/cli/neighbors.go @@ -31,7 +31,7 @@ func (a *App) runNeighbors(ctx context.Context, args []string) error { if err != nil { return usageErr(err) } - number, err := parseRequiredThreadNumber("number", *numberRaw) + number, err := parseRequiredThreadNumber("number", *numberRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } diff --git a/internal/cli/references.go b/internal/cli/references.go index b7a6f4e..f382a24 100644 --- a/internal/cli/references.go +++ b/internal/cli/references.go @@ -106,7 +106,7 @@ func parseRequiredPositiveInt(name, value string) (int, error) { return parsed, nil } -func parseOptionalThreadNumber(value string) (int, error) { +func parseOptionalThreadNumber(value, repository string) (int, error) { if strings.TrimSpace(value) == "" { return 0, nil } @@ -114,11 +114,14 @@ func parseOptionalThreadNumber(value string) (int, error) { if !ok || ref.Number <= 0 { return 0, fmt.Errorf("expected positive issue or pull request number, got %q", value) } + if ref.FullName() != "" && !strings.EqualFold(ref.FullName(), repository) { + return 0, fmt.Errorf("thread reference repository %q does not match %q", ref.FullName(), repository) + } return ref.Number, nil } -func parseRequiredThreadNumber(name, value string) (int, error) { - parsed, err := parseOptionalThreadNumber(value) +func parseRequiredThreadNumber(name, value, repository string) (int, error) { + parsed, err := parseOptionalThreadNumber(value, repository) if err != nil { return 0, err } @@ -128,7 +131,7 @@ func parseRequiredThreadNumber(name, value string) (int, error) { return parsed, nil } -func parseClusterMemberCommandIDs(command, clusterIDRaw, numberRaw string) (int, int, error) { +func parseClusterMemberCommandIDs(command, clusterIDRaw, numberRaw, repository string) (int, int, error) { clusterID, err := parseOptionalPositiveInt(clusterIDRaw) if err != nil { return 0, 0, err @@ -136,7 +139,7 @@ func parseClusterMemberCommandIDs(command, clusterIDRaw, numberRaw string) (int, if clusterID == 0 { return 0, 0, fmt.Errorf("%s requires --id", command) } - number, err := parseOptionalThreadNumber(numberRaw) + number, err := parseOptionalThreadNumber(numberRaw, repository) if err != nil { return 0, 0, err } @@ -185,14 +188,14 @@ func parseOptionalPositiveIntList(value string) ([]int, error) { return out, nil } -func parseOptionalThreadNumberList(value string) ([]int, error) { +func parseOptionalThreadNumberList(value, repository string) ([]int, error) { if strings.TrimSpace(value) == "" { return nil, nil } parts := strings.Split(value, ",") out := make([]int, 0, len(parts)) for _, part := range parts { - parsed, err := parseOptionalThreadNumber(strings.TrimSpace(part)) + parsed, err := parseRequiredThreadNumber("numbers member", strings.TrimSpace(part), repository) if err != nil { return nil, err } @@ -203,4 +206,4 @@ func parseOptionalThreadNumberList(value string) ([]int, error) { var githubThreadURLPattern = regexp.MustCompile(`(?i)^https?://github\.com/([\w.-]+)/([\w.-]+)/(?:issues|pull)/(\d+)(?:[/?#].*)?$`) var ownerRepoThreadPattern = regexp.MustCompile(`(?i)^([\w.-]+)/([\w.-]+)#(\d+)$`) -var pathThreadPattern = regexp.MustCompile(`(?i)(?:^|/)(?:issues|pull)/(\d+)(?:[/?#].*)?$`) +var pathThreadPattern = regexp.MustCompile(`(?i)^(?:issues|pull)/(\d+)(?:[/?#].*)?$`) diff --git a/internal/cli/references_test.go b/internal/cli/references_test.go new file mode 100644 index 0000000..0e5845c --- /dev/null +++ b/internal/cli/references_test.go @@ -0,0 +1,121 @@ +package cli + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/charmbracelet/bubbles/textinput" + tea "github.com/charmbracelet/bubbletea" + "github.com/openclaw/gitcrawl/internal/config" + "github.com/openclaw/gitcrawl/internal/store" +) + +func TestCloseThreadRejectsCrossRepositoryReference(t *testing.T) { + ctx := context.Background() + configPath := seedGHShimRepo(t, ctx) + app := New() + app.Stdout = &bytes.Buffer{} + err := app.Run(ctx, []string{"--config", configPath, "close-thread", "openclaw/openclaw", "--number", "https://github.com/other/repo/issues/10"}) + if ExitCode(err) != 2 || !strings.Contains(err.Error(), "does not match") { + t.Errorf("cross-repository close error = %v, want usage error for repository mismatch", err) + } + cfg, err := config.Load(configPath) + if err != nil { + t.Fatal(err) + } + st, err := store.Open(ctx, cfg.DBPath) + if err != nil { + t.Fatal(err) + } + defer st.Close() + repo, err := st.RepositoryByFullName(ctx, "openclaw/openclaw") + if err != nil { + t.Fatal(err) + } + threads, err := st.ListThreadsFiltered(ctx, store.ThreadListOptions{RepoID: repo.ID, Numbers: []int{10}, IncludeClosed: true}) + if err != nil || len(threads) != 1 || threads[0].ClosedAtLocal != "" { + t.Fatalf("wrong-repository reference changed local thread: %+v, %v", threads, err) + } +} + +func TestThreadReferencesPreserveRepositoryScope(t *testing.T) { + for _, value := range []string{"10", "#10", "issues/10", "pull/10", "OPENCLAW/OPENCLAW#10", "https://github.com/OPENCLAW/openclaw/pull/10#discussion_r1"} { + if number, err := parseOptionalThreadNumber(value, "openclaw/openclaw"); err != nil || number != 10 { + t.Errorf("parse %q = %d, %v", value, number, err) + } + } + for _, value := range []string{"other/repo#10", "https://github.com/other/repo/issues/10", "https://example.com/other/repo/issues/10", "other/repo/issues/10"} { + if number, err := parseOptionalThreadNumber(value, "openclaw/openclaw"); err == nil { + t.Errorf("parse %q = %d without rejecting mismatched or unsupported repository", value, number) + } + } +} + +func TestTUIJumpRejectsCrossRepositoryReference(t *testing.T) { + input := textinput.New() + input.SetValue("https://github.com/other/repo/issues/10") + model := clusterBrowserModel{ + searchInput: input, jumping: true, + payload: clusterBrowserPayload{Repository: "openclaw/openclaw", Clusters: []store.ClusterSummary{{ID: 1, RepresentativeNumber: 10}}}, + } + next, cmd := model.handleJumpKey(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd != nil || next.jumping || !strings.Contains(next.status, "does not match") || next.hasDetail { + t.Fatalf("cross-repository jump: status=%q, hasDetail=%t, cmd=%v", next.status, next.hasDetail, cmd) + } +} + +func TestCommandsRejectCrossRepositoryReferencesBeforeOpeningRuntime(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "invalid.toml") + if err := os.WriteFile(configPath, []byte("[invalid"), 0o600); err != nil { + t.Fatal(err) + } + for _, command := range []string{"sync", "threads", "embed", "summarize", "neighbors", "close-thread", "reopen-thread", "exclude-cluster-member", "include-cluster-member", "set-cluster-canonical"} { + t.Run(command, func(t *testing.T) { + flag := "--number" + if command == "sync" || command == "threads" { + flag = "--numbers" + } + args := []string{"--config", configPath, command, "openclaw/openclaw", flag, "other/repo#10"} + if strings.Contains(command, "cluster") { + args = append(args, "--id", "1") + } + err := New().Run(context.Background(), args) + if ExitCode(err) != 2 || !strings.Contains(err.Error(), "does not match") { + t.Fatalf("error = %v, want repository mismatch before runtime access", err) + } + }) + } +} + +func TestThreadNumberListsRejectEmptyMembers(t *testing.T) { + for _, value := range []string{",", "10,", ",10", "10, ,12"} { + t.Run(value, func(t *testing.T) { + if numbers, err := parseOptionalThreadNumberList(value, "openclaw/openclaw"); err == nil { + t.Fatalf("numbers = %v; malformed selection must not become an unrestricted sync", numbers) + } + }) + } +} + +func TestExplicitEmptyThreadSelectionIsRejected(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "invalid.toml") + if err := os.WriteFile(configPath, []byte("[invalid"), 0o600); err != nil { + t.Fatal(err) + } + for _, command := range []string{"sync", "threads", "embed", "summarize"} { + flag := "--numbers" + if command == "embed" || command == "summarize" { + flag = "--number" + } + for _, value := range []string{"", " ", ","} { + err := New().Run(context.Background(), []string{"--config", configPath, command, "openclaw/openclaw", flag, value}) + if ExitCode(err) != 2 { + t.Errorf("%s %s %q: error=%v, want usage error before runtime access", command, flag, value, err) + } + } + } +} diff --git a/internal/cli/summarize.go b/internal/cli/summarize.go index 0e812d6..19c64c7 100644 --- a/internal/cli/summarize.go +++ b/internal/cli/summarize.go @@ -68,10 +68,13 @@ func (a *App) runSummarize(ctx context.Context, args []string) error { if err != nil { return usageErr(err) } - number, err := parseOptionalThreadNumber(*numberRaw) + number, err := parseOptionalThreadNumber(*numberRaw, owner+"/"+repoName) if err != nil { return usageErr(err) } + if flagWasSet(fs, "number") && number == 0 { + return usageErr(fmt.Errorf("--number requires a thread reference")) + } limit, err := parseOptionalPositiveInt(*limitRaw) if err != nil { return usageErr(err) diff --git a/internal/cli/sync.go b/internal/cli/sync.go index aae408a..9fed2d7 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -46,10 +46,13 @@ func (a *App) runSync(ctx context.Context, args []string) error { if err != nil { return usageErr(err) } - numbers, err := parseOptionalThreadNumberList(*numbersRaw) + numbers, err := parseOptionalThreadNumberList(*numbersRaw, owner+"/"+repo) if err != nil { return usageErr(err) } + if flagWasSet(fs, "numbers") && len(numbers) == 0 { + return usageErr(fmt.Errorf("--numbers requires at least one thread reference")) + } with, err := parseSyncWith(*withRaw) if err != nil { return usageErr(err) diff --git a/internal/cli/tui_input.go b/internal/cli/tui_input.go index 0bff277..cd02268 100644 --- a/internal/cli/tui_input.go +++ b/internal/cli/tui_input.go @@ -97,8 +97,12 @@ func (m clusterBrowserModel) handleJumpKey(msg tea.KeyMsg) (clusterBrowserModel, m.jumping = false value := strings.TrimSpace(m.searchInput.Value()) m.searchInput.Blur() - number, err := parseOptionalThreadNumber(value) - if err != nil || number <= 0 { + number, err := parseOptionalThreadNumber(value, m.payload.Repository) + if err != nil { + m.status = err.Error() + return m, nil + } + if number <= 0 { m.status = "Enter a positive issue or PR number" return m, nil } diff --git a/internal/cli/tui_render_extra_test.go b/internal/cli/tui_render_extra_test.go index 3121c6f..66c0ec9 100644 --- a/internal/cli/tui_render_extra_test.go +++ b/internal/cli/tui_render_extra_test.go @@ -304,7 +304,7 @@ func TestTUIJumpKeyAndRefreshCommandBranches(t *testing.T) { input.SetValue("#0") model := clusterBrowserModel{searchInput: input, jumping: true} next, cmd := model.handleJumpKey(tea.KeyMsg{Type: tea.KeyEnter}) - if cmd != nil || next.jumping || next.status != "Enter a positive issue or PR number" { + if cmd != nil || next.jumping || !strings.Contains(next.status, "expected positive") { t.Fatalf("bad enter next=%+v cmd=%v", next, cmd) } input = textinput.New() @@ -312,7 +312,7 @@ func TestTUIJumpKeyAndRefreshCommandBranches(t *testing.T) { model = clusterBrowserModel{ searchInput: input, jumping: true, - payload: clusterBrowserPayload{Clusters: []store.ClusterSummary{{ID: 1, RepresentativeNumber: 123}}}, + payload: clusterBrowserPayload{Repository: "openclaw/openclaw", Clusters: []store.ClusterSummary{{ID: 1, RepresentativeNumber: 123}}}, allClusters: []store.ClusterSummary{{ID: 1, RepresentativeNumber: 123}}, detailCache: map[string]store.ClusterDetail{}, } diff --git a/internal/cli/tui_test.go b/internal/cli/tui_test.go index 3ee29af..97eb16b 100644 --- a/internal/cli/tui_test.go +++ b/internal/cli/tui_test.go @@ -2053,7 +2053,7 @@ func TestTUIJumpAndSortHelpersCoverStoreBackedBranches(t *testing.T) { model.jumping = true updated, _ := model.handleJumpKey(tea.KeyMsg{Type: tea.KeyEnter}) model = updated - if !strings.Contains(model.status, "Enter a positive") { + if !strings.Contains(model.status, "expected positive") { t.Fatalf("bad jump status = %q", model.status) } model.startJumpInput()