Copilot commented on code in PR #8808: URL: https://github.com/apache/incubator-devlake/pull/8808#discussion_r2983739322
########## backend/plugins/claude_code/service/connection_test_helper.go: ########## @@ -0,0 +1,112 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + stdctx "context" + "encoding/json" + "fmt" + "io" + "net/http" + "strings" + "time" + + corectx "github.com/apache/incubator-devlake/core/context" + "github.com/apache/incubator-devlake/core/errors" + helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api" + "github.com/apache/incubator-devlake/plugins/claude_code/models" +) + +// TestConnectionResult represents the payload returned by the connection test endpoints. +type TestConnectionResult struct { + Success bool `json:"success"` + Message string `json:"message"` + OrganizationId string `json:"organizationId,omitempty"` +} + +// TestConnection exercises the Claude Code Analytics API to validate credentials. +// It makes a minimal request with limit=1 to confirm the connection is valid. +func TestConnection(ctx stdctx.Context, br corectx.BasicRes, connection *models.ClaudeCodeConnection) (*TestConnectionResult, errors.Error) { + if connection == nil { + return nil, errors.BadInput.New("connection is required") + } + + connection.Normalize() + + hasToken := strings.TrimSpace(connection.Token) != "" + hasCustomHeaders := len(connection.CustomHeaders) > 0 + if !hasToken && !hasCustomHeaders { + return nil, errors.BadInput.New("either token or at least one custom header is required") + } + if strings.TrimSpace(connection.Organization) == "" { + return nil, errors.BadInput.New("organizationId is required") + } + + apiClient, err := helper.NewApiClientFromConnection(ctx, br, connection) + if err != nil { + return nil, err + } + + // Use today's date for the test request. + today := time.Now().UTC().Format("2006-01-02") + endpoint := fmt.Sprintf("v1/organizations/usage_report/claude_code?starting_at=%s&limit=1", today) + + res, err := apiClient.Get(endpoint, nil, nil) + if err != nil { Review Comment: The test request path (`v1/organizations/usage_report/claude_code`) does not match the analytics endpoints used by the collectors in this PR (`v1/organizations/analytics/...`). If `usage_report/claude_code` is deprecated/removed, connection tests will fail even though collection would work (or vice versa). Align the test endpoint with the same API surface the collectors use (e.g., a minimal `analytics/users` request with `limit=1`). ```suggestion // Use a minimal analytics users request with limit=1 for the test. endpoint := "v1/organizations/analytics/users?limit=1" res, err := apiClient.Get(endpoint, nil, nil) if err != nil { if err != nil { ``` ########## config-ui/src/plugins/register/claude-code/connection-fields/token.tsx: ########## @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +import { useEffect, useMemo, type ChangeEvent } from 'react'; +import { Input } from 'antd'; + +import { Block } from '@/components'; + +import * as S from './styled'; + +interface Props { + type: 'create' | 'update'; + initialValues: any; + values: any; + setValues: (value: any) => void; + setErrors: (value: any) => void; +} + +export const Token = ({ type, initialValues, values, setValues, setErrors }: Props) => { + useEffect(() => { + setValues({ token: type === 'create' ? (initialValues.token ?? '') : undefined }); + }, [type, initialValues.token]); + + const hasCustomHeaders = (values.customHeaders ?? []).length > 0; + + const error = useMemo(() => { + if (type === 'update') return ''; + if (hasCustomHeaders) return ''; + return values.token?.trim() ? '' : 'Anthropic API Key is required (unless custom headers are configured)'; + }, [type, values.token, hasCustomHeaders]); Review Comment: `hasCustomHeaders` is currently based only on `customHeaders.length > 0`, so users can bypass the required API key by adding an empty custom header row. This matches the backend behavior (it only checks slice length) and can lead to saving an unauthenticated connection. Consider computing `hasCustomHeaders` based on at least one header with a non-empty key/value (and setting an error if headers are present but invalid). ########## backend/plugins/claude_code/api/connection.go: ########## @@ -0,0 +1,110 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package api + +import ( + "strings" + + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/plugin" + helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api" + "github.com/apache/incubator-devlake/plugins/claude_code/models" +) + +// PostConnections creates a new Claude Code connection. +func PostConnections(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput, errors.Error) { + connection := &models.ClaudeCodeConnection{} + if err := helper.Decode(input.Body, connection, vld); err != nil { + return nil, err + } + + connection.Normalize() + if err := validateConnection(connection); err != nil { + return nil, err + } + + if err := connectionHelper.Create(connection, input); err != nil { + return nil, err + } + return &plugin.ApiResourceOutput{Body: connection.Sanitize()}, nil +} + +func PatchConnection(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput, errors.Error) { + connection := &models.ClaudeCodeConnection{} + if err := connectionHelper.First(connection, input.Params); err != nil { + return nil, err + } + if err := (&models.ClaudeCodeConnection{}).MergeFromRequest(connection, input.Body); err != nil { + return nil, errors.Convert(err) + } + connection.Normalize() + if err := validateConnection(connection); err != nil { + return nil, err + } + if err := connectionHelper.SaveWithCreateOrUpdate(connection); err != nil { + return nil, err + } + return &plugin.ApiResourceOutput{Body: connection.Sanitize()}, nil +} + +func DeleteConnection(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput, errors.Error) { + conn := &models.ClaudeCodeConnection{} + output, err := connectionHelper.Delete(conn, input) + if err != nil { + return output, err + } + output.Body = conn.Sanitize() + return output, nil +} + +func ListConnections(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput, errors.Error) { + var connections []models.ClaudeCodeConnection + if err := connectionHelper.List(&connections); err != nil { + return nil, err + } + for i := range connections { + connections[i] = connections[i].Sanitize() + } + return &plugin.ApiResourceOutput{Body: connections}, nil +} + +func GetConnection(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput, errors.Error) { + connection := &models.ClaudeCodeConnection{} + if err := connectionHelper.First(connection, input.Params); err != nil { + return nil, err + } + return &plugin.ApiResourceOutput{Body: connection.Sanitize()}, nil +} + +func validateConnection(connection *models.ClaudeCodeConnection) errors.Error { + if connection == nil { + return errors.BadInput.New("connection is required") + } + hasToken := strings.TrimSpace(connection.Token) != "" + hasCustomHeaders := len(connection.CustomHeaders) > 0 + if !hasToken && !hasCustomHeaders { + return errors.BadInput.New("either token or at least one custom header is required") + } + if strings.TrimSpace(connection.Organization) == "" { + return errors.BadInput.New("organization is required") + } Review Comment: `validateConnection` treats any non-empty `CustomHeaders` slice as valid authentication. This allows saving/testing a connection with only blank header entries (e.g., `{key:'', value:''}`), which will not set any headers in `SetupAuthentication` and will lead to auth failures at runtime. Consider validating that at least one custom header has a non-empty key (and likely a non-empty value) before accepting it as an auth alternative to `Token`. ########## backend/plugins/claude_code/service/connection_test_helper.go: ########## @@ -0,0 +1,112 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + stdctx "context" + "encoding/json" + "fmt" + "io" + "net/http" + "strings" + "time" + + corectx "github.com/apache/incubator-devlake/core/context" + "github.com/apache/incubator-devlake/core/errors" + helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api" + "github.com/apache/incubator-devlake/plugins/claude_code/models" +) + +// TestConnectionResult represents the payload returned by the connection test endpoints. +type TestConnectionResult struct { + Success bool `json:"success"` + Message string `json:"message"` + OrganizationId string `json:"organizationId,omitempty"` +} + +// TestConnection exercises the Claude Code Analytics API to validate credentials. +// It makes a minimal request with limit=1 to confirm the connection is valid. +func TestConnection(ctx stdctx.Context, br corectx.BasicRes, connection *models.ClaudeCodeConnection) (*TestConnectionResult, errors.Error) { + if connection == nil { + return nil, errors.BadInput.New("connection is required") + } + + connection.Normalize() + + hasToken := strings.TrimSpace(connection.Token) != "" + hasCustomHeaders := len(connection.CustomHeaders) > 0 + if !hasToken && !hasCustomHeaders { + return nil, errors.BadInput.New("either token or at least one custom header is required") + } + if strings.TrimSpace(connection.Organization) == "" { + return nil, errors.BadInput.New("organizationId is required") + } + + apiClient, err := helper.NewApiClientFromConnection(ctx, br, connection) + if err != nil { + return nil, err + } + + // Use today's date for the test request. + today := time.Now().UTC().Format("2006-01-02") + endpoint := fmt.Sprintf("v1/organizations/usage_report/claude_code?starting_at=%s&limit=1", today) Review Comment: The connection test uses today's date (`time.Now().UTC()`) even though collectors explicitly assume the Analytics API requires data to be at least a few days old (see `claudeCodeAvailabilityLagDays` usage in tasks). This can make the test endpoint fail for otherwise-correct credentials. Use a date that is guaranteed to be outside the availability lag window (e.g., now minus the lag days) for the test request. ```suggestion // Use a date safely outside the analytics availability lag window for the test request. testDate := time.Now().UTC().AddDate(0, 0, -7).Format("2006-01-02") endpoint := fmt.Sprintf("v1/organizations/usage_report/claude_code?starting_at=%s&limit=1", testDate) ``` ########## backend/plugins/claude_code/models/migrationscripts/20260319_replace_analytics_tables.go: ########## @@ -0,0 +1,207 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package migrationscripts + +import ( + "encoding/json" + "time" + + "github.com/apache/incubator-devlake/core/context" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/models/migrationscripts/archived" + "github.com/apache/incubator-devlake/helpers/migrationhelper" +) + +// replaceClaudeCodeAnalyticsTables drops the old deprecated endpoint tables and +// creates the five new analytics endpoint tables. +type replaceClaudeCodeAnalyticsTables struct{} + +func (*replaceClaudeCodeAnalyticsTables) Up(basicRes context.BasicRes) errors.Error { + return migrationhelper.AutoMigrateTables( + basicRes, + &ccUserActivity20260319{}, Review Comment: The comment says this migration "drops the old deprecated endpoint tables", but `Up` only calls `AutoMigrateTables` (which typically creates/updates tables but does not drop old ones). If dropping is required to avoid stale tables/data, add explicit drop/cleanup logic; otherwise update the comment to reflect what the migration actually does. ########## config-ui/src/plugins/register/claude-code/connection-fields/custom-headers.tsx: ########## @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +import { useEffect, type ChangeEvent } from 'react'; +import { Button, Input } from 'antd'; + +import { Block } from '@/components'; + +interface Props { + type: 'create' | 'update'; + initialValues: any; + values: any; + setValues: (value: any) => void; + setErrors: (value: any) => void; +} + +export const CustomHeaders = ({ type, initialValues, values, setValues }: Props) => { + const headers: Array<{ key: string; value: string }> = values.customHeaders ?? []; + + useEffect(() => { + setValues({ customHeaders: initialValues.customHeaders ?? [] }); + }, [type, initialValues.customHeaders]); + + const addHeader = () => { + setValues({ customHeaders: [...headers, { key: '', value: '' }] }); + }; Review Comment: The UI allows adding blank custom header entries (`{ key: '', value: '' }`). Because token-required logic and backend validation currently treat any non-empty header array as sufficient, this can accidentally create connections that have neither a valid token nor any effective auth headers. Consider preventing add/save when header key/value are empty, and/or filtering out empty entries before persisting. ########## backend/plugins/claude_code/service/connection_test_helper.go: ########## @@ -0,0 +1,112 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package service + +import ( + stdctx "context" + "encoding/json" + "fmt" + "io" + "net/http" + "strings" + "time" + + corectx "github.com/apache/incubator-devlake/core/context" + "github.com/apache/incubator-devlake/core/errors" + helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api" + "github.com/apache/incubator-devlake/plugins/claude_code/models" +) + +// TestConnectionResult represents the payload returned by the connection test endpoints. +type TestConnectionResult struct { + Success bool `json:"success"` + Message string `json:"message"` + OrganizationId string `json:"organizationId,omitempty"` +} + +// TestConnection exercises the Claude Code Analytics API to validate credentials. +// It makes a minimal request with limit=1 to confirm the connection is valid. +func TestConnection(ctx stdctx.Context, br corectx.BasicRes, connection *models.ClaudeCodeConnection) (*TestConnectionResult, errors.Error) { + if connection == nil { + return nil, errors.BadInput.New("connection is required") + } + + connection.Normalize() + + hasToken := strings.TrimSpace(connection.Token) != "" + hasCustomHeaders := len(connection.CustomHeaders) > 0 + if !hasToken && !hasCustomHeaders { + return nil, errors.BadInput.New("either token or at least one custom header is required") + } + if strings.TrimSpace(connection.Organization) == "" { + return nil, errors.BadInput.New("organizationId is required") Review Comment: Error message says `organizationId is required`, but the connection field in this plugin/model is `organization` (and the API-side `validateConnection` returns "organization is required"). This inconsistency can confuse users and makes it harder to map UI fields to backend validation. Use consistent naming across validation and responses (either standardize on `organization` everywhere or explicitly rename the field/JSON tag to `organizationId`). ```suggestion return nil, errors.BadInput.New("organization is required") ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
