Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 2 additions & 33 deletions .claude/rules/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,6 @@ paths:

## 依存の向き

<!-- TRANSIENT(Part1 Task 7): internal/usecase ができたらこの注記を消す -->
> **`internal/usecase` はまだ存在しない。** 導入する判断は済んでおり(「層を足す前に」
> 参照)、この図はその**到達点**である。作る作業は
> `docs/superpowers/plans/2026-09-07-phase2-remediation-part1.md` の Task 5〜7 にある。
> 規約を先に置いたのは、規約が設計に沿っていない状態で実装を始めると誤った方向に
> 進むためで、**この節の「〜しない」は今日書くコードにそのまま効く**
> (`internal/tui` が `internal/gh/cli` を import しないことは既に守られており、
> depguard も入っている)。

```
cmd/octoscope (合成ルート: cli.New → usecase.New → app.New)
Expand Down Expand Up @@ -69,14 +60,6 @@ embed した interface のメソッドは数に含めない — embed 先が同
スタブをいくつ書かされたかを見る。6 を超えたら、その interface は
「1 つの画面が使う分」より大きい。

<!-- TRANSIENT(Part1 Task 7): detail.Source が 6 メソッドになったらこの注記を消す -->
> **2026-09-07 時点で `internal/tui/detail` がこれを破っている。**
> `prSource` 7 / `issueSource` 6 で、同じ操作を PR と Issue で 2 本ずつ持っている
> だけである。`gh.ItemRef` は既に `Kind` を持つので、この振り分けは View の
> 仕事ではない。`internal/usecase` に移して 6 メソッドにする作業が
> `docs/superpowers/plans/2026-09-07-phase2-remediation-part1.md` の Task 7 にある。
> **新しく書く interface でこれを言い訳にしない。**

## GitHub API 固有の値はパッケージの外に出さない

`"APPROVED"`、`"CHANGES_REQUESTED"`、`"OPEN"` のような GitHub API の文字列を
Expand Down Expand Up @@ -109,30 +92,17 @@ TUI の都合ではない。ビューが知るべきなのは「行コメント
同じことが「種別(PR / Issue)で呼ぶものが変わる」にも当てはまる。
`gh.ItemRef.Kind` を View で `switch` しない。

<!-- TRANSIENT(Part1 Task 7): 呼び出し順序と振り分けを usecase に移したらこの注記を消す -->
> **2026-09-07 時点で `diff` と `review` と `detail` がこれを破っている。**
> `diff/comment.go` の `post()` が `StartReview` → `AddReviewThread` を、
> `review/review.go` の `submit()` が pending の有無で呼び分けを、
> `detail/detail.go` の `fetch` / `postComment` / `setState` / `applyPicker` が
> 種別の振り分けを持っている。移す作業は上の実装計画の Task 6〜7。
> **新しく書くコードでこれを言い訳にしない。**

## `usecase.Item` を画面の写しにしない

`usecase.Item` は PR と Issue の合流点であって DTO ではない。
**ここは UI の都合が下の層に漏れる唯一の穴**なので、太らせない。

<!-- TRANSIENT(Part1 Task 5): usecase.Item ができたらこの注記を消す -->
> **`usecase.Item` もまだ存在しない。** 作るのは上の実装計画の Task 5 で、
> この節はそのときに従う規則である。

- **`Item` に共通フィールドを足してよいのは、PR と Issue の両方に GitHub 側の
対応物があるときだけ。**
- PR にしか無いものは `Item.PR`(`*gh.PR`)から読む。`Item` に写さない
- 「画面に出したいものが `Item` に無い」と思ったら、まず `internal/gh` の
ドメイン型に無いのではないかを疑う。`gh.Issue` の公開フィールドは 10 個あり、
`Item` はその全部を持つように作る(2026-09-07 に数えた)
<!-- TRANSIENT(Part1 Task 5): 「作る」を「持っている」に直す -->
`Item` はその全部を持っている(2026-09-07 に数えた)

この規則があるかぎり、UI だけの修正(色・桁・文言・キー・状態遷移・
何を描くかの選び方)は `internal/usecase` に波及しない。波及するのは
Expand All @@ -151,8 +121,7 @@ GitHub への**新しい操作**を足すときだけで、それは元から UI
DI コンテナ、ドメインモデルとインフラモデルの二重定義、Input/Output DTO は
**入れていない**。

<!-- TRANSIENT(Part1 Task 5): 「(パッケージ自体はまだ無い)」だけ消す。日付は残す -->
**`internal/usecase` を入れる判断を 2026-09-07 にした(パッケージ自体はまだ無い)。**
**`internal/usecase` を入れる判断を 2026-09-07 にした。**
それまでは「Web サービス向けの構造だから入れない」という一般論で退けていたが、
その判断は `internal/tui` が 1 行も存在しない時点(Phase 0、`67ba0de`)に書かれ、
以後一度も再検証されていなかった。再検証したときの実測は次のとおりである。
Expand Down
4 changes: 0 additions & 4 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ linters:
desc: views go through internal/usecase; only cmd/octoscope names a backend
# usecase sits below the UI: it decides which GitHub calls an
# operation takes, and knows nothing of screens or messages.
# TRANSIENT(Part1 Task 5): drop these three lines once the package exists.
# The package does not exist yet -- the rule is here so the boundary
# is guarded from the first file, not after the fact. No file matches
# until then, which costs nothing.
usecase-layer:
files:
- "**/internal/usecase/**"
Expand Down
4 changes: 3 additions & 1 deletion cmd/octoscope/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/kukv/octoscope/internal/i18n"
"github.com/kukv/octoscope/internal/tui/app"
"github.com/kukv/octoscope/internal/tui/icon"
"github.com/kukv/octoscope/internal/usecase"
)

// version is set by GoReleaser via -ldflags at release build time.
Expand Down Expand Up @@ -49,7 +50,8 @@ func main() {
// not here: answering it costs a gh subprocess, and waiting for one before
// the first frame left the terminal blank for as long as it took.
client := cli.New(dir, *repoFlag)
p := tea.NewProgram(app.New(client, app.Options{HasRepo: *repoFlag != ""}))
uc := usecase.New(client)
p := tea.NewProgram(app.New(uc, app.Options{HasRepo: *repoFlag != ""}))
if _, err := p.Run(); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
Expand Down
4 changes: 4 additions & 0 deletions internal/gh/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ func (c *Client) ListLabels(ctx context.Context, repo string) ([]gh.Label, error
// ListAssignees returns the logins of users assignable on the repository.
// gh api substitutes {owner}/{repo} from the current directory's repo; for an
// override we build the explicit path (gh api takes no --repo).
//
// per_page=100 is REST's maximum, and the request is not paged: a repository
// with more than 100 assignable users would give the picker a list nobody
// could pick from anyway.
func (c *Client) ListAssignees(ctx context.Context, repo string) ([]string, error) {
path := "repos/{owner}/{repo}/assignees?per_page=100"
if r := c.effectiveRepo(repo); r != "" {
Expand Down
59 changes: 59 additions & 0 deletions internal/gh/cli/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package cli
import (
"context"
"errors"
"os"
"path/filepath"
"reflect"
"slices"
"strconv"
Expand Down Expand Up @@ -38,6 +40,16 @@ func newTestClient(out string, err error) (*Client, *fakeRun) {
return &Client{dir: "/repo", run: f.run}, f
}

func readTestdata(t *testing.T, name string) string {
t.Helper()

b, err := os.ReadFile(filepath.Join("testdata", name))
if err != nil {
t.Fatalf("read testdata: %v", err)
}
return string(b)
}

func TestListPRs(t *testing.T) {
c, f := newTestClient(prListJSON, nil)
prs, err := c.ListPRs(t.Context())
Expand Down Expand Up @@ -408,3 +420,50 @@ func TestTheListsAskForMoreThanTheDefaultThirty(t *testing.T) {
})
}
}

func TestListPRsParsesARecordedResponse(t *testing.T) {
c, _ := newTestClient(readTestdata(t, "pr_list.json"), nil)

prs, err := c.ListPRs(t.Context())
if err != nil {
t.Fatalf("ListPRs: %v", err)
}
if len(prs) == 0 {
t.Fatal("no pull requests parsed out of the recording")
}
for _, pr := range prs {
if pr.Number == 0 {
t.Errorf("pr %q has no number", pr.Title)
}
if pr.Title == "" {
t.Errorf("pr #%d has no title", pr.Number)
}
if pr.Author.Login == "" {
t.Errorf("pr #%d has no author", pr.Number)
}
if pr.URL == "" {
t.Errorf("pr #%d has no url; o has nothing to open", pr.Number)
}
if pr.UpdatedAt.IsZero() {
t.Errorf("pr #%d has no updatedAt; the board sorts on it", pr.Number)
}
}
}

func TestGetPRParsesARecordedResponse(t *testing.T) {
c, _ := newTestClient(readTestdata(t, "pr_view.json"), nil)

pr, err := c.GetPR(t.Context(), "", 55)
if err != nil {
t.Fatalf("GetPR: %v", err)
}
if pr.Number != 55 {
t.Errorf("number = %d, want 55", pr.Number)
}
if pr.Body == "" {
t.Error("body is empty; the detail view has nothing to draw")
}
if pr.Head == "" || pr.Base == "" {
t.Errorf("head/base = %q/%q, want both", pr.Head, pr.Base)
}
}
20 changes: 14 additions & 6 deletions internal/gh/cli/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ import (
"github.com/kukv/octoscope/internal/gh"
)

const (
// A single diff line can exceed bufio's default 64KiB limit (a minified
// bundle is one line), and the scanner then stops mid-file.
scanBufInit = 64 * 1024
scanBufMax = 8 * 1024 * 1024

// Everything after the second @@ in `@@ -12,7 +12,9 @@ func Walk(...)` is
// git's enclosing-function heuristic, which can itself start with + or -.
hunkHeaderFields = 3
)

// PRDiff returns the pull request's diff, one entry per file.
//
// --color never is passed explicitly: gh colours its output when it thinks a
Expand Down Expand Up @@ -142,7 +153,7 @@ func fileStatusFromAPI(s string) gh.FileStatus {
func parseBarePatch(patch string) []gh.Hunk {
p := &diffParser{file: &gh.FileDiff{}}
s := bufio.NewScanner(strings.NewReader(patch))
s.Buffer(make([]byte, 0, 64*1024), 8*1024*1024)
s.Buffer(make([]byte, 0, scanBufInit), scanBufMax)
for s.Scan() {
p.line(s.Text())
}
Expand All @@ -157,10 +168,7 @@ func parseBarePatch(patch string) []gh.Hunk {
func parseDiff(b []byte) []gh.FileDiff {
p := &diffParser{}
s := bufio.NewScanner(bytes.NewReader(b))
// A single diff line can be far longer than bufio's default 64KiB limit
// (a minified bundle is one line), and a scanner that gives up mid-file
// would drop every file after it.
s.Buffer(make([]byte, 0, 64*1024), 8*1024*1024)
s.Buffer(make([]byte, 0, scanBufInit), scanBufMax)
for s.Scan() {
p.line(s.Text())
}
Expand Down Expand Up @@ -267,7 +275,7 @@ func pathFromGitHeader(line string) string {
// too, so fields past the second `@@` must not be scanned for a range.
func hunkStarts(header string) (oldNo, newNo int) {
fields := strings.Fields(header)
for _, f := range fields[:min(3, len(fields))] {
for _, f := range fields[:min(hunkHeaderFields, len(fields))] {
switch {
case strings.HasPrefix(f, "-"):
oldNo = firstNumber(f[1:])
Expand Down
19 changes: 19 additions & 0 deletions internal/gh/cli/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,3 +340,22 @@ func TestDiffLineNamesTheSideToCommentOn(t *testing.T) {
})
}
}

// prFiles is what PRDiff falls back to when `gh pr diff` fails; the recording
// proves parseBarePatch is fed the shape GitHub actually sends there.
func TestPRFilesParsesARecordedResponse(t *testing.T) {
c, _ := newTestClient(readTestdata(t, "pr_files.json"), nil)

files, err := c.prFiles(t.Context(), "", 55)
if err != nil {
t.Fatalf("prFiles: %v", err)
}
if len(files) == 0 {
t.Fatal("no files parsed out of the recording")
}
for _, f := range files {
if f.Path == "" {
t.Error("a file has no path")
}
}
}
87 changes: 87 additions & 0 deletions internal/gh/cli/graphql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ package cli

import (
"context"
"encoding/json"
"errors"
"reflect"
"regexp"
"slices"
"strconv"
"strings"
"testing"

Expand Down Expand Up @@ -318,3 +321,87 @@ func TestTheQueryCarriesEveryAliasWeReadBack(t *testing.T) {
}
}
}

// GitHub caps a labels connection's first at 100; asking for less silently
// drops labels past that count.
func TestWorkQueryAsksForAsManyLabelsAsGitHubAllows(t *testing.T) {
t.Parallel()

if strings.Contains(workQuery, "labels(first: 10)") {
t.Error("work.graphql still asks for 10 labels; GitHub allows 100")
}
if n := strings.Count(workQuery, "labels(first: 100)"); n != 2 {
t.Errorf("labels(first: 100) appears %d times, want 2 (PullRequest and Issue)", n)
}
}

// 101 is refused outright with EXCESSIVE_PAGINATION, failing the whole
// document.
func TestNoConnectionAsksForMoreThanGitHubAllows(t *testing.T) {
t.Parallel()

docs := map[string]string{
"work.graphql": workQuery,
"review.graphql": reviewContextQuery,
}
re := regexp.MustCompile(`first:\s*(\d+)`)
for name, doc := range docs {
for _, m := range re.FindAllStringSubmatch(doc, -1) {
n, err := strconv.Atoi(m[1])
if err != nil {
t.Fatalf("%s: %v", name, err)
}
if n > 100 {
t.Errorf("%s: %s exceeds GitHub's cap of 100", name, m[0])
}
}
}
}

// A recording is the only way to know the four aliases the query declares
// still match the keys the answer carries.
func TestListWorkParsesARecordedResponse(t *testing.T) {
raw := readTestdata(t, "work.json")

var doc struct {
Data map[string]json.RawMessage `json:"data"`
}
if err := json.Unmarshal([]byte(raw), &doc); err != nil {
t.Fatalf("unmarshal recording: %v", err)
}
wantAliases := []string{"assigned", "mentioned", "reviewRequested", "yourPRs"}
gotAliases := make([]string, 0, len(doc.Data))
for k := range doc.Data {
gotAliases = append(gotAliases, k)
}
slices.Sort(gotAliases)
if !slices.Equal(gotAliases, wantAliases) {
t.Errorf("recorded aliases = %v, want %v", gotAliases, wantAliases)
}

c, _ := newTestClient(raw, nil)
w, err := c.ListWork(t.Context())
if err != nil {
t.Fatalf("ListWork: %v", err)
}
total := 0
for _, section := range gh.WorkSections() {
total += len(w[section])
}
if total == 0 {
t.Fatal("no work items parsed out of the recording")
}
for _, section := range gh.WorkSections() {
for _, item := range w[section] {
if item.Ref.Repo == "" {
t.Errorf("section %d: %q has no repo; the card cannot be opened", section, item.Title)
}
if item.Ref.Number == 0 {
t.Errorf("section %d: %q has no number", section, item.Title)
}
if item.URL == "" {
t.Errorf("section %d: %q has no url", section, item.Title)
}
}
}
}
Loading