diff --git a/README.md b/README.md index 8ca428c..f0a0723 100644 --- a/README.md +++ b/README.md @@ -105,28 +105,16 @@ prism fetch --format text # Raw PR data as text ```json { - "provider": "github", "pull_request": { + "provider": "github", "repository": "owner/repo", "id": "123", "title": "Add retry handling for payment API", "author": "example", "source_branch": "feature/payment-retry", "target_branch": "main", - "description": "Adds retry logic for transient payment API failures" + "url": "https://github.com/owner/repo/pull/123" }, - "changed_files": [ - { - "path": "internal/payment/client.go", - "status": "modified", - "additions": 45, - "deletions": 3, - "language": "Go", - "is_test": false, - "is_config": false, - "is_generated": false - } - ], "analysis": { "change_type": "feature", "risk_level": "medium", @@ -145,7 +133,16 @@ prism fetch --format text # Raw PR data as text "No test files included in this change" ], "summary": "feature: Add retry handling for payment API (1 file changed, +45/-3)" - } + }, + "changed_files": [ + { + "path": "internal/payment/client.go", + "status": "modified", + "additions": 45, + "deletions": 3, + "language": "Go" + } + ] } ``` @@ -166,6 +163,7 @@ prism fetch --format text # Raw PR data as text | Author | example | | Branch | feature/payment-retry -> main | | Provider | github | +| URL | https://github.com/owner/repo/pull/123 | ## Analysis @@ -435,8 +433,9 @@ make clean # Remove bin/ - **v0.1.0** — GitHub provider, analyze/prompt/fetch commands, JSON/Markdown/text output, light/detailed/cross modes, config/lang/template support, exit codes ✅ - **v0.2.0** — Provider plugin architecture, `--provider` flag, AWS CodeCommit support via plugin ✅ -- **v0.3.0** — Policy files, custom review axes, project-specific rules -- **v0.4.0+** — Review policy as code, SARIF output, metrics, IDE/CI integration +- **v0.3.0** — Public library API (`pkg/prism`), CLI uses `pkg/prism` internally, foundation for `prism-api` (HTTP service) +- **v0.4.0** — Policy files, custom review axes, project-specific rules +- **v0.5.0+** — SARIF output, metrics, IDE/CI integration ## Releases diff --git a/cmd/prism/integration_test.go b/cmd/prism/integration_test.go index 84addbf..ee17bfe 100644 --- a/cmd/prism/integration_test.go +++ b/cmd/prism/integration_test.go @@ -3,7 +3,6 @@ package main import ( "bytes" "context" - "encoding/json" "fmt" "net/http" "net/http/httptest" @@ -11,7 +10,6 @@ import ( "testing" "github.com/hidetzu/prism/internal/domain" - "github.com/hidetzu/prism/internal/formatter" "github.com/hidetzu/prism/internal/provider" ghprovider "github.com/hidetzu/prism/internal/provider/github" "github.com/hidetzu/prism/internal/usecase" @@ -158,82 +156,10 @@ func TestCLIHelpContainsCommands(t *testing.T) { } } -// TestUsecaseIntegrationAnalyzeJSON tests the full pipeline through usecase layer -// with a mock HTTP server, verifying JSON output structure. -func TestUsecaseIntegrationAnalyzeJSON(t *testing.T) { - server := setupGitHubMock(t) - t.Cleanup(server.Close) - - // Use provider with mock server directly. - ghprov := newTestProvider(t, server) - ref, err := ghprov.Parse("https://github.com/owner/repo/pull/1") - if err != nil { - t.Fatalf("Parse: %v", err) - } - - var buf bytes.Buffer - err = usecaseAnalyze(t, ghprov, ref, "json", &buf) - if err != nil { - t.Fatalf("Analyze: %v", err) - } - - var out formatter.Output - if err := json.Unmarshal(buf.Bytes(), &out); err != nil { - t.Fatalf("unmarshal: %v", err) - } - if out.Provider != "github" { - t.Errorf("provider = %q", out.Provider) - } - if out.PullRequest.Title != "Fix login bug" { - t.Errorf("title = %q", out.PullRequest.Title) - } - if out.Analysis.ChangeType != "bugfix" { - t.Errorf("change_type = %q, want bugfix", out.Analysis.ChangeType) - } - if len(out.ChangedFiles) != 2 { - t.Errorf("changed_files = %d, want 2", len(out.ChangedFiles)) - } -} - -func TestUsecaseIntegrationAnalyzeMarkdown(t *testing.T) { - server := setupGitHubMock(t) - t.Cleanup(server.Close) - - ghprov := newTestProvider(t, server) - ref, _ := ghprov.Parse("https://github.com/owner/repo/pull/1") - - var buf bytes.Buffer - if err := usecaseAnalyze(t, ghprov, ref, "markdown", &buf); err != nil { - t.Fatalf("Analyze markdown: %v", err) - } - out := buf.String() - if !strings.Contains(out, "# Fix login bug") { - t.Error("markdown missing title") - } - if !strings.Contains(out, "bugfix") { - t.Error("markdown missing change type") - } -} - -func TestUsecaseIntegrationAnalyzeText(t *testing.T) { - server := setupGitHubMock(t) - t.Cleanup(server.Close) - - ghprov := newTestProvider(t, server) - ref, _ := ghprov.Parse("https://github.com/owner/repo/pull/1") - - var buf bytes.Buffer - if err := usecaseAnalyze(t, ghprov, ref, "text", &buf); err != nil { - t.Fatalf("Analyze text: %v", err) - } - out := buf.String() - if !strings.Contains(out, "Fix login bug") { - t.Error("text missing title") - } - if !strings.Contains(out, "bugfix") { - t.Error("text missing change type") - } -} +// Note: The analyze pipeline (provider → classifier → analyzer → formatter) is +// covered by unit tests in pkg/prism, internal/formatter, internal/classifier, +// and internal/analyzer. The integration tests below focus on prompt and fetch +// use cases which still go through internal/usecase. func TestUsecaseIntegrationPromptLight(t *testing.T) { server := setupGitHubMock(t) @@ -365,11 +291,6 @@ func (p *ghProviderForTest) FetchPullRequest(ctx context.Context, ref domain.PRR return prov.FetchPullRequest(ctx, ref) } -func usecaseAnalyze(t *testing.T, p provider.Provider, ref domain.PRRef, format string, buf *bytes.Buffer) error { - t.Helper() - return usecase.Analyze(context.Background(), p, ref, usecase.AnalyzeOptions{Format: format}, buf) -} - func usecasePrompt(t *testing.T, p provider.Provider, ref domain.PRRef, mode, format, lang string, buf *bytes.Buffer) error { t.Helper() return usecase.Prompt(context.Background(), p, ref, usecase.PromptOptions{Mode: mode, Format: format, Lang: lang}, buf) diff --git a/cmd/prism/main.go b/cmd/prism/main.go index 7073cc1..0e6a896 100644 --- a/cmd/prism/main.go +++ b/cmd/prism/main.go @@ -9,11 +9,13 @@ import ( "github.com/hidetzu/prism/internal/config" "github.com/hidetzu/prism/internal/domain" + "github.com/hidetzu/prism/internal/formatter" "github.com/hidetzu/prism/internal/provider" "github.com/hidetzu/prism/internal/usecase" + "github.com/hidetzu/prism/pkg/prism" ) -const version = "0.2.0" +const version = "0.3.0-dev" // Exit codes as defined in docs/spec.md. const ( @@ -33,9 +35,13 @@ func main() { func exitCode(err error) int { switch { - case errors.Is(err, domain.ErrInvalidArgs): + case errors.Is(err, domain.ErrInvalidArgs), + errors.Is(err, prism.ErrInvalidInput), + errors.Is(err, prism.ErrUnsupportedProvider): return ExitInvalidArgs - case errors.Is(err, domain.ErrProvider): + case errors.Is(err, domain.ErrProvider), + errors.Is(err, prism.ErrAuthRequired), + errors.Is(err, prism.ErrUpstreamFailure): return ExitProviderError case errors.Is(err, domain.ErrAnalysis): return ExitAnalysisError @@ -134,19 +140,36 @@ func runAnalyze(cmd *cobra.Command, args []string) error { return fmt.Errorf("load config: %w", err) } - p, ref, err := resolveProvider(cmd, cfg, args[0]) - if err != nil { - return err - } - format, _ := cmd.Flags().GetString("format") if format == "" { format = cfg.DefaultFormat } + if format == "" { + format = "json" + } + if format != "json" && format != "markdown" && format != "text" { + return fmt.Errorf("%w: invalid format %q: must be json, markdown, or text", domain.ErrInvalidArgs, format) + } - return usecase.Analyze(cmd.Context(), p, ref, usecase.AnalyzeOptions{ - Format: format, - }, os.Stdout) + providerName, _ := cmd.Flags().GetString("provider") + + result, err := prism.Analyze(cmd.Context(), prism.AnalyzeOptions{ + Provider: providerName, + PRURL: args[0], + GitHubToken: cfg.GitHubToken, + }) + if err != nil { + return err + } + + switch format { + case "markdown": + return formatter.FormatMarkdown(os.Stdout, result) + case "text": + return formatter.FormatText(os.Stdout, result) + default: + return formatter.FormatJSON(os.Stdout, result) + } } func runPrompt(cmd *cobra.Command, args []string) error { diff --git a/docs/adr/0002-public-api-boundary.md b/docs/adr/0002-public-api-boundary.md index cb4635a..e5ed3d2 100644 --- a/docs/adr/0002-public-api-boundary.md +++ b/docs/adr/0002-public-api-boundary.md @@ -174,19 +174,19 @@ The `AnalyzeOptions` struct is designed to be **additive-only**: new optional fi - **Formatter functions** — callers work with `Result` as structured data, not serialized strings. If string output is needed, callers format `Result` themselves. - **Prompt templates** — templates are internal. Custom templates are still supported via `--template` at the CLI level, which reads from a file path. -### 4. Differences from the CLI JSON schema +### 4. CLI JSON output and `Result` are byte-identical -`Result` is **structurally similar** to the CLI JSON output (`docs/json-schema.md`) but not identical. The differences are intentional: +As of Phase 2 (v0.3.0), the CLI JSON output is produced by serializing `pkg/prism.Result` directly. The two are guaranteed to be byte-identical, verified by golden tests in `internal/formatter/testdata/`. -| Field | CLI JSON | `pkg/prism.Result` | Reason | -|-------|----------|---------------------|--------| -| `provider` | top-level | `pull_request.provider` | Library consumers treat the PR as a self-contained object; provider belongs with PR metadata | +The Phase 1 design intentionally diverged from the v0.2.x CLI JSON schema in three places: + +| Field | v0.2.x CLI JSON | `pkg/prism.Result` (v0.3.0+) | Reason | +|-------|------------------|------------------------------|--------| +| `provider` | top-level | `pull_request.provider` | Provider belongs with PR metadata; library consumers treat the PR as a self-contained object | | `pull_request.description` | included | **excluded** | Descriptions can be large and are rarely needed by programmatic consumers; reduces response size | | `pull_request.url` | not present | **included** | Avoids requiring consumers to reconstruct the URL from owner/repo/id | -These differences will be unified in **Phase 2** when the CLI is refactored to use `pkg/prism` internally. At that point, the CLI JSON schema will align with `pkg/prism.Result` (a potential breaking change to CLI output, to be announced in release notes). - -Until then, consumers who need the exact CLI JSON shape should use the CLI binary; consumers who want the library API should use `pkg/prism`. +In v0.3.0, the CLI JSON schema is updated to match `pkg/prism.Result`. This is a **breaking change** to the CLI JSON output, documented in the v0.3.0 release notes. ### 5. Compatibility guarantees @@ -264,18 +264,21 @@ Accepted. Minimal public surface, maximum internal freedom, clean consumer story 3. Add unit tests for `pkg/prism` covering validation and error categorization 4. Document the public API in `docs/public-api.md` or README -### Phase 2 (subsequent PR) +### Phase 2 (completed in v0.3.0) + +5. Refactor `cmd/prism analyze` to use `pkg/prism.Analyze()` internally ✓ +6. Rewrite `internal/formatter` to take `prism.Result` instead of domain types ✓ +7. Update CLI JSON schema to match `pkg/prism.Result` (breaking change documented in release notes) ✓ + +### Phase 2 deferred items -5. Refactor `cmd/prism` to use `pkg/prism` internally - - `analyze` JSON format: call `pkg/prism.Analyze()` and `json.Marshal(result)` - - `analyze` markdown/text format: either reuse existing `internal/formatter` (taking `Result`) or keep current path - - `prompt` command: switch to `pkg/prism.Prompt()` -6. Verify CLI JSON output matches `pkg/prism.Result` JSON exactly via a golden test +- `cmd/prism prompt` still uses `internal/usecase` because `pkg/prism.Prompt()` returns a plain string and does not yet cover `--format json|markdown` or `--template`. Future work: extend `pkg/prism` with a `RenderPrompt` function returning a structured bundle. +- `cmd/prism fetch` still uses `internal/usecase` since it bypasses analysis entirely. -### Phase 3 (after CLI refactor) +### Phase 3 -7. Create `prism-api` repository -8. Tag prism v0.3.0 once the public API is validated +8. Create `prism-api` repository +9. Tag prism v0.3.0 once the public API is validated --- diff --git a/docs/architecture.md b/docs/architecture.md index 4ea6a7a..50fe8ba 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -55,12 +55,14 @@ flowchart LR B --> C[PullRequest
domain model] C --> D[Classifier] D --> E[Analyzer] - E --> F[AnalysisResult] + E --> F[prism.Result] F --> G{Output} - G -->|analyze| H[Formatter
JSON / Markdown] + G -->|analyze| H[Formatter
JSON / Markdown / Text] G -->|prompt| I[Prompt Renderer
light / detailed / cross] ``` +The `analyze` flow is exposed as `pkg/prism.Analyze()` and used by both the CLI and external library consumers. The `prompt` flow currently goes through `internal/usecase`. + ## Package Responsibilities ### `pkg/prism` @@ -71,7 +73,7 @@ Exposes `Analyze` and `Prompt` functions, stable input/output types (`AnalyzeOpt Consumers: -- **cmd/prism** — the CLI (refactor to use pkg/prism is Phase 2) +- **cmd/prism** — the CLI (`analyze` command uses `pkg/prism.Analyze()` internally) - **prism-api** — HTTP service (planned) - **Editor / IDE plugins** — library consumers - **CI / automation tools** — library consumers @@ -80,7 +82,7 @@ See [ADR-0002](adr/0002-public-api-boundary.md) for the design rationale and com ### `cmd/prism` -CLI entrypoint. Parses arguments, resolves configuration, and delegates to use cases. Should remain thin. +CLI entrypoint. Parses arguments, loads configuration, and delegates to either `pkg/prism` (for `analyze`) or `internal/usecase` (for `prompt` and `fetch`). Should remain thin. ### `internal/domain` @@ -123,7 +125,7 @@ Estimates risk level and suggests review axes based on classification results an ### `internal/formatter` -Serializes `AnalysisResult` into output formats (JSON, Markdown, text). +Serializes `pkg/prism.Result` into output formats (JSON, Markdown, text). Used by `cmd/prism analyze` to render the result returned by `pkg/prism.Analyze()`. The CLI JSON output is byte-identical to `pkg/prism.Result` JSON, verified by golden tests. ### `internal/prompt` @@ -131,7 +133,9 @@ Renders `PromptBundle` for each review mode using templates. ### `internal/usecase` -Orchestrates the pipeline: fetch → classify → analyze → format/render. Each use case corresponds to a CLI command. +Orchestrates the pipeline for `prism prompt` and `prism fetch` commands. The `analyze` use case has been replaced by `pkg/prism.Analyze()` (the CLI calls `pkg/prism` directly and formats the result via `internal/formatter`). + +Deferred to a future phase: extending `pkg/prism.Prompt` to cover the prompt CLI features (`--format json|markdown`, `--template`), so that prompt and fetch can also flow through `pkg/prism`. ## Design Principles diff --git a/docs/json-schema.md b/docs/json-schema.md index 1986038..e8c3f35 100644 --- a/docs/json-schema.md +++ b/docs/json-schema.md @@ -2,7 +2,7 @@ This document describes the JSON output of the `prism analyze` CLI command. -> **Note:** Library consumers using [`pkg/prism`](../pkg/prism) receive a `Result` struct that is structurally similar but not identical to this CLI JSON output. See [ADR-0002](adr/0002-public-api-boundary.md) for the differences. The CLI JSON schema and `pkg/prism.Result` will be unified in Phase 2 when the CLI is refactored to call `pkg/prism` internally. +The CLI calls [`pkg/prism.Analyze()`](../pkg/prism) internally and serializes the returned `Result` as JSON. The CLI JSON output and `pkg/prism.Result` are guaranteed to be byte-identical (verified by golden tests in `internal/formatter/testdata/`). ## `prism analyze` output @@ -10,22 +10,24 @@ This document describes the JSON output of the `prism analyze` CLI command. | Field | Type | Description | |-------|------|-------------| -| `provider` | string | PR source (`"github"`, `"codecommit"`) | -| `pull_request` | object | PR metadata | +| `pull_request` | object | PR metadata (provider, repository, identifiers, branches, URL) | +| `analysis` | object | Analysis results (change type, risk, review axes) | | `changed_files` | array | List of changed files | -| `analysis` | object | Analysis results | ### `pull_request` | Field | Type | Description | |-------|------|-------------| +| `provider` | string | PR source (`"github"`, `"codecommit"`, etc.) | | `repository` | string | Repository identifier (e.g., `"owner/repo"`) | | `id` | string | PR number/ID | | `title` | string | PR title | | `author` | string | PR author | -| `source_branch` | string | Source branch name | -| `target_branch` | string | Target branch name | -| `description` | string | PR description body | +| `source_branch` | string | Source branch name (omitted if empty) | +| `target_branch` | string | Target branch name (omitted if empty) | +| `url` | string | Canonical PR URL | + +> **Note:** PR description is not included in the JSON output. Description text can be large and is rarely used by programmatic consumers. Use `prism fetch` if you need the raw description. ### `changed_files[]` @@ -35,10 +37,11 @@ This document describes the JSON output of the `prism analyze` CLI command. | `status` | string | Change status (`"added"`, `"modified"`, `"removed"`, `"renamed"`) | | `additions` | int | Lines added | | `deletions` | int | Lines deleted | -| `language` | string | Detected language | -| `is_test` | bool | Whether this is a test file | -| `is_config` | bool | Whether this is a config file | -| `is_generated` | bool | Whether this is a generated file | +| `language` | string | Detected language (omitted if unknown) | +| `is_test` | bool | Test file flag (omitted if false) | +| `is_config` | bool | Config file flag (omitted if false) | +| `is_generated` | bool | Generated file flag (omitted if false) | +| `patch` | string | Unified diff content (omitted; populated only when explicitly requested via library API) | ### `analysis` @@ -46,11 +49,11 @@ This document describes the JSON output of the `prism analyze` CLI command. |-------|------|-------------| | `change_type` | string | Classified change type (see below) | | `risk_level` | string | `"low"`, `"medium"`, or `"high"` | -| `affected_areas` | string[] | Affected domain areas | -| `review_axes` | string[] | Suggested review focus points | -| `related_files` | string[] | Files related to the change | -| `warnings` | string[] | Notable concerns | -| `summary` | string | Brief description of the change | +| `affected_areas` | string[] | Affected domain areas (omitted if empty) | +| `review_axes` | string[] | Suggested review focus points (omitted if empty) | +| `related_files` | string[] | Files related to the change (omitted if empty) | +| `warnings` | string[] | Notable concerns (omitted if empty) | +| `summary` | string | Brief description of the change (omitted if empty) | ### Change types @@ -78,3 +81,15 @@ JSON output follows [Semantic Versioning](versioning.md): - **Patch/Minor**: New fields may be added (backward compatible) - **Major**: Fields may be renamed, removed, or change type - Consumers should ignore unknown fields for forward compatibility + +## Breaking changes in v0.3.0 + +The JSON output structure changed in v0.3.0 to align with `pkg/prism.Result`: + +- `provider` moved from top-level to `pull_request.provider` +- `pull_request.description` removed (use `prism fetch` for raw description) +- `pull_request.url` added (canonical PR URL) +- `changed_files[].patch` no longer included by default +- Empty/zero fields are now omitted from output (`omitempty` semantics) + +If you have downstream tooling parsing the v0.2.x output, update it to expect the new structure. diff --git a/internal/classifier/classifier.go b/internal/classifier/classifier.go index 8896e44..5ccc1cb 100644 --- a/internal/classifier/classifier.go +++ b/internal/classifier/classifier.go @@ -104,4 +104,3 @@ func classifyByDescription(desc string) (domain.ChangeType, bool) { return "", false } - diff --git a/internal/formatter/json.go b/internal/formatter/json.go index 1a18f9f..8e42a60 100644 --- a/internal/formatter/json.go +++ b/internal/formatter/json.go @@ -4,67 +4,13 @@ import ( "encoding/json" "io" - "github.com/hidetzu/prism/internal/domain" + "github.com/hidetzu/prism/pkg/prism" ) -// FormatJSON writes the analysis result as JSON to w. -func FormatJSON(w io.Writer, providerName string, pr domain.PullRequest, result domain.AnalysisResult) error { - out := toOutput(providerName, pr, result) - +// FormatJSON writes a prism.Result as indented JSON to w. +func FormatJSON(w io.Writer, result prism.Result) error { enc := json.NewEncoder(w) enc.SetIndent("", " ") enc.SetEscapeHTML(false) - return enc.Encode(out) -} - -func toOutput(providerName string, pr domain.PullRequest, result domain.AnalysisResult) Output { - files := make([]ChangedFileOutput, 0, len(pr.ChangedFiles)) - for _, f := range pr.ChangedFiles { - files = append(files, ChangedFileOutput{ - Path: f.Path, - Status: string(f.Status), - Additions: f.Additions, - Deletions: f.Deletions, - Language: f.Language, - IsTest: f.IsTest, - IsConfig: f.IsConfig, - IsGenerated: f.IsGenerated, - }) - } - - axes := make([]string, 0, len(result.ReviewAxes)) - for _, a := range result.ReviewAxes { - axes = append(axes, string(a)) - } - - return Output{ - Provider: providerName, - PullRequest: PullRequestOutput{ - Repository: pr.Repository, - ID: pr.ID, - Title: pr.Title, - Author: pr.Author, - SourceBranch: pr.SourceBranch, - TargetBranch: pr.TargetBranch, - Description: pr.Description, - }, - ChangedFiles: files, - Analysis: AnalysisOutput{ - ChangeType: string(result.ChangeType), - RiskLevel: string(result.RiskLevel), - AffectedAreas: nonNilSlice(result.AffectedAreas), - ReviewAxes: axes, - RelatedFiles: nonNilSlice(result.RelatedFiles), - Warnings: nonNilSlice(result.Warnings), - Summary: result.Summary, - }, - } -} - -// nonNilSlice ensures nil slices become empty arrays in JSON. -func nonNilSlice(s []string) []string { - if s == nil { - return []string{} - } - return s + return enc.Encode(result) } diff --git a/internal/formatter/json_test.go b/internal/formatter/json_test.go index e851ba5..16461c6 100644 --- a/internal/formatter/json_test.go +++ b/internal/formatter/json_test.go @@ -7,29 +7,40 @@ import ( "path/filepath" "testing" - "github.com/hidetzu/prism/internal/domain" "github.com/hidetzu/prism/internal/formatter" + "github.com/hidetzu/prism/pkg/prism" ) -func testPR() domain.PullRequest { - return domain.PullRequest{ - Repository: "owner/repo", - ID: "42", - Title: "Add OAuth2 login", - Author: "dev", - SourceBranch: "feature/oauth", - TargetBranch: "main", - Description: "Adds OAuth2 login flow", - ChangedFiles: []domain.ChangedFile{ +func testResult() prism.Result { + return prism.Result{ + PR: prism.PRInfo{ + Provider: "github", + Repository: "owner/repo", + ID: "42", + Title: "Add OAuth2 login", + Author: "dev", + SourceBranch: "feature/oauth", + TargetBranch: "main", + URL: "https://github.com/owner/repo/pull/42", + }, + Analysis: prism.AnalysisResult{ + ChangeType: "feature", + RiskLevel: "medium", + AffectedAreas: []string{"auth"}, + ReviewAxes: []string{"security", "error handling"}, + Warnings: []string{"New authentication flow"}, + Summary: "Adds OAuth2 login flow with tests", + }, + Files: []prism.ChangedFile{ { Path: "internal/auth/oauth.go", - Status: domain.FileStatusAdded, + Status: "added", Additions: 120, Language: "Go", }, { Path: "internal/auth/oauth_test.go", - Status: domain.FileStatusAdded, + Status: "added", Additions: 80, Language: "Go", IsTest: true, @@ -38,21 +49,9 @@ func testPR() domain.PullRequest { } } -func testResult() domain.AnalysisResult { - return domain.AnalysisResult{ - ChangeType: domain.ChangeTypeFeature, - RiskLevel: domain.RiskLevelMedium, - AffectedAreas: []string{"auth"}, - ReviewAxes: []domain.ReviewAxis{domain.ReviewAxisSecurity, domain.ReviewAxisErrorHandling}, - Warnings: []string{"New authentication flow"}, - Summary: "Adds OAuth2 login flow with tests", - } -} - func TestFormatJSONGolden(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatJSON(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatJSON(&buf, testResult()); err != nil { t.Fatalf("FormatJSON: %v", err) } @@ -62,7 +61,6 @@ func TestFormatJSONGolden(t *testing.T) { t.Fatalf("read golden file: %v", err) } - // Normalize both by re-encoding to ensure consistent formatting. got := normalizeJSON(t, buf.Bytes()) expected := normalizeJSON(t, want) @@ -73,24 +71,26 @@ func TestFormatJSONGolden(t *testing.T) { func TestFormatJSONStructure(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatJSON(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatJSON(&buf, testResult()); err != nil { t.Fatalf("FormatJSON: %v", err) } - var out formatter.Output + var out prism.Result if err := json.Unmarshal(buf.Bytes(), &out); err != nil { t.Fatalf("unmarshal: %v", err) } - if out.Provider != "github" { - t.Errorf("provider = %q, want %q", out.Provider, "github") + if out.PR.Provider != "github" { + t.Errorf("pull_request.provider = %q, want %q", out.PR.Provider, "github") + } + if out.PR.ID != "42" { + t.Errorf("pull_request.id = %q, want %q", out.PR.ID, "42") } - if out.PullRequest.ID != "42" { - t.Errorf("pull_request.id = %q, want %q", out.PullRequest.ID, "42") + if out.PR.URL != "https://github.com/owner/repo/pull/42" { + t.Errorf("pull_request.url = %q", out.PR.URL) } - if len(out.ChangedFiles) != 2 { - t.Errorf("changed_files length = %d, want 2", len(out.ChangedFiles)) + if len(out.Files) != 2 { + t.Errorf("changed_files length = %d, want 2", len(out.Files)) } if out.Analysis.ChangeType != "feature" { t.Errorf("analysis.change_type = %q, want %q", out.Analysis.ChangeType, "feature") @@ -100,40 +100,14 @@ func TestFormatJSONStructure(t *testing.T) { } } -func TestFormatJSONNilSlices(t *testing.T) { - pr := domain.PullRequest{ - Repository: "o/r", - ID: "1", - } - result := domain.AnalysisResult{ - ChangeType: domain.ChangeTypeBugfix, - RiskLevel: domain.RiskLevelLow, - // All slice fields are nil. - } - +func TestFormatJSONOmitsDescription(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatJSON(&buf, "github", pr, result) - if err != nil { + if err := formatter.FormatJSON(&buf, testResult()); err != nil { t.Fatalf("FormatJSON: %v", err) } - var out formatter.Output - if err := json.Unmarshal(buf.Bytes(), &out); err != nil { - t.Fatalf("unmarshal: %v", err) - } - - // Nil slices should become empty arrays, not null. - if out.Analysis.AffectedAreas == nil { - t.Error("affected_areas is null, want empty array") - } - if out.Analysis.ReviewAxes == nil { - t.Error("review_axes is null, want empty array") - } - if out.Analysis.RelatedFiles == nil { - t.Error("related_files is null, want empty array") - } - if out.Analysis.Warnings == nil { - t.Error("warnings is null, want empty array") + if bytes.Contains(buf.Bytes(), []byte("description")) { + t.Errorf("output should not contain 'description' field, got: %s", buf.String()) } } diff --git a/internal/formatter/markdown.go b/internal/formatter/markdown.go index bafaff7..d960d85 100644 --- a/internal/formatter/markdown.go +++ b/internal/formatter/markdown.go @@ -5,63 +5,62 @@ import ( "io" "strings" - "github.com/hidetzu/prism/internal/domain" + "github.com/hidetzu/prism/pkg/prism" ) -// FormatMarkdown writes the analysis result as human-readable Markdown to w. -func FormatMarkdown(w io.Writer, providerName string, pr domain.PullRequest, result domain.AnalysisResult) error { +// FormatMarkdown writes a prism.Result as human-readable Markdown to w. +func FormatMarkdown(w io.Writer, result prism.Result) error { var b strings.Builder // Header - fmt.Fprintf(&b, "# %s\n\n", pr.Title) + fmt.Fprintf(&b, "# %s\n\n", result.PR.Title) // Metadata b.WriteString("## Pull Request\n\n") fmt.Fprintf(&b, "| Field | Value |\n") fmt.Fprintf(&b, "|-------|-------|\n") - fmt.Fprintf(&b, "| Repository | %s |\n", pr.Repository) - fmt.Fprintf(&b, "| PR | #%s |\n", pr.ID) - fmt.Fprintf(&b, "| Author | %s |\n", pr.Author) - fmt.Fprintf(&b, "| Branch | %s -> %s |\n", pr.SourceBranch, pr.TargetBranch) - fmt.Fprintf(&b, "| Provider | %s |\n", providerName) - b.WriteString("\n") - - if pr.Description != "" { - b.WriteString("### Description\n\n") - b.WriteString(pr.Description) - b.WriteString("\n\n") + fmt.Fprintf(&b, "| Repository | %s |\n", result.PR.Repository) + fmt.Fprintf(&b, "| PR | #%s |\n", result.PR.ID) + fmt.Fprintf(&b, "| Author | %s |\n", result.PR.Author) + fmt.Fprintf(&b, "| Branch | %s -> %s |\n", result.PR.SourceBranch, result.PR.TargetBranch) + fmt.Fprintf(&b, "| Provider | %s |\n", result.PR.Provider) + if result.PR.URL != "" { + fmt.Fprintf(&b, "| URL | %s |\n", result.PR.URL) } + b.WriteString("\n") // Analysis b.WriteString("## Analysis\n\n") - fmt.Fprintf(&b, "- **Change Type:** %s\n", result.ChangeType) - fmt.Fprintf(&b, "- **Risk Level:** %s\n", result.RiskLevel) - fmt.Fprintf(&b, "- **Summary:** %s\n", result.Summary) + fmt.Fprintf(&b, "- **Change Type:** %s\n", result.Analysis.ChangeType) + fmt.Fprintf(&b, "- **Risk Level:** %s\n", result.Analysis.RiskLevel) + if result.Analysis.Summary != "" { + fmt.Fprintf(&b, "- **Summary:** %s\n", result.Analysis.Summary) + } b.WriteString("\n") // Review Axes - if len(result.ReviewAxes) > 0 { + if len(result.Analysis.ReviewAxes) > 0 { b.WriteString("### Review Axes\n\n") - for _, axis := range result.ReviewAxes { + for _, axis := range result.Analysis.ReviewAxes { fmt.Fprintf(&b, "- %s\n", axis) } b.WriteString("\n") } // Affected Areas - if len(result.AffectedAreas) > 0 { + if len(result.Analysis.AffectedAreas) > 0 { b.WriteString("### Affected Areas\n\n") - for _, area := range result.AffectedAreas { + for _, area := range result.Analysis.AffectedAreas { fmt.Fprintf(&b, "- %s\n", area) } b.WriteString("\n") } // Warnings - if len(result.Warnings) > 0 { + if len(result.Analysis.Warnings) > 0 { b.WriteString("### Warnings\n\n") - for _, w := range result.Warnings { - fmt.Fprintf(&b, "- %s\n", w) + for _, warn := range result.Analysis.Warnings { + fmt.Fprintf(&b, "- %s\n", warn) } b.WriteString("\n") } @@ -70,7 +69,7 @@ func FormatMarkdown(w io.Writer, providerName string, pr domain.PullRequest, res b.WriteString("## Changed Files\n\n") fmt.Fprintf(&b, "| File | Status | +/- | Language |\n") fmt.Fprintf(&b, "|------|--------|-----|----------|\n") - for _, f := range pr.ChangedFiles { + for _, f := range result.Files { flags := fileFlags(f) name := f.Path if flags != "" { @@ -85,7 +84,7 @@ func FormatMarkdown(w io.Writer, providerName string, pr domain.PullRequest, res return err } -func fileFlags(f domain.ChangedFile) string { +func fileFlags(f prism.ChangedFile) string { var flags []string if f.IsTest { flags = append(flags, "test") diff --git a/internal/formatter/markdown_test.go b/internal/formatter/markdown_test.go index 435add9..ca93760 100644 --- a/internal/formatter/markdown_test.go +++ b/internal/formatter/markdown_test.go @@ -5,26 +5,23 @@ import ( "strings" "testing" - "github.com/hidetzu/prism/internal/domain" "github.com/hidetzu/prism/internal/formatter" + "github.com/hidetzu/prism/pkg/prism" ) func TestFormatMarkdownContainsTitle(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatMarkdown(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatMarkdown(&buf, testResult()); err != nil { t.Fatalf("FormatMarkdown: %v", err) } - out := buf.String() - if !strings.Contains(out, "# Add OAuth2 login") { + if !strings.Contains(buf.String(), "# Add OAuth2 login") { t.Error("output missing PR title header") } } func TestFormatMarkdownContainsMetadata(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatMarkdown(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatMarkdown(&buf, testResult()); err != nil { t.Fatalf("FormatMarkdown: %v", err) } out := buf.String() @@ -34,6 +31,7 @@ func TestFormatMarkdownContainsMetadata(t *testing.T) { "dev", "feature/oauth", "main", + "https://github.com/owner/repo/pull/42", } for _, c := range checks { if !strings.Contains(out, c) { @@ -44,8 +42,7 @@ func TestFormatMarkdownContainsMetadata(t *testing.T) { func TestFormatMarkdownContainsAnalysis(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatMarkdown(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatMarkdown(&buf, testResult()); err != nil { t.Fatalf("FormatMarkdown: %v", err) } out := buf.String() @@ -62,8 +59,7 @@ func TestFormatMarkdownContainsAnalysis(t *testing.T) { func TestFormatMarkdownChangedFiles(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatMarkdown(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatMarkdown(&buf, testResult()); err != nil { t.Fatalf("FormatMarkdown: %v", err) } out := buf.String() @@ -77,33 +73,36 @@ func TestFormatMarkdownChangedFiles(t *testing.T) { func TestFormatMarkdownWarnings(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatMarkdown(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatMarkdown(&buf, testResult()); err != nil { t.Fatalf("FormatMarkdown: %v", err) } - out := buf.String() - if !strings.Contains(out, "New authentication flow") { + if !strings.Contains(buf.String(), "New authentication flow") { t.Error("output missing warning") } } -func TestFormatMarkdownNoDescription(t *testing.T) { - pr := domain.PullRequest{ - Repository: "o/r", - ID: "1", - Title: "Quick fix", - } - result := domain.AnalysisResult{ - ChangeType: domain.ChangeTypeBugfix, - RiskLevel: domain.RiskLevelLow, +func TestFormatMarkdownMinimalResult(t *testing.T) { + result := prism.Result{ + PR: prism.PRInfo{ + Provider: "github", + Repository: "o/r", + ID: "1", + Title: "Quick fix", + }, + Analysis: prism.AnalysisResult{ + ChangeType: "bugfix", + RiskLevel: "low", + }, } var buf bytes.Buffer - err := formatter.FormatMarkdown(&buf, "github", pr, result) - if err != nil { + if err := formatter.FormatMarkdown(&buf, result); err != nil { t.Fatalf("FormatMarkdown: %v", err) } out := buf.String() - if strings.Contains(out, "### Description") { - t.Error("should not include Description section when description is empty") + if !strings.Contains(out, "# Quick fix") { + t.Error("output missing title") + } + if !strings.Contains(out, "bugfix") { + t.Error("output missing change type") } } diff --git a/internal/formatter/testdata/basic.golden.json b/internal/formatter/testdata/basic.golden.json index 386bd8c..9904939 100644 --- a/internal/formatter/testdata/basic.golden.json +++ b/internal/formatter/testdata/basic.golden.json @@ -1,36 +1,14 @@ { - "provider": "github", "pull_request": { + "provider": "github", "repository": "owner/repo", "id": "42", "title": "Add OAuth2 login", "author": "dev", "source_branch": "feature/oauth", "target_branch": "main", - "description": "Adds OAuth2 login flow" + "url": "https://github.com/owner/repo/pull/42" }, - "changed_files": [ - { - "path": "internal/auth/oauth.go", - "status": "added", - "additions": 120, - "deletions": 0, - "language": "Go", - "is_test": false, - "is_config": false, - "is_generated": false - }, - { - "path": "internal/auth/oauth_test.go", - "status": "added", - "additions": 80, - "deletions": 0, - "language": "Go", - "is_test": true, - "is_config": false, - "is_generated": false - } - ], "analysis": { "change_type": "feature", "risk_level": "medium", @@ -41,10 +19,26 @@ "security", "error handling" ], - "related_files": [], "warnings": [ "New authentication flow" ], "summary": "Adds OAuth2 login flow with tests" - } + }, + "changed_files": [ + { + "path": "internal/auth/oauth.go", + "status": "added", + "additions": 120, + "deletions": 0, + "language": "Go" + }, + { + "path": "internal/auth/oauth_test.go", + "status": "added", + "additions": 80, + "deletions": 0, + "language": "Go", + "is_test": true + } + ] } diff --git a/internal/formatter/text.go b/internal/formatter/text.go index d4e7cec..83012c7 100644 --- a/internal/formatter/text.go +++ b/internal/formatter/text.go @@ -5,47 +5,55 @@ import ( "io" "strings" - "github.com/hidetzu/prism/internal/domain" + "github.com/hidetzu/prism/pkg/prism" ) -// FormatText writes the analysis result as plain text to w. -func FormatText(w io.Writer, providerName string, pr domain.PullRequest, result domain.AnalysisResult) error { +// FormatText writes a prism.Result as plain text to w. +func FormatText(w io.Writer, result prism.Result) error { write := func(format string, args ...interface{}) error { _, err := fmt.Fprintf(w, format, args...) return err } - if err := write("%s\n", pr.Title); err != nil { + if err := write("%s\n", result.PR.Title); err != nil { return err } - if err := write("%s\n\n", strings.Repeat("=", len(pr.Title))); err != nil { + if err := write("%s\n\n", strings.Repeat("=", len(result.PR.Title))); err != nil { return err } - if err := write("Repository: %s\n", pr.Repository); err != nil { + if err := write("Repository: %s\n", result.PR.Repository); err != nil { return err } - if err := write("PR: #%s\n", pr.ID); err != nil { + if err := write("PR: #%s\n", result.PR.ID); err != nil { return err } - if err := write("Author: %s\n", pr.Author); err != nil { + if err := write("Author: %s\n", result.PR.Author); err != nil { return err } - if err := write("Branch: %s -> %s\n", pr.SourceBranch, pr.TargetBranch); err != nil { + if err := write("Branch: %s -> %s\n", result.PR.SourceBranch, result.PR.TargetBranch); err != nil { return err } - if err := write("Provider: %s\n\n", providerName); err != nil { + if err := write("Provider: %s\n", result.PR.Provider); err != nil { + return err + } + if result.PR.URL != "" { + if err := write("URL: %s\n", result.PR.URL); err != nil { + return err + } + } + if err := write("\n"); err != nil { return err } - if err := write("Change Type: %s\n", result.ChangeType); err != nil { + if err := write("Change Type: %s\n", result.Analysis.ChangeType); err != nil { return err } - if err := write("Risk Level: %s\n", result.RiskLevel); err != nil { + if err := write("Risk Level: %s\n", result.Analysis.RiskLevel); err != nil { return err } - if result.Summary != "" { - if err := write("Summary: %s\n", result.Summary); err != nil { + if result.Analysis.Summary != "" { + if err := write("Summary: %s\n", result.Analysis.Summary); err != nil { return err } } @@ -53,11 +61,11 @@ func FormatText(w io.Writer, providerName string, pr domain.PullRequest, result return err } - if len(result.ReviewAxes) > 0 { + if len(result.Analysis.ReviewAxes) > 0 { if err := write("Review Axes:\n"); err != nil { return err } - for _, axis := range result.ReviewAxes { + for _, axis := range result.Analysis.ReviewAxes { if err := write(" - %s\n", axis); err != nil { return err } @@ -67,11 +75,11 @@ func FormatText(w io.Writer, providerName string, pr domain.PullRequest, result } } - if len(result.AffectedAreas) > 0 { + if len(result.Analysis.AffectedAreas) > 0 { if err := write("Affected Areas:\n"); err != nil { return err } - for _, area := range result.AffectedAreas { + for _, area := range result.Analysis.AffectedAreas { if err := write(" - %s\n", area); err != nil { return err } @@ -81,12 +89,12 @@ func FormatText(w io.Writer, providerName string, pr domain.PullRequest, result } } - if len(result.Warnings) > 0 { + if len(result.Analysis.Warnings) > 0 { if err := write("Warnings:\n"); err != nil { return err } - for _, w := range result.Warnings { - if err := write(" - %s\n", w); err != nil { + for _, warn := range result.Analysis.Warnings { + if err := write(" - %s\n", warn); err != nil { return err } } @@ -98,7 +106,7 @@ func FormatText(w io.Writer, providerName string, pr domain.PullRequest, result if err := write("Changed Files:\n"); err != nil { return err } - for _, f := range pr.ChangedFiles { + for _, f := range result.Files { flags := fileFlags(f) if flags != "" { flags = " " + flags diff --git a/internal/formatter/text_test.go b/internal/formatter/text_test.go index 0f23c9c..1be6e72 100644 --- a/internal/formatter/text_test.go +++ b/internal/formatter/text_test.go @@ -10,20 +10,17 @@ import ( func TestFormatTextContainsTitle(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatText(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatText(&buf, testResult()); err != nil { t.Fatalf("FormatText: %v", err) } - out := buf.String() - if !strings.Contains(out, "Add OAuth2 login") { + if !strings.Contains(buf.String(), "Add OAuth2 login") { t.Error("output missing PR title") } } func TestFormatTextContainsMetadata(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatText(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatText(&buf, testResult()); err != nil { t.Fatalf("FormatText: %v", err) } out := buf.String() @@ -34,6 +31,7 @@ func TestFormatTextContainsMetadata(t *testing.T) { "feature/oauth", "main", "github", + "https://github.com/owner/repo/pull/42", } for _, c := range checks { if !strings.Contains(out, c) { @@ -44,8 +42,7 @@ func TestFormatTextContainsMetadata(t *testing.T) { func TestFormatTextContainsAnalysis(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatText(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatText(&buf, testResult()); err != nil { t.Fatalf("FormatText: %v", err) } out := buf.String() @@ -62,8 +59,7 @@ func TestFormatTextContainsAnalysis(t *testing.T) { func TestFormatTextChangedFiles(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatText(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatText(&buf, testResult()); err != nil { t.Fatalf("FormatText: %v", err) } out := buf.String() @@ -77,12 +73,10 @@ func TestFormatTextChangedFiles(t *testing.T) { func TestFormatTextWarnings(t *testing.T) { var buf bytes.Buffer - err := formatter.FormatText(&buf, "github", testPR(), testResult()) - if err != nil { + if err := formatter.FormatText(&buf, testResult()); err != nil { t.Fatalf("FormatText: %v", err) } - out := buf.String() - if !strings.Contains(out, "New authentication flow") { + if !strings.Contains(buf.String(), "New authentication flow") { t.Error("output missing warning") } } diff --git a/internal/formatter/types.go b/internal/formatter/types.go deleted file mode 100644 index a290637..0000000 --- a/internal/formatter/types.go +++ /dev/null @@ -1,45 +0,0 @@ -package formatter - -// Output types with JSON tags matching docs/json-schema.md. - -// Output is the top-level JSON output structure. -type Output struct { - Provider string `json:"provider"` - PullRequest PullRequestOutput `json:"pull_request"` - ChangedFiles []ChangedFileOutput `json:"changed_files"` - Analysis AnalysisOutput `json:"analysis"` -} - -// PullRequestOutput represents PR metadata in the output. -type PullRequestOutput struct { - Repository string `json:"repository"` - ID string `json:"id"` - Title string `json:"title"` - Author string `json:"author"` - SourceBranch string `json:"source_branch"` - TargetBranch string `json:"target_branch"` - Description string `json:"description"` -} - -// ChangedFileOutput represents a changed file in the output. -type ChangedFileOutput struct { - Path string `json:"path"` - Status string `json:"status"` - Additions int `json:"additions"` - Deletions int `json:"deletions"` - Language string `json:"language"` - IsTest bool `json:"is_test"` - IsConfig bool `json:"is_config"` - IsGenerated bool `json:"is_generated"` -} - -// AnalysisOutput represents analysis results in the output. -type AnalysisOutput struct { - ChangeType string `json:"change_type"` - RiskLevel string `json:"risk_level"` - AffectedAreas []string `json:"affected_areas"` - ReviewAxes []string `json:"review_axes"` - RelatedFiles []string `json:"related_files"` - Warnings []string `json:"warnings"` - Summary string `json:"summary"` -} diff --git a/internal/provider/github/client.go b/internal/provider/github/client.go index 0712ae6..6909b69 100644 --- a/internal/provider/github/client.go +++ b/internal/provider/github/client.go @@ -201,4 +201,3 @@ func mapFileStatus(status string) domain.FileStatus { return domain.FileStatusModified } } - diff --git a/internal/provider/github/parser_test.go b/internal/provider/github/parser_test.go index 1a438e7..101a355 100644 --- a/internal/provider/github/parser_test.go +++ b/internal/provider/github/parser_test.go @@ -8,12 +8,12 @@ import ( func TestParse(t *testing.T) { tests := []struct { - name string - input string + name string + input string wantOwner string wantRepo string wantNum int - wantErr bool + wantErr bool }{ { name: "valid PR URL", diff --git a/internal/provider/registry.go b/internal/provider/registry.go index 7e53b8d..cd795f9 100644 --- a/internal/provider/registry.go +++ b/internal/provider/registry.go @@ -2,6 +2,7 @@ package provider import ( "fmt" + "net/http" "net/url" "os/exec" @@ -12,7 +13,8 @@ import ( // Registry manages built-in and plugin providers. type Registry struct { - githubToken string + githubToken string + githubBaseURL string // empty = use the real GitHub API } // NewRegistry creates a provider registry. @@ -20,6 +22,13 @@ func NewRegistry(githubToken string) *Registry { return &Registry{githubToken: githubToken} } +// NewRegistryWithGitHubBaseURL creates a registry that points the GitHub +// provider at a custom base URL. Used by tests to redirect API calls to a +// local httptest server. +func NewRegistryWithGitHubBaseURL(githubToken, baseURL string) *Registry { + return &Registry{githubToken: githubToken, githubBaseURL: baseURL} +} + // Resolve returns a Provider for the given PR URL. // If providerName is non-empty, it uses that provider directly. // If providerName is empty, it auto-detects the provider from the URL. @@ -34,6 +43,9 @@ func (r *Registry) Resolve(providerName string, prURL string) (Provider, error) switch providerName { case "github": + if r.githubBaseURL != "" { + return github.NewProviderWithClient(http.DefaultClient, r.githubToken, r.githubBaseURL), nil + } return github.NewProvider(r.githubToken), nil default: return r.resolvePlugin(providerName, prURL) diff --git a/internal/provider/registry_test.go b/internal/provider/registry_test.go index 83e9716..3aa1858 100644 --- a/internal/provider/registry_test.go +++ b/internal/provider/registry_test.go @@ -65,3 +65,22 @@ func TestResolvePluginNotFound(t *testing.T) { t.Fatal("expected error for missing plugin binary") } } + +func TestNewRegistryWithGitHubBaseURL(t *testing.T) { + reg := NewRegistryWithGitHubBaseURL("test-token", "http://localhost:9999") + if reg.githubToken != "test-token" { + t.Errorf("githubToken = %q", reg.githubToken) + } + if reg.githubBaseURL != "http://localhost:9999" { + t.Errorf("githubBaseURL = %q", reg.githubBaseURL) + } + + // Should still resolve to a github provider for github.com URLs. + p, err := reg.Resolve("", "https://github.com/o/r/pull/1") + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if p == nil { + t.Fatal("provider is nil") + } +} diff --git a/internal/usecase/analyze.go b/internal/usecase/analyze.go deleted file mode 100644 index a70a9d0..0000000 --- a/internal/usecase/analyze.go +++ /dev/null @@ -1,42 +0,0 @@ -package usecase - -import ( - "context" - "fmt" - "io" - - "github.com/hidetzu/prism/internal/analyzer" - "github.com/hidetzu/prism/internal/classifier" - "github.com/hidetzu/prism/internal/domain" - "github.com/hidetzu/prism/internal/formatter" - "github.com/hidetzu/prism/internal/provider" -) - -// AnalyzeOptions holds options for the Analyze use case. -type AnalyzeOptions struct { - Format string // "json", "markdown", "text" -} - -// Analyze fetches a PR, classifies and analyzes it, and writes formatted output. -func Analyze(ctx context.Context, p provider.Provider, ref domain.PRRef, opts AnalyzeOptions, w io.Writer) error { - if err := ValidateAnalyzeOptions(opts); err != nil { - return err - } - - pr, err := p.FetchPullRequest(ctx, ref) - if err != nil { - return fmt.Errorf("%w: %v", domain.ErrProvider, err) - } - - changeType := classifier.Classify(pr) - result := analyzer.Analyze(pr, changeType) - - switch opts.Format { - case "markdown": - return formatter.FormatMarkdown(w, ref.Provider, pr, result) - case "text": - return formatter.FormatText(w, ref.Provider, pr, result) - default: - return formatter.FormatJSON(w, ref.Provider, pr, result) - } -} diff --git a/internal/usecase/analyze_test.go b/internal/usecase/analyze_test.go deleted file mode 100644 index 94d5037..0000000 --- a/internal/usecase/analyze_test.go +++ /dev/null @@ -1,128 +0,0 @@ -package usecase_test - -import ( - "bytes" - "context" - "encoding/json" - "strings" - "testing" - - "github.com/hidetzu/prism/internal/domain" - "github.com/hidetzu/prism/internal/formatter" - "github.com/hidetzu/prism/internal/usecase" -) - -func TestAnalyzeJSON(t *testing.T) { - p := &mockProvider{pr: samplePR()} - var buf bytes.Buffer - - err := usecase.Analyze(context.Background(), p, sampleRef(), usecase.AnalyzeOptions{ - Format: "json", - }, &buf) - if err != nil { - t.Fatalf("Analyze: %v", err) - } - - var out formatter.Output - if err := json.Unmarshal(buf.Bytes(), &out); err != nil { - t.Fatalf("unmarshal: %v", err) - } - - if out.Provider != "github" { - t.Errorf("provider = %q, want %q", out.Provider, "github") - } - if out.PullRequest.Title != "Fix null pointer in handler" { - t.Errorf("title = %q", out.PullRequest.Title) - } - if out.Analysis.ChangeType != "bugfix" { - t.Errorf("change_type = %q, want %q", out.Analysis.ChangeType, "bugfix") - } -} - -func TestAnalyzeMarkdown(t *testing.T) { - p := &mockProvider{pr: samplePR()} - var buf bytes.Buffer - - err := usecase.Analyze(context.Background(), p, sampleRef(), usecase.AnalyzeOptions{ - Format: "markdown", - }, &buf) - if err != nil { - t.Fatalf("Analyze: %v", err) - } - - out := buf.String() - if len(out) == 0 { - t.Error("empty markdown output") - } -} - -func TestAnalyzeText(t *testing.T) { - p := &mockProvider{pr: samplePR()} - var buf bytes.Buffer - - err := usecase.Analyze(context.Background(), p, sampleRef(), usecase.AnalyzeOptions{ - Format: "text", - }, &buf) - if err != nil { - t.Fatalf("Analyze text: %v", err) - } - - out := buf.String() - if !strings.Contains(out, "Fix null pointer in handler") { - t.Error("text output missing title") - } - if !strings.Contains(out, "bugfix") { - t.Error("text output missing change type") - } -} - -func TestAnalyzeInvalidFormat(t *testing.T) { - p := &mockProvider{pr: samplePR()} - var buf bytes.Buffer - - err := usecase.Analyze(context.Background(), p, sampleRef(), usecase.AnalyzeOptions{ - Format: "xml", - }, &buf) - if err == nil { - t.Fatal("expected error for invalid format") - } -} - -func TestAnalyzeProviderError(t *testing.T) { - p := &mockProvider{err: errMock} - var buf bytes.Buffer - - err := usecase.Analyze(context.Background(), p, sampleRef(), usecase.AnalyzeOptions{}, &buf) - if err == nil { - t.Fatal("expected error") - } -} - -func TestAnalyzeClassifiesDocsOnly(t *testing.T) { - pr := domain.PullRequest{ - Repository: "o/r", - ID: "1", - Title: "Update docs", - ChangedFiles: []domain.ChangedFile{ - {Path: "README.md", Status: domain.FileStatusModified, Additions: 5}, - }, - } - p := &mockProvider{pr: pr} - var buf bytes.Buffer - - err := usecase.Analyze(context.Background(), p, sampleRef(), usecase.AnalyzeOptions{Format: "json"}, &buf) - if err != nil { - t.Fatalf("Analyze: %v", err) - } - - var out formatter.Output - if err := json.Unmarshal(buf.Bytes(), &out); err != nil { - t.Fatalf("unmarshal: %v", err) - } - if out.Analysis.ChangeType != "docs-only" { - t.Errorf("change_type = %q, want %q", out.Analysis.ChangeType, "docs-only") - } - if out.Analysis.RiskLevel != "low" { - t.Errorf("risk_level = %q, want %q", out.Analysis.RiskLevel, "low") - } -} diff --git a/internal/usecase/fetch.go b/internal/usecase/fetch.go index eae1822..41bdff2 100644 --- a/internal/usecase/fetch.go +++ b/internal/usecase/fetch.go @@ -8,7 +8,6 @@ import ( "github.com/hidetzu/prism/internal/domain" "github.com/hidetzu/prism/internal/provider" - ) // FetchOptions holds options for the Fetch use case. diff --git a/internal/usecase/validate.go b/internal/usecase/validate.go index c67a4f6..c758a51 100644 --- a/internal/usecase/validate.go +++ b/internal/usecase/validate.go @@ -6,10 +6,6 @@ import ( "github.com/hidetzu/prism/internal/domain" ) -var validAnalyzeFormats = map[string]bool{ - "json": true, "markdown": true, "text": true, -} - var validPromptModes = map[string]bool{ "light": true, "detailed": true, "cross": true, } @@ -26,14 +22,6 @@ var validLangs = map[string]bool{ "en": true, "ja": true, } -// ValidateAnalyzeOptions returns an error if options are invalid. -func ValidateAnalyzeOptions(opts AnalyzeOptions) error { - if opts.Format != "" && !validAnalyzeFormats[opts.Format] { - return fmt.Errorf("%w: invalid format %q: must be json, markdown, or text", domain.ErrInvalidArgs, opts.Format) - } - return nil -} - // ValidatePromptOptions returns an error if options are invalid. func ValidatePromptOptions(opts PromptOptions) error { if opts.Mode != "" && !validPromptModes[opts.Mode] { diff --git a/internal/usecase/validate_test.go b/internal/usecase/validate_test.go index 9280b61..07c86e2 100644 --- a/internal/usecase/validate_test.go +++ b/internal/usecase/validate_test.go @@ -1,38 +1,11 @@ package usecase_test import ( - "errors" "testing" - "github.com/hidetzu/prism/internal/domain" "github.com/hidetzu/prism/internal/usecase" ) -func TestValidateAnalyzeOptions(t *testing.T) { - tests := []struct { - format string - wantErr bool - }{ - {"json", false}, - {"markdown", false}, - {"text", false}, - {"", false}, - {"xml", true}, - } - for _, tt := range tests { - err := usecase.ValidateAnalyzeOptions(usecase.AnalyzeOptions{Format: tt.format}) - if tt.wantErr && err == nil { - t.Errorf("format %q: expected error", tt.format) - } - if !tt.wantErr && err != nil { - t.Errorf("format %q: unexpected error: %v", tt.format, err) - } - if tt.wantErr && err != nil && !errors.Is(err, domain.ErrInvalidArgs) { - t.Errorf("format %q: error should wrap ErrInvalidArgs", tt.format) - } - } -} - func TestValidatePromptOptions(t *testing.T) { tests := []struct { name string diff --git a/pkg/prism/integration_test.go b/pkg/prism/integration_test.go new file mode 100644 index 0000000..baa0a31 --- /dev/null +++ b/pkg/prism/integration_test.go @@ -0,0 +1,204 @@ +package prism + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/hidetzu/prism/internal/provider" +) + +// setupGitHubMock spins up an httptest server that mimics the GitHub REST API +// for a single PR. It returns the server URL the registry should target. +func setupGitHubMock(t *testing.T) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + + mux.HandleFunc("GET /repos/owner/repo/pulls/1", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, `{ + "number": 1, + "title": "Add retry handling", + "body": "Adds retry logic for transient failures", + "user": {"login": "dev"}, + "head": {"ref": "feature/retry"}, + "base": {"ref": "main", "repo": {"full_name": "owner/repo"}} + }`) + }) + + mux.HandleFunc("GET /repos/owner/repo/pulls/1/files", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, `[ + { + "filename": "internal/payment/client.go", + "status": "modified", + "additions": 45, + "deletions": 3, + "patch": "@@ -1,3 +1,45 @@" + }, + { + "filename": "internal/payment/client_test.go", + "status": "added", + "additions": 80, + "deletions": 0, + "patch": "@@ -0,0 +1,80 @@" + } + ]`) + }) + + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +// withMockGitHub overrides newRegistry so that the github provider talks to +// the given mock server. The override is reverted on test cleanup. +func withMockGitHub(t *testing.T, server *httptest.Server) { + t.Helper() + previous := newRegistry + newRegistry = func(githubToken string) *provider.Registry { + return provider.NewRegistryWithGitHubBaseURL(githubToken, server.URL) + } + t.Cleanup(func() { newRegistry = previous }) +} + +func TestAnalyzeHappyPath(t *testing.T) { + server := setupGitHubMock(t) + withMockGitHub(t, server) + + result, err := Analyze(context.Background(), AnalyzeOptions{ + PRURL: "https://github.com/owner/repo/pull/1", + GitHubToken: "test-token", + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + + // PR metadata + if result.PR.Provider != "github" { + t.Errorf("PR.Provider = %q, want github", result.PR.Provider) + } + if result.PR.Repository != "owner/repo" { + t.Errorf("PR.Repository = %q, want owner/repo", result.PR.Repository) + } + if result.PR.ID != "1" { + t.Errorf("PR.ID = %q, want 1", result.PR.ID) + } + if result.PR.Title != "Add retry handling" { + t.Errorf("PR.Title = %q", result.PR.Title) + } + if result.PR.URL != "https://github.com/owner/repo/pull/1" { + t.Errorf("PR.URL = %q", result.PR.URL) + } + + // Analysis was actually run + if result.Analysis.ChangeType == "" { + t.Error("Analysis.ChangeType is empty") + } + if result.Analysis.RiskLevel == "" { + t.Error("Analysis.RiskLevel is empty") + } + + // Files come through with enrichment from internal/fileutil + if len(result.Files) != 2 { + t.Fatalf("Files len = %d, want 2", len(result.Files)) + } + if result.Files[0].Language != "Go" { + t.Errorf("Files[0].Language = %q, want Go", result.Files[0].Language) + } + if !result.Files[1].IsTest { + t.Error("Files[1].IsTest = false, want true (test file detection)") + } + + // IncludePatches=false (default) → patches must be empty + for i, f := range result.Files { + if f.Patch != "" { + t.Errorf("Files[%d].Patch = %q, want empty (IncludePatches=false)", i, f.Patch) + } + } +} + +func TestAnalyzeHappyPathIncludePatches(t *testing.T) { + server := setupGitHubMock(t) + withMockGitHub(t, server) + + result, err := Analyze(context.Background(), AnalyzeOptions{ + PRURL: "https://github.com/owner/repo/pull/1", + GitHubToken: "test-token", + IncludePatches: true, + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + + if len(result.Files) != 2 { + t.Fatalf("Files len = %d, want 2", len(result.Files)) + } + for i, f := range result.Files { + if f.Patch == "" { + t.Errorf("Files[%d].Patch is empty, want non-empty (IncludePatches=true)", i) + } + } +} + +func TestAnalyzeExplicitProvider(t *testing.T) { + server := setupGitHubMock(t) + withMockGitHub(t, server) + + result, err := Analyze(context.Background(), AnalyzeOptions{ + PRURL: "https://github.com/owner/repo/pull/1", + Provider: "github", + GitHubToken: "test-token", + }) + if err != nil { + t.Fatalf("Analyze: %v", err) + } + if result.PR.Provider != "github" { + t.Errorf("PR.Provider = %q, want github", result.PR.Provider) + } +} + +func TestAnalyzeUpstreamFailure(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + t.Cleanup(server.Close) + withMockGitHub(t, server) + + _, err := Analyze(context.Background(), AnalyzeOptions{ + PRURL: "https://github.com/owner/repo/pull/1", + GitHubToken: "test-token", + }) + if err == nil { + t.Fatal("expected error for upstream failure") + } + if !errors.Is(err, ErrUpstreamFailure) { + t.Errorf("expected ErrUpstreamFailure, got %v", err) + } +} + +func TestPromptHappyPath(t *testing.T) { + server := setupGitHubMock(t) + withMockGitHub(t, server) + + out, err := Prompt(context.Background(), AnalyzeOptions{ + PRURL: "https://github.com/owner/repo/pull/1", + GitHubToken: "test-token", + Mode: "light", + Language: "en", + }) + if err != nil { + t.Fatalf("Prompt: %v", err) + } + if out == "" { + t.Fatal("Prompt returned empty string") + } + // Light mode should include the PR title in the user prompt. + // We don't assert exact format, just sanity-check that something useful came out. + if len(out) < 50 { + t.Errorf("Prompt output too short: %q", out) + } +} diff --git a/pkg/prism/prism.go b/pkg/prism/prism.go index 9b862af..ef1ac68 100644 --- a/pkg/prism/prism.go +++ b/pkg/prism/prism.go @@ -181,9 +181,16 @@ func Prompt(ctx context.Context, opts AnalyzeOptions) (string, error) { return fmt.Sprintf("%s\n\n---\n\n%s", bundle.SystemPrompt, bundle.UserPrompt), nil } +// newRegistry is the registry constructor used by Analyze and Prompt. +// Tests in this package may override it to inject a registry that targets +// a local httptest server. This is the only test seam exposed inside pkg/prism. +var newRegistry = func(githubToken string) *provider.Registry { + return provider.NewRegistry(githubToken) +} + // resolveProvider builds the provider and PRRef from AnalyzeOptions. func resolveProvider(opts AnalyzeOptions) (provider.Provider, domain.PRRef, error) { - reg := provider.NewRegistry(opts.GitHubToken) + reg := newRegistry(opts.GitHubToken) p, err := reg.Resolve(opts.Provider, opts.PRURL) if err != nil { // Registry errors are either unknown host (invalid) or plugin-not-found (unsupported).