fix(health): address code review issues 3-6,8

shortcuts/health/api.go

@@ -3,15 +3,20 @@ package health
 import (
 	"fmt"
 	"net/url"
+	"os"
 
 	"github.com/gitlink-org/gitlink-cli/shortcuts/common"
 )
 
+// v1RepoPath constructs the v1 API path for a repository.
+// Issue operations use the v1 API (/v1/{owner}/{repo}) to match shortcuts/issue conventions,
+// while PR operations use the v2 API via ctx.RepoPath() (/{owner}/{repo}/pulls) to match shortcuts/pr conventions.
+
 func v1RepoPath(owner, repo string) string {
 	return fmt.Sprintf("/v1/%s/%s", owner, repo)
 }
 
-func fetchPRListPage(ctx *common.RuntimeContext, state string, page, limit int) ([]interface{}, map[string]interface{}) {
+func fetchPRListPage(ctx *common.RuntimeContext, state string, page, limit int) ([]interface{}, error) {
 	q := url.Values{}
 	q.Set("page", fmt.Sprintf("%d", page))
 	q.Set("limit", fmt.Sprintf("%d", limit))
@@ -20,22 +25,64 @@ func fetchPRListPage(ctx *common.RuntimeContext, state string, page, limit int)
 	}
 	env, err := ctx.CallAPIWithQuery("GET", ctx.RepoPath()+"/pulls", q)
 	if err != nil {
-		fmt.Printf("  CLI error: pr +list state=%s page=%d: %v\n", state, page, err)
+		fmt.Fprintf(os.Stderr, "  CLI error: pr +list state=%s page=%d: %v\n", state, page, err)
-		return nil, nil
+		return nil, err
 	}
 	if !env.OK {
-		fmt.Printf("  API error: pr +list state=%s page=%d\n", state, page)
+		err := fmt.Errorf("API error: pr +list state=%s page=%d", state, page)
-		return nil, nil
+		fmt.Fprintf(os.Stderr, "  %v\n", err)
+		return nil, err
 	}
 	data, ok := env.Data.(map[string]interface{})
 	if !ok {
-		return nil, nil
+		return nil, fmt.Errorf("unexpected response type for pr +list")
 	}
 	issues, _ := data["issues"].([]interface{})
-	return issues, data
+	return issues, nil
 }
 
-func fetchIssueListPage(ctx *common.RuntimeContext, owner, repo, state string, page, limit int) ([]interface{}, map[string]interface{}) {
+// fetchPRDetail retrieves full PR detail to extract merged_at timestamp.
+// The list API doesn't return merged_at; only the detail endpoint does.
+func fetchPRDetail(ctx *common.RuntimeContext, prNumber int) (string, error) {
+	path := fmt.Sprintf("%s/pulls/%d", ctx.RepoPath(), prNumber)
+	env, err := ctx.CallAPI("GET", path, nil)
+	if err != nil {
+		return "", fmt.Errorf("PR detail fetch #%d: %w", prNumber, err)
+	}
+	if !env.OK {
+		return "", fmt.Errorf("PR detail API error #%d", prNumber)
+	}
+	data, ok := env.Data.(map[string]interface{})
+	if !ok {
+		return "", nil
+	}
+
+	// Try multiple possible locations for merged_at in the detail response
+	for _, candidate := range []string{
+		"merged_at",
+		"mergedAt",
+	} {
+		// data.pull_request.merged_at
+		if pr, _ := data["pull_request"].(map[string]interface{}); pr != nil {
+			if v, _ := pr[candidate].(string); v != "" {
+				return v, nil
+			}
+		}
+		// data.issue.merged_at
+		if issue, _ := data["issue"].(map[string]interface{}); issue != nil {
+			if v, _ := issue[candidate].(string); v != "" {
+				return v, nil
+			}
+		}
+		// data.merged_at (flat)
+		if v, _ := data[candidate].(string); v != "" {
+			return v, nil
+		}
+	}
+	return "", nil
+}
+
+func fetchIssueListPage(ctx *common.RuntimeContext, owner, repo, state string, page, limit int) ([]interface{}, error) {
 	q := url.Values{}
 	q.Set("page", fmt.Sprintf("%d", page))
 	q.Set("limit", fmt.Sprintf("%d", limit))
@@ -44,17 +91,18 @@ func fetchIssueListPage(ctx *common.RuntimeContext, owner, repo, state string, p
 	}
 	env, err := ctx.CallAPIWithQuery("GET", v1RepoPath(owner, repo)+"/issues", q)
 	if err != nil {
-		fmt.Printf("  CLI error: issue +list state=%s page=%d: %v\n", state, page, err)
+		fmt.Fprintf(os.Stderr, "  CLI error: issue +list state=%s page=%d: %v\n", state, page, err)
-		return nil, nil
+		return nil, err
 	}
 	if !env.OK {
-		fmt.Printf("  API error: issue +list state=%s page=%d\n", state, page)
+		err := fmt.Errorf("API error: issue +list state=%s page=%d", state, page)
-		return nil, nil
+		fmt.Fprintf(os.Stderr, "  %v\n", err)
+		return nil, err
 	}
 	data, ok := env.Data.(map[string]interface{})
 	if !ok {
-		return nil, nil
+		return nil, fmt.Errorf("unexpected response type for issue +list")
 	}
 	issues, _ := data["issues"].([]interface{})
-	return issues, data
+	return issues, nil
 }

shortcuts/health/db.go

@@ -4,6 +4,7 @@ import (
 	"database/sql"
 	_ "embed"
 	"fmt"
+	"os"
 
 	_ "modernc.org/sqlite"
 )
@@ -103,7 +104,9 @@ func savePullTags(db *sql.DB, pullID int, tagIDs []int) {
 		return
 	}
 	for _, tagID := range tagIDs {
-		db.Exec("INSERT OR IGNORE INTO pull_tags (pull_id, tag_id) VALUES (?, ?)", pullID, tagID)
+		if _, err := db.Exec("INSERT OR IGNORE INTO pull_tags (pull_id, tag_id) VALUES (?, ?)", pullID, tagID); err != nil {
+			fmt.Fprintf(os.Stderr, "  DB error: save pull_tags (pull=%d tag=%d): %v\n", pullID, tagID, err)
+		}
 	}
 }
 
@@ -112,7 +115,9 @@ func saveIssueTags(db *sql.DB, issueID int, tagIDs []int) {
 		return
 	}
 	for _, tagID := range tagIDs {
-		db.Exec("INSERT OR IGNORE INTO issue_tags (issue_id, tag_id) VALUES (?, ?)", issueID, tagID)
+		if _, err := db.Exec("INSERT OR IGNORE INTO issue_tags (issue_id, tag_id) VALUES (?, ?)", issueID, tagID); err != nil {
+			fmt.Fprintf(os.Stderr, "  DB error: save issue_tags (issue=%d tag=%d): %v\n", issueID, tagID, err)
+		}
 	}
 }
 
@@ -134,7 +139,22 @@ func extractTagNames(data map[string]interface{}, key string) []string {
 	return names
 }
 
-func savePull(db *sql.DB, repoID int, pr map[string]interface{}) {
+// mergeTimeFromList tries to extract merged_at directly from the list API response
+// (preferred, avoids an extra API call per PR).
+func mergeTimeFromList(pr map[string]interface{}) string {
+	for _, key := range []string{"merged_at", "mergedAt", "pr_merge_time", "merge_time"} {
+		if v, _ := pr[key].(string); v != "" {
+			return v
+		}
+		// Some APIs return numeric timestamps
+		if v, ok := pr[key].(float64); ok && v > 0 {
+			return fmt.Sprintf("%.0f", v)
+		}
+	}
+	return ""
+}
+
+func savePull(db *sql.DB, repoID int, pr map[string]interface{}, mergedAt string) {
 	// id: pull_request_id preferred, fallback to id
 	var prID float64
 	if v, ok := pr["pull_request_id"].(float64); ok && v > 0 {
@@ -171,9 +191,19 @@ func savePull(db *sql.DB, repoID int, pr map[string]interface{}) {
 		processorID = &pid
 	}
 
-	db.Exec(`INSERT OR REPLACE INTO pulls (id, repo_id, number, creater_id, status, processor_id, create_time, close_time)
+	// Priority: explicit mergedAt arg > list response field > nil
+	var mergedAtVal interface{} = nil
+	if mergedAt != "" {
+		mergedAtVal = mergedAt
+	} else if ma := mergeTimeFromList(pr); ma != "" {
+		mergedAtVal = ma
+	}
+
+	if _, err := db.Exec(`INSERT OR REPLACE INTO pulls (id, repo_id, number, creater_id, status, processor_id, create_time, merged_at)
 		VALUES (?, ?, ?, ?, ?, ?, ?, ?)`,
-		int(prID), repoID, int(prNumber), createrID, status, processorID, createTime, nil)
+		int(prID), repoID, int(prNumber), createrID, status, processorID, createTime, mergedAtVal); err != nil {
+		fmt.Fprintf(os.Stderr, "  DB error: save pull %d: %v\n", int(prID), err)
+	}
 
 	// Save tags
 	tagNames := extractTagNames(pr, "issue_tags")
@@ -205,7 +235,7 @@ func saveIssue(db *sql.DB, repoID int, issue map[string]interface{}, issueNumber
 	statusName := extractStatusName(issue)
 	var status string
 	var closeTime interface{}
-	if statusName == "关闭" {
+	if statusName == "关闭" || statusName == "Closed" {
 		status = "close"
 		if v, _ := issue["closed_on"].(string); v != "" {
 			closeTime = v
@@ -218,9 +248,11 @@ func saveIssue(db *sql.DB, repoID int, issue map[string]interface{}, issueNumber
 		status = "open"
 	}
 
-	db.Exec(`INSERT OR REPLACE INTO issues (id, repo_id, number, creater_id, processor_id, create_time, close_time, status)
+	if _, err := db.Exec(`INSERT OR REPLACE INTO issues (id, repo_id, number, creater_id, processor_id, create_time, close_time, status)
 		VALUES (?, ?, ?, ?, ?, ?, ?, ?)`,
-		int(issueID), repoID, issueNumber, createrID, processorID, createTime, closeTime, status)
+		int(issueID), repoID, issueNumber, createrID, processorID, createTime, closeTime, status); err != nil {
+		fmt.Fprintf(os.Stderr, "  DB error: save issue %d: %v\n", int(issueID), err)
+	}
 
 	// Save tags
 	tagNames := extractTagNames(issue, "tags")

shortcuts/health/health.go

@@ -113,7 +113,10 @@ func fetchPRs(ctx *common.RuntimeContext, db *sql.DB, repoID int, state string,
 			return nil
 		}
 		fmt.Fprintf(os.Stderr, "  PR list: state=%s, page=%d...\n", state, page)
-		prs, _ := fetchPRListPage(ctx, state, page, 20)
+		prs, err := fetchPRListPage(ctx, state, page, 20)
+		if err != nil {
+			return err
+		}
 		if len(prs) == 0 {
 			break
 		}
@@ -140,7 +143,28 @@ func fetchPRs(ctx *common.RuntimeContext, db *sql.DB, repoID int, state string,
 			if dup {
 				continue
 			}
-			savePull(db, repoID, pr)
+
+			// For merged PRs: try list response first, fall back to detail API
+			mergedAt := ""
+			if state == "merged" {
+				mergedAt = mergeTimeFromList(pr)
+				if mergedAt == "" {
+					if prNumFloat, ok := pr["pull_request_number"].(float64); ok && prNumFloat > 0 {
+						prNum := int(prNumFloat)
+						if err := limiter.Wait(egCtx); err != nil {
+							return nil
+						}
+						fmt.Fprintf(os.Stderr, "  PR detail #%d...\n", prNum)
+						if ma, err := fetchPRDetail(ctx, prNum); err != nil {
+							fmt.Fprintf(os.Stderr, "  PR detail #%d: %v\n", prNum, err)
+						} else {
+							mergedAt = ma
+						}
+					}
+				}
+			}
+
+			savePull(db, repoID, pr, mergedAt)
 		}
 		if len(prs) < 20 {
 			break
@@ -160,7 +184,10 @@ func fetchIssues(ctx *common.RuntimeContext, db *sql.DB, repoID int, state strin
 			return nil
 		}
 		fmt.Fprintf(os.Stderr, "  Issue list: state=%s, page=%d...\n", state, page)
-		issues, _ := fetchIssueListPage(ctx, ctx.Owner, ctx.Repo, state, page, 20)
+		issues, err := fetchIssueListPage(ctx, ctx.Owner, ctx.Repo, state, page, 20)
+		if err != nil {
+			return err
+		}
 		if len(issues) == 0 {
 			break
 		}

shortcuts/health/schema.sql

@@ -39,12 +39,13 @@ CREATE TABLE IF NOT EXISTS pulls (
     status TEXT CHECK(status IN ('merged', 'closed', 'open')),
     processor_id INTEGER,
     create_time TIMESTAMP,
-    close_time TIMESTAMP,
+    merged_at TIMESTAMP,
     FOREIGN KEY (repo_id) REFERENCES repos(id),
     FOREIGN KEY (creater_id) REFERENCES users(id),
     FOREIGN KEY (processor_id) REFERENCES users(id)
 );
 
+CREATE INDEX IF NOT EXISTS idx_pulls_merged_at ON pulls(merged_at);
 CREATE INDEX IF NOT EXISTS idx_pulls_repo_id ON pulls(repo_id);
 CREATE INDEX IF NOT EXISTS idx_pulls_status ON pulls(status);
 CREATE INDEX IF NOT EXISTS idx_pulls_create_time ON pulls(create_time);

shortcuts/register_test.go

@@ -14,6 +14,7 @@ func TestRegisterAll(t *testing.T) {
 		"repo", "issue", "label", "pr", "release", "branch",
 		"org", "user", "search", "ci", "workflow",
 		"compare", "member", "milestone", "pipeline", "webhook",
+		"health",
 	}
 
 	groupSet := map[string]bool{}

skills/README.md

@@ -114,9 +114,9 @@ skills/
 │   └── SKILL.md                       # PM 操作指南
 ├── gitlink-health/                    # 项目健康度分析
 │   ├── SKILL.md                       # 健康度分析指南
-│   ├── collector/
+│   ├── data/
-│   │   ├── fetcher.py                 # 数据采集脚本
+│   │   ├── .gitignore                 # 忽略 *.db 文件
-│   │   └── schema.sql                 # 建表 SQL
+│   │   └── .gitkeep                   # 占位文件
 │   ├── references/
 │   │   └── queries.md                 # SQL 查询参考
 │   └── asset/

skills/gitlink-health/references/queries.md

@@ -58,14 +58,14 @@ CREATE TABLE pulls (
     status TEXT CHECK(status IN ('merged', 'closed', 'open')),
     processor_id INTEGER,       -- 指派人 → users.id（nullable，来自 list 的 assign_user_login）
     create_time TIMESTAMP,      -- 创建时间，ISO 格式
-    close_time TIMESTAMP,       -- 始终为 NULL（API 不提供）
+    merged_at TIMESTAMP,        -- 合并时间（从 PR 详情 API 获取）
     FOREIGN KEY (repo_id) REFERENCES repos(id),
     FOREIGN KEY (creater_id) REFERENCES users(id),
     FOREIGN KEY (processor_id) REFERENCES users(id)
 );
 ```
 
-API 字段映射：`status` ← `pull_request_status`（0=open, 1=merged, 2=closed），`creater_id` ← `author_login`。
+API 字段映射：`status` ← `pull_request_status`（0=open, 1=merged, 2=closed），`creater_id` ← `author_login`；`merged_at` 需要调用 PR 详情 API（`GET /:owner/:repo/pulls/:number`）获取。
 
 ### tags