wip

moonray · moonray · commit cb86a002a13c · 2025-05-05T12:44:17.000-04:00
diff --git a/CONTINUITY.md b/CONTINUITY.md
@@ -0,0 +1,38 @@
+# CONTINUITY.md
+
+## GitHub Projects V2 Tools Continuity
+
+### Objective
+Refine and robustly test the owner resolution logic for GitHub Projects V2 tools, ensuring correct handling of organizations, users, and ambiguous cases, and maintain strict TDD and modularity.
+
+### Key Accomplishments
+- Refactored owner resolution logic into a reusable `resolveOwnerID` function.
+- All tools (ListOrganizationProjectsTool, ListUserProjectsTool, GetProjectTool, CreateProjectTool) now use this logic.
+- Updated and extended tests for all relevant scenarios, including ambiguous and error cases.
+- Fixed test panics due to mock exhaustion by queuing multiple mock responses where needed.
+- Ensured all GraphQL queries and mutations use the correct owner ID (user or org) as required by GitHub's API.
+- All tests for repository resource content and owner resolution now pass except for a single edge case (`owner_is_user`), which is under investigation.
+
+### Outstanding Issues
+- `TestOwnerResolutionInCreateProject/owner_is_user` fails, likely due to a mismatch between the expected and actual mutation input or mock handler logic.
+- Need to confirm mutation input for `createProjectV2` uses the resolved user ID and matches the expected GraphQL structure.
+- All other tests, including ambiguous and negative owner cases, pass.
+
+### Next Steps
+1. Add debug output to the test or handler to reveal the actual error message and request body for the failing case.
+2. Inspect the mutation input struct and marshaling to ensure the correct field (`ownerId`) is sent.
+3. Once all tests pass, refactor Projects V2 logic and tool factories into a single `projects.go` file for convention compliance and PR acceptance.
+
+### Design and Implementation Notes
+- Owner resolution is always attempted for both org and user; org is preferred if both exist.
+- Mock handlers in tests must return the correct GraphQL response structure to avoid unmarshalling errors.
+- All code changes follow TDD, modular, and DRY principles as per user rules.
+- Security: GitHub tokens are handled via env var and not hardcoded.
+
+### User Preferences
+- Strict TDD and validation using specs/references.
+- Modular, minimal, and DRY Go code.
+- All logic and factories to be colocated for each feature.
+
+---
+_Last updated: 2025-04-21 03:48:58-04:00_
diff --git a/TODO.md b/TODO.md
@@ -0,0 +1,24 @@
+# TODO.md
+
+## GitHub Projects V2 Tools: Next Steps
+
+### Immediate
+- [ ] Debug and fix `TestOwnerResolutionInCreateProject/owner_is_user` failure:
+    - [ ] Add debug output in the test handler to print the actual error and request body for this case.
+    - [ ] Confirm that `createProjectV2` mutation receives the resolved user ID as `ownerId`.
+    - [ ] Inspect struct tags and marshaling for mutation input.
+    - [ ] Adjust either test or code until all cases pass.
+- [ ] Run full test suite and validate all tests pass with no regressions.
+
+### After All Tests Pass
+- [ ] Refactor Projects V2 business logic and MCP tool factories into a single `projects.go` file, matching codebase conventions.
+- [ ] Validate that all tool registrations and integrations remain functional after refactor.
+- [ ] Update/validate documentation and CONTINUITY.md as needed.
+
+### Ongoing
+- [ ] Maintain strict TDD and minimal/DRY Go code.
+- [ ] Ensure all GraphQL interactions match GitHub's documented API structure.
+- [ ] Keep security best practices for token management.
+
+---
+_Last updated: 2025-04-21 03:49:21-04:00_
diff --git a/pkg/github/is_graphql_not_found.go b/pkg/github/is_graphql_not_found.go
@@ -0,0 +1,15 @@
+package github
+
+import "strings"
+
+// isGraphQLNotFound returns true if the error is a GraphQL 'not found' error.
+func isGraphQLNotFound(err error) bool {
+	if err == nil {
+		return false
+	}
+	msg := err.Error()
+	return strings.Contains(msg, "could not resolve to a User") ||
+		strings.Contains(msg, "could not resolve to an Organization") ||
+		strings.Contains(msg, "non-200 OK status code: 400") ||
+		strings.Contains(msg, "non-200 OK status code: 404")
+}
diff --git a/pkg/github/is_graphql_not_found_test.go b/pkg/github/is_graphql_not_found_test.go
@@ -0,0 +1,30 @@
+package github
+
+import (
+	"errors"
+	"testing"
+)
+
+func TestIsGraphQLNotFound(t *testing.T) {
+	cases := []struct {
+		name    string
+		err     error
+		expects bool
+	}{
+		{"nil error", nil, false},
+		{"empty error", errors.New(""), false},
+		{"user not found", errors.New("could not resolve to a User"), true},
+		{"org not found", errors.New("could not resolve to an Organization"), true},
+		{"400 code", errors.New("non-200 OK status code: 400"), true},
+		{"404 code", errors.New("non-200 OK status code: 404"), true},
+		{"other error", errors.New("some other error"), false},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			result := isGraphQLNotFound(c.err)
+			if result != c.expects {
+				t.Errorf("expected %v, got %v for input %v", c.expects, result, c.err)
+			}
+		})
+	}
+}
diff --git a/pkg/github/owner_resolution.go b/pkg/github/owner_resolution.go
@@ -0,0 +1,47 @@
+package github
+
+import (
+	"context"
+	"errors"
+	"fmt"
+
+	ghv4 "github.com/shurcooL/githubv4"
+)
+
+// resolveOwnerID resolves an owner login (org or user) to a GraphQL ID, preferring org if both exist.
+// Returns the ID or an error ("owner not found" if neither found).
+type GraphQLClient interface {
+	Query(ctx context.Context, q interface{}, vars map[string]interface{}) error
+}
+
+func resolveOwnerID(ctx context.Context, client GraphQLClient, owner string) (ghv4.ID, error) {
+	var orgQ struct {
+		Organization *struct{ ID ghv4.ID } `graphql:"organization(login: $login)"`
+	}
+	orgVars := map[string]interface{}{"login": ghv4.String(owner)}
+	orgErr := client.Query(ctx, &orgQ, orgVars)
+	orgNotFound := orgErr != nil && isGraphQLNotFound(orgErr)
+	if orgErr != nil && !orgNotFound {
+		return "", fmt.Errorf("organization lookup failed: %w", orgErr)
+	}
+	if orgQ.Organization != nil {
+		return orgQ.Organization.ID, nil
+	}
+
+	var userQ struct {
+		User *struct{ ID ghv4.ID } `graphql:"user(login: $login)"`
+	}
+	userVars := map[string]interface{}{"login": ghv4.String(owner)}
+	userErr := client.Query(ctx, &userQ, userVars)
+	userNotFound := userErr != nil && isGraphQLNotFound(userErr)
+	if userErr != nil && !userNotFound {
+		return "", fmt.Errorf("user lookup failed: %w", userErr)
+	}
+	if userQ.User != nil {
+		return userQ.User.ID, nil
+	}
+	if orgNotFound && userNotFound {
+		return "", errors.New("owner not found")
+	}
+	return "", errors.New("owner not found") // Defensive fallback
+}
diff --git a/pkg/github/owner_resolution_test.go b/pkg/github/owner_resolution_test.go
@@ -0,0 +1,128 @@
+package github
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	ghv4 "github.com/shurcooL/githubv4"
+	"github.com/stretchr/testify/assert"
+)
+
+
+type fakeGraphQLClient struct {
+	orgID      ghv4.ID
+	userID     ghv4.ID
+	orgErr     error
+	userErr    error
+}
+
+func (f *fakeGraphQLClient) Query(ctx context.Context, q interface{}, vars map[string]interface{}) error {
+	if oq, ok := q.(*struct {
+		Organization *struct{ ID ghv4.ID } `graphql:"organization(login: $login)"`
+	}); ok {
+		if f.orgErr != nil {
+			return f.orgErr
+		}
+		if f.orgID != "" {
+			*oq = struct {
+				Organization *struct{ ID ghv4.ID } `graphql:"organization(login: $login)"`
+			}{Organization: &struct{ ID ghv4.ID }{ID: f.orgID}}
+		}
+		return nil
+	}
+	if uq, ok := q.(*struct {
+		User *struct{ ID ghv4.ID } `graphql:"user(login: $login)"`
+	}); ok {
+		if f.userErr != nil {
+			return f.userErr
+		}
+		if f.userID != "" {
+			*uq = struct {
+				User *struct{ ID ghv4.ID } `graphql:"user(login: $login)"`
+			}{User: &struct{ ID ghv4.ID }{ID: f.userID}}
+		}
+		return nil
+	}
+	return errors.New("unexpected query type")
+}
+
+func (f *fakeGraphQLClient) Mutate(ctx context.Context, m interface{}, input interface{}, v map[string]interface{}) error {
+	return errors.New("not implemented")
+}
+
+func TestResolveOwnerID(t *testing.T) {
+	ctx := context.Background()
+	owner := "testowner"
+
+	tests := []struct {
+		name      string
+		orgID     ghv4.ID
+		userID    ghv4.ID
+		orgErr    error
+		userErr   error
+		expectID  ghv4.ID
+		expectErr string
+	}{
+		{
+			name:     "org exists",
+			orgID:    "ORGID",
+			userID:   "",
+			expectID:  "ORGID",
+		},
+		{
+			name:     "user exists",
+			orgID:    "",
+			userID:   "USERID",
+			expectID:  "USERID",
+		},
+		{
+			name:     "both org and user exist (prefer org)",
+			orgID:    "ORGID",
+			userID:   "USERID",
+			expectID:  "ORGID",
+		},
+		{
+			name:     "neither org nor user exist",
+			orgID:    "",
+			userID:   "",
+			orgErr:   errors.New("non-200 OK status code: 404"),
+			userErr:  errors.New("non-200 OK status code: 404"),
+			expectErr: "owner not found",
+		},
+		{
+			name:     "org fatal error",
+			orgID:    "",
+			userID:   "USERID",
+			orgErr:   errors.New("fatal org error"),
+			expectErr: "organization lookup failed",
+		},
+		{
+			name:     "user fatal error",
+			orgID:    "",
+			userID:   "",
+			orgErr:   errors.New("non-200 OK status code: 404"),
+			userErr:  errors.New("fatal user error"),
+			expectErr: "user lookup failed",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			client := &fakeGraphQLClient{
+				orgID:   tc.orgID,
+				userID:  tc.userID,
+				orgErr:  tc.orgErr,
+				userErr: tc.userErr,
+			}
+			id, err := resolveOwnerID(ctx, client, owner)
+			if tc.expectErr != "" {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), tc.expectErr)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tc.expectID, id)
+			}
+		})
+	}
+}
diff --git a/pkg/github/projects.go b/pkg/github/projects.go
@@ -354,6 +354,8 @@ func GetProjectItems(ctx context.Context, in *GetProjectItemsInput, client *ghv4
 
 // CreateProject creates a new project using the provided githubv4.Client.
 // If client is nil, a default client is created using GITHUB_TOKEN from environment.
+// CreateProject creates a new project using the provided githubv4.Client.
+// Resolves the owner (organization or user) to a GraphQL ID and uses it in the mutation input.
 func CreateProject(ctx context.Context, in *CreateProjectInput, client *ghv4.Client) (*Project, error) {
 	if in.Owner == "" || in.Title == "" {
 		return nil, errors.New("owner and title are required")
@@ -367,22 +369,10 @@ func CreateProject(ctx context.Context, in *CreateProjectInput, client *ghv4.Cli
 		client = ghv4.NewClient(&http.Client{Transport: &authTransport{token: token}})
 	}
 
-	// Lookup owner ID (org or user)
-	var ownerQ struct {
-		Organization *struct{ ID ghv4.ID } `graphql:"organization(login: $login)"`
-		User         *struct{ ID ghv4.ID } `graphql:"user(login: $login)"`
-	}
-	ownerVars := map[string]interface{}{"login": ghv4.String(in.Owner)}
-	if err := client.Query(ctx, &ownerQ, ownerVars); err != nil {
-		return nil, fmt.Errorf("owner lookup failed: %w", err)
-	}
-	var ownerID ghv4.ID
-	if ownerQ.Organization != nil {
-		ownerID = ownerQ.Organization.ID
-	} else if ownerQ.User != nil {
-		ownerID = ownerQ.User.ID
-	} else {
-		return nil, errors.New("owner not found")
+	// Always resolve the owner to a GraphQL ID (works for both orgs and users)
+	ownerID, err := resolveOwnerID(ctx, client, in.Owner)
+	if err != nil {
+		return nil, err
 	}
 
 	type createProjectInput struct {
diff --git a/pkg/github/projects_test.go b/pkg/github/projects_test.go
diff --git a/pkg/github/projects_tools.go b/pkg/github/projects_tools.go
diff --git a/pkg/github/repository_resource_test.go b/pkg/github/repository_resource_test.go
