feat: Add read-only feature map command UX#636
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements an optional issue-backed "Feature Map" system, enabling users to maintain a capability view within GitHub issues. It introduces the feature command suite (list, check, for), updates the command registry, and provides extensive documentation. The review identified a critical compilation error caused by the missing markdownSection function. Additionally, feedback suggests improving the robustness of issue fetching to avoid "not found" errors due to the 1000-issue limit, refining JSON error responses in the CLI, and refactoring duplicated positional argument parsing logic.
| } | ||
| } | ||
| for _, section := range []string{"User Need", "Capability", "Surface"} { | ||
| if strings.TrimSpace(markdownSection(issue.Body, section)) == "" { |
There was a problem hiding this comment.
The function markdownSection is used here but is not defined in this file or any other file in the package provided in this pull request. This will cause a compilation error.
| if options.Issue <= 0 { | ||
| return FeatureMapForReport{}, fmt.Errorf("issue must be > 0") | ||
| } | ||
| issues, _, err := fetchFeatureMapIssues(FeatureMapOptions{Repo: options.Repo, Limit: options.Limit}, runner) |
There was a problem hiding this comment.
The BuildFeatureMapForReport function relies on fetchFeatureMapIssues which fetches the 1000 most recently updated issues by default. If the requested options.Issue is an older issue that hasn't been updated recently, it may not be found in the list, leading to a "not found" error even if the issue exists. It would be more robust to fetch the specific issue first using gh issue view.
| if _, ok := featureNumbers[linked]; !ok { | ||
| diagnostics = append(diagnostics, FeatureMapDiagnostic{Severity: "error", Issue: issue.Number, Code: "linked_feature_not_found", Message: fmt.Sprintf("linked feature #%d was not found", linked)}) | ||
| } |
There was a problem hiding this comment.
The check for linked_feature_not_found may produce false positives if the linked feature issue is not among the 1000 most recently updated issues fetched by fetchFeatureMapIssues. Consider fetching the specific linked issue if it's missing from the initial list to confirm its existence.
| if err != nil { | ||
| if *jsonOutput { | ||
| out, _ := json.MarshalIndent(report, "", " ") | ||
| fmt.Fprintf(stdout, "%s\n", out) | ||
| } | ||
| fmt.Fprintf(stderr, "%v\n", err) | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
When an error occurs and jsonOutput is enabled, the code marshals and prints the zero-value report to stdout. This results in a JSON object with empty fields and no schema version, which may be difficult for consumers to handle. Consider emitting a structured error JSON or omitting the JSON output on error.
| func extractFeatureForPositional(args []string, stderr io.Writer) ([]string, int, bool) { | ||
| cleaned := make([]string, 0, len(args)) | ||
| positional := 0 | ||
| valueFlags := map[string]struct{}{"--repo": {}, "--issue": {}, "--limit": {}} | ||
| for i := 0; i < len(args); i++ { | ||
| arg := args[i] | ||
| cleaned = append(cleaned, arg) | ||
| if _, ok := valueFlags[arg]; ok { | ||
| if i+1 < len(args) { | ||
| i++ | ||
| cleaned = append(cleaned, args[i]) | ||
| } | ||
| continue | ||
| } | ||
| if strings.HasPrefix(arg, "-") { | ||
| continue | ||
| } | ||
| n, err := strconv.Atoi(arg) | ||
| if err != nil || n <= 0 { | ||
| fmt.Fprintf(stderr, "unexpected positional argument %q; use a numeric issue or --issue N\n\n", arg) | ||
| return nil, 0, false | ||
| } | ||
| if positional > 0 && positional != n { | ||
| fmt.Fprint(stderr, "only one positional issue can be provided\n\n") | ||
| return nil, 0, false | ||
| } | ||
| positional = n | ||
| cleaned = cleaned[:len(cleaned)-1] | ||
| } | ||
| return cleaned, positional, true | ||
| } |
There was a problem hiding this comment.
The logic in extractFeatureForPositional for extracting numeric positional arguments while skipping flags that take values is highly similar to extractNumericPositional and other helpers in this file. This duplication increases maintenance overhead. Consider refactoring these into a single generic helper.
1 participant
Closes #635