CLI Tool First Phase#550
Conversation
apipctl gateway current
WalkthroughRename CLI from apipctl to ap; add comprehensive gateway/MCP/API commands; implement policy-manifest processing, PolicyHub integration with caching and checksum verification; add Docker image build orchestration, many utility modules, tests, and GitHub Actions workflows for build/test and release automation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as ap CLI
participant Config as Config Manager
participant PolicyHub as PolicyHub API
participant Cache as Local Cache
participant Docker as Docker Engine
User->>CLI: ap gateway build --file manifest.yaml
CLI->>Config: Load config / active gateway
CLI->>CLI: Parse manifest → Separate local vs hub policies
alt Local policies
CLI->>Cache: Validate local ZIPs, compute SHA256, copy to workspace
end
alt Hub policies
CLI->>PolicyHub: POST cleaned manifest → resolve policies
PolicyHub-->>CLI: Resolved metadata (download URLs, checksums)
CLI->>Cache: Download/verify policy zips, update index/cache
end
CLI->>CLI: Generate lock file & prepare workspace
CLI->>Docker: BuildGatewayImages(workspace, targets)
Docker-->>CLI: Build results/logs
CLI-->>User: Print build summary
sequenceDiagram
actor User
participant CLI as ap CLI
participant Client as Gateway HTTP Client
participant Gateway as Gateway API
User->>CLI: ap gateway add --server https://example
CLI->>CLI: Prompt for OAuth2 token or check env Basic Auth
CLI->>Client: Create HTTP client (auth headers set)
CLI->>Config: Save gateway to ~/.ap/config.yaml
CLI-->>User: Confirm gateway added
User->>CLI: ap gateway health
CLI->>Client: GET /health
Client->>Gateway: HTTP GET /health
Gateway-->>Client: JSON health payload
Client-->>CLI: Parsed response
CLI-->>User: Display health status and message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
cli/src/Makefile-47-73 (1)
47-73: Build target ignores test failures.The
buildtarget runs tests but uses|| trueon line 52, which causes the build to proceed even when tests fail. The warning message on lines 66-68 is informative but doesn't stop the build. This could lead to shipping broken code.Consider either failing the build on test failures or renaming this target to make the behavior explicit (e.g.,
build-with-optional-tests).🔎 Proposed fix to fail build on test failures
## build: Build the binary for current platform (runs tests first) build: @echo "===============================================" @echo "Running Tests" @echo "===============================================" @mkdir -p tests/logs - @$(GOTEST) -v ./tests/... > tests/logs/test-results.log 2>&1 || true + @$(GOTEST) -v ./tests/... 2>&1 | tee tests/logs/test-results.log; test $${PIPESTATUS[0]} -eq 0 @echo "" @echo "===============================================" @echo "Test Results Summary" @echo "===============================================" @printf "%-40s | %s\n" "Test Name" "Status" @printf "%-40s-+---------\n" "----------------------------------------" @grep -E "^=== RUN|^--- PASS|^--- FAIL" tests/logs/test-results.log | \ awk 'BEGIN { test="" } \ /^=== RUN/ { test=$$3 } \ /^--- PASS/ { printf "%-40s | ✓ PASS\n", test } \ /^--- FAIL/ { printf "%-40s | ✗ FAIL\n", test }' || true @echo "===============================================" @echo "ℹ Full test logs: tests/logs/test-results.log" - @if grep -q "^--- FAIL" tests/logs/test-results.log 2>/dev/null; then \ - echo "⚠ Some tests failed - check logs for details"; \ - fi + @if grep -q "^--- FAIL" tests/logs/test-results.log 2>/dev/null; then \ + echo "✗ Tests failed - build aborted. Check logs for details"; \ + exit 1; \ + fi @echo "" @echo "Building $(BINARY_NAME)..."cli/src/internal/policy/offline.go-138-154 (1)
138-154: Add explicit cleanup for temporary zip file.When a local policy is a directory, a temporary zip file is created for checksum calculation but never explicitly deleted. Since
utils.GetTempDir()returns a persistent user directory (not an auto-cleaning system temp location), adddefer os.Remove(checksumPath)after successfully creating the zip to ensure cleanup on function exit.cli/src/cmd/root.go-40-42 (1)
40-42: Remove or implement the--insecureflag reference in the help documentation.The help text at line 42 documents
--insecure → -ias a supported flag shortcut, but this flag is not defined anywhere in the codebase. Either remove this reference from the documentation or implement the actual flag. Currently, users following the documented shorthand will encounter an error.cli/src/cmd/gateway/current.go-61-66 (1)
61-66: Check environment variables for Basic Auth status display.Basic Auth is supported via the
WSO2AP_GW_USERNAMEandWSO2AP_GW_PASSWORDenvironment variables, which are checked by the HTTP client (seecli/src/internal/gateway/client.go). However, the current status command only checksgateway.Tokenand doesn't detect if Basic Auth is configured via these environment variables, causing it to display "none" as the auth status for users with Basic Auth enabled.Update the status display to check for these environment variables and display "Basic Auth" accordingly.
cli/src/cmd/gateway/use.go-30-34 (1)
30-34: Flag name mismatch between example and implementation.The example shows
--name devbututils.FlagNameis defined as"display-name"(percli/src/utils/flags.goline 23). Users following the example will get an "unknown flag" error.Either update the example to use
--display-nameor use a different flag constant (e.g.,FlagGatewayNameor similar) that maps to--name.🔎 Proposed fix
const ( UseCmdLiteral = "use" UseCmdExample = `# Set dev as the active gateway -ap gateway use --name dev` +ap gateway use --display-name dev` )Or define a new flag constant if
--nameis preferred for gateway commands.cli/src/internal/policy/manifest.go-155-162 (1)
155-162: Remove redundant validation check.Lines 160-162 check
!hasFilePath && !hasVersion, but this condition is already validated by lines 155-157. The second check is unreachable.🔎 Proposed fix
if !hasVersion && !hasFilePath { return fmt.Errorf("policy %s: must specify either version or filePath", policy.Name) } - // Hub policies must have version - if !hasFilePath && !hasVersion { - return fmt.Errorf("policy %s: hub policies must specify version", policy.Name) - } - // Validate local policy zip file structurecli/src/tests/cmd/gateway_image_build_test.go-247-258 (1)
247-258: Hub policies test step numbers also mismatched.Same issue: expects
"[4/6]"but implementation uses"[5/9]".cli/src/tests/cmd/gateway_image_build_test.go-447-459 (1)
447-459: Offline mode test step numbers mismatched.Test expects
"[2/4]","[3/4]"butrunOfflineBuild()uses"[2/7]","[3/7]".cli/src/internal/policy/hub.go-260-263 (1)
260-263: Broken condition:fmt.Sprint(os.Stderr)does not capture stderr content.
fmt.Sprint(os.Stderr)returns the string representation of the file pointer (e.g.,"&{0xc000...}"), not the content written to stderr. This condition always evaluates totrue, making it dead code.🔎 Proposed fix
Simply remove the condition since it doesn't work:
// Download policy - if !strings.Contains(fmt.Sprint(os.Stderr), "cache entry exists but file missing") { - fmt.Printf("downloading...") - } + fmt.Printf("downloading...") if err := downloadPolicy(policy.DownloadURL, cachePath); err != nil {cli/src/internal/policy/hub.go-135-140 (1)
135-140: HTTP client missing timeout.The HTTP client created at line 135 has no timeout configured. Network issues could cause the command to hang indefinitely.
🔎 Proposed fix
- client := &http.Client{} + client := &http.Client{ + Timeout: 30 * time.Second, + } resp, err := client.Do(req)Also add
"time"to imports.cli/src/cmd/gateway/image/build.go-351-359 (1)
351-359: Same ignored error pattern in offline build.Apply the same fix here for consistency and safety.
🔎 Proposed fix
// Generate workspace lock file with filePaths - tempGatewayImageBuildDir, _ := utils.GetTempGatewayImageBuildDir() + tempGatewayImageBuildDir, err := utils.GetTempGatewayImageBuildDir() + if err != nil { + return fmt.Errorf("failed to get temp gateway image build directory: %w", err) + } workspaceLockPath := filepath.Join(tempGatewayImageBuildDir, utils.DefaultManifestLockFile)cli/src/cmd/gateway/image/build.go-232-239 (1)
232-239: Ignored error fromGetTempGatewayImageBuildDir.The error is discarded with
_, but at line 463 inrunDockerBuild()the same function's error is properly handled. If this fails, subsequent operations will use an empty path.🔎 Proposed fix
// Generate workspace lock file with filePaths - tempGatewayImageBuildDir, _ := utils.GetTempGatewayImageBuildDir() + tempGatewayImageBuildDir, err := utils.GetTempGatewayImageBuildDir() + if err != nil { + return fmt.Errorf("failed to get temp gateway image build directory: %w", err) + } workspaceLockPath := filepath.Join(tempGatewayImageBuildDir, utils.DefaultManifestLockFile)cli/src/cmd/gateway/build.go-198-240 (1)
198-240: Bug: Missing policies calculation includes local policies.
requestedPolicies(lines 210-216) includes all manifest policies, butresponse.Dataonly contains hub-resolved policies. Local policies will always appear as "missing" since they're never returned by PolicyHub.🔎 Proposed fix
// Create requested policies list for tracking - requestedPolicies := make([]policyhub.RequestedPolicy, len(manifest.Policies)) - for i, policy := range manifest.Policies { - requestedPolicies[i] = policyhub.RequestedPolicy{ - Name: policy.Name, - Version: policy.Version, + // Only track hub policies for missing check (local policies won't be in response) + var requestedPolicies []policyhub.RequestedPolicy + for _, policy := range manifest.Policies { + if policy.FilePath == "" { // Hub policy (no local path) + requestedPolicies = append(requestedPolicies, policyhub.RequestedPolicy{ + Name: policy.Name, + Version: policy.Version, + }) } }cli/src/internal/policy/hub.go-314-340 (1)
314-340: HTTP download missing timeout.Similar to
resolveWithPolicyHub,http.Getuses the default client with no timeout.🔎 Proposed fix
// downloadPolicy downloads a policy from a URL to a destination path func downloadPolicy(url, destPath string) error { - resp, err := http.Get(url) + client := &http.Client{ + Timeout: 60 * time.Second, // Longer timeout for downloads + } + resp, err := client.Get(url) if err != nil { return fmt.Errorf("failed to download policy: %w", err) }cli/src/tests/cmd/gateway_image_build_test.go-140-152 (1)
140-152: Step numbers in expected patterns don't match implementation.The test expects
"[1/6]","[2/6]","[3/6]"butcli/src/cmd/gateway/image/build.gouses"[1/8]"or"[1/9]"steps for online builds. These tests will fail.🔎 Proposed fix
// Check for expected output patterns expectedPatterns := []string{ "=== Gateway Image Build ===", - "[1/6] Checking Docker Availability", - "[2/6] Reading Policy Manifest", - "[3/6] Processing Local Policies", + "[1/8] Checking Docker Availability", + "[2/9] Reading Policy Manifest", + "[3/9] Categorizing Policies", "Local policies:", }cli/src/cmd/gateway/api/get.go-64-69 (1)
64-69: Fix flag name mismatch in the example command.The example at line 41 uses
--namebut the flag is registered as--display-nameviautils.FlagName. Users following the example will get a command error.Change the example from
--name "PetStore API"to--display-name "PetStore API"to match the actual flag constant.cli/src/utils/policy_utils.go-41-55 (1)
41-55: Eliminate code duplication between normalizeNameForComparison and ToKebabCase.Both functions implement identical kebab-case conversion logic. The unexported
normalizeNameForComparisonshould be removed and replaced with calls to the exportedToKebabCase.🔎 Proposed fix
-// normalizeNameForComparison converts a name to kebab-case for comparison -func normalizeNameForComparison(name string) string { - // Convert to kebab-case (lowercase with hyphens) - var result strings.Builder - - for i, r := range name { - // Add hyphen before uppercase letters (except the first character) - if i > 0 && r >= 'A' && r <= 'Z' { - result.WriteRune('-') - } - result.WriteRune(r) - } - - return strings.ToLower(result.String()) -}Then update any callers to use
ToKebabCaseinstead (if any exist).Also applies to: 259-271
cli/src/utils/constants.go-54-54 (1)
54-54: Major security concern: Checksum verification disabled by default.Setting
GatewayVerifyChecksumOnBuild = falsedisables integrity checks for downloaded policies, potentially allowing corrupted or tampered policies to be used without detection. Consider enabling this by default for security.🔎 Proposed fix
- GatewayVerifyChecksumOnBuild = false + GatewayVerifyChecksumOnBuild = truecli/src/internal/policyhub/client.go-142-166 (1)
142-166: Add Content-Type validation before downloading.The download function doesn't validate the Content-Type header before saving the file. This could lead to saving non-ZIP content if the server returns an error page or unexpected content.
🔎 Proposed enhancement
if resp.StatusCode != http.StatusOK { body, _ := io.ReadAll(resp.Body) return fmt.Errorf("download failed with status %d: %s", resp.StatusCode, string(body)) } + + // Validate Content-Type + contentType := resp.Header.Get("Content-Type") + if contentType != "" && contentType != "application/zip" && contentType != "application/octet-stream" { + return fmt.Errorf("unexpected content type: %s, expected application/zip", contentType) + } out, err := os.Create(destPath)cli/src/utils/policy_utils.go-540-553 (1)
540-553: Fragile version folder detection logic.The logic expects exactly one non-hidden, non-metadata directory, which is brittle. If the ZIP structure changes or contains additional folders, this will fail. Consider validating against the expected structure more explicitly.
💡 Suggested improvement
// Find the version folder (ignore metadata folders) var versionFolderName string - var versionFolderCount int for _, entry := range entries { - // Skip macOS metadata folders and hidden folders - if entry.IsDir() && !strings.HasPrefix(entry.Name(), "__") && !strings.HasPrefix(entry.Name(), ".") { - versionFolderName = entry.Name() - versionFolderCount++ + // Look for version folders matching v*.*.* pattern + if entry.IsDir() && regexp.MustCompile(`^v\d+\.\d+\.\d+$`).MatchString(entry.Name()) { + if versionFolderName != "" { + return fmt.Errorf("invalid policy structure: multiple version folders found") + } + versionFolderName = entry.Name() } } - // Ensure we found exactly one version folder - if versionFolderCount != 1 { - return fmt.Errorf("invalid policy structure: expected single version folder, found %d (excluding metadata folders)", versionFolderCount) + if versionFolderName == "" { + return fmt.Errorf("invalid policy structure: no valid version folder found") }Committable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (5)
cli/src/HELP.md-127-130 (1)
127-130: Add language specifier to code block.The code block on line 128 is missing a language specifier. Add
textorplaintextfor output examples.🔎 Proposed fix
**Output:** -``` +```text ap version v0.0.1 (built at 2025-12-16T05:35:30Z) ```cli/src/tests/resources/policy-manifest.yaml-1-1 (1)
1-1: Remove or renamepolicy-manifest.yaml— file contains only a filename reference instead of policy manifest content.This file contains only the text
test-local-policy-manifest.yaml(a filename reference) rather than actual YAML policy manifest content. Tests referencetest-policy-manifest.yamlandtest-local-policy-manifest.yaml, which properly contain policy definitions with version, checksums, and file paths. Either removepolicy-manifest.yamlif it's a duplicate, or rename and populate it with actual policy manifest YAML matching the pattern of other test manifest files in the resources directory.docs/gateway/quick-start-guide.md-36-37 (1)
36-37: Clarify the gateway add command syntax.The comment "requires gateway to be added first with: ap gateway add" is oversimplified. Based on the README, the actual command requires flags like
--display-nameand--server. Consider updating this to provide a more accurate example or reference the full command.🔎 Suggested fix
-# Or use the CLI (requires gateway to be added first with: ap gateway add) +# Or use the CLI (requires gateway to be added first, see "Adding a Gateway" section) ap gateway healthcli/src/cmd/gateway/image/build.go-98-118 (1)
98-118: Edge case:filepath.Basemay return unexpected values.When
manifestPathis/or.,filepath.Basereturns/or.respectively, which would create an invalid or confusing gateway name and image tag.🔎 Proposed validation
func initializeDefaults() error { // Default gateway name from directory name if not provided if gatewayName == "" { absPath, err := filepath.Abs(manifestPath) if err != nil { return fmt.Errorf("failed to get absolute path: %w", err) } gatewayName = filepath.Base(absPath) + if gatewayName == "." || gatewayName == "/" || gatewayName == "" { + return fmt.Errorf("cannot derive gateway name from path '%s'; please specify --name explicitly", manifestPath) + } }cli/src/tests/cmd/gateway_image_build_test.go-166-171 (1)
166-171: Lock file path may not match actual output location.The lock file is generated in
manifestPath(the resources directory), but this test looks for it at"../../policy-manifest-lock.yaml"(relative to test dir). This path doesn't align withabsResourcesDir.🔎 Proposed fix
// Check if lock file was generated - lockFilePath := filepath.Join("..", "..", "policy-manifest-lock.yaml") + lockFilePath := filepath.Join(absResourcesDir, "policy-manifest-lock.yaml") if _, err := os.Stat(lockFilePath); err == nil {
🧹 Nitpick comments (40)
.github/workflows/cli-build-test.yml (1)
42-45: Redundant test execution and builds.Running
make build build-allexecutes tests twice (once inbuildand once inbuild-allwhich depends ontest) and also builds the current platform binary twice. Consider using justmake build-allsince it already runs tests and produces all platform binaries, or usemake test build-all-skip-testsfor a cleaner separation.🔎 Proposed fix
- name: Run build and tests working-directory: ./cli/src run: | - make build build-all + make build-allcli/src/utils/file_utils.go (1)
30-48: Clarify expected checksum format.The function compares raw hex-encoded checksums, but based on the manifest files (e.g.,
policy-manifest-lock-bad.yaml), checksums are stored with asha256:prefix. Callers must strip this prefix before calling this function. Consider either handling the prefix here or documenting the expected format.🔎 Option to handle the prefix within the function
+import "strings" + // VerifyFileChecksum verifies the SHA-256 checksum of a file func VerifyFileChecksum(filePath string, expectedChecksum string) error { + // Strip "sha256:" prefix if present + expectedChecksum = strings.TrimPrefix(expectedChecksum, "sha256:") + file, err := os.Open(filePath)cli/src/internal/gateway/client.go (1)
145-190: Consider extracting a helper to reduce duplication.
PostYAMLandPutYAMLduplicate the URL construction and request creation logic fromPostandPut. While acceptable for now, a helper function could reduce this duplication.🔎 Proposed refactor with helper function
// newRequest creates a new HTTP request with URL construction func (c *Client) newRequest(method, path string, body io.Reader) (*http.Request, error) { baseURL := strings.TrimSuffix(c.gateway.Server, "/") if !strings.HasPrefix(path, "/") { path = "/" + path } url := baseURL + path req, err := http.NewRequest(method, url, body) if err != nil { return nil, fmt.Errorf("failed to create request: %w", err) } return req, nil } // PostYAML performs a POST request with YAML content func (c *Client) PostYAML(path string, body io.Reader) (*http.Response, error) { req, err := c.newRequest("POST", path, body) if err != nil { return nil, err } req.Header.Set("Content-Type", "application/x-yaml") return c.Do(req) }.github/workflows/cli-release.yml (3)
57-72: Add error handling for missing binaries before rename.The rename step assumes all binaries were built successfully. If a build fails silently or partially, the
mvcommands will fail with unclear errors.🔎 Recommended fix to verify binaries exist before renaming
- name: Rename binaries with version working-directory: ./cli/src/build env: VERSION: ${{ github.event.inputs.version }} run: | + # Verify all binaries exist before renaming + BINARIES=( + "ap-darwin-arm64" + "ap-darwin-amd64" + "ap-linux-arm64" + "ap-linux-amd64" + "ap-windows-arm64.exe" + "ap-windows-amd64.exe" + ) + + for binary in "${BINARIES[@]}"; do + if [ ! -f "$binary" ]; then + echo "Error: Binary $binary not found. Build may have failed." + exit 1 + fi + done + # Rename binaries to include version mv ap-darwin-arm64 ap-darwin-arm64-v${VERSION} mv ap-darwin-amd64 ap-darwin-amd64-v${VERSION}
111-115: Provide complete Windows installation instructions.The Windows installation section contains placeholder comments instead of actual instructions, which may confuse users.
🔎 Proposed complete Windows installation instructions
### Windows - ```powershell - # Download the .exe file for your platform - # Rename and move to a directory in your PATH - ``` + + 1. Download the appropriate `.exe` file for your platform + 2. Rename the file to `ap.exe` + 3. Move to a directory in your PATH (e.g., `C:\Windows\System32` or create a new directory and add it to PATH) + + Or using PowerShell: + ```powershell + # Download and rename + Rename-Item -Path "ap-windows-amd64-v${{ github.event.inputs.version }}.exe" -NewName "ap.exe" + # Move to a directory in PATH (requires admin) + Move-Item -Path "ap.exe" -Destination "C:\Windows\System32\" + ```
50-55: Consider running onlybuild-allinstead of both targets sequentially.The
buildandbuild-alltargets are compatible and don't directly conflict. However, running them together is inefficient: tests execute twice (once in thebuildtarget, then again as a dependency ofbuild-all). Sincebuild-allalready produces all platform-specific binaries, thebuildtarget becomes redundant. Use onlymake build-all VERSION=$VERSIONfor the release workflow.cli/src/internal/policy/offline.go (1)
38-44: Consider key collision risk with colon separator.The key format
name:versioncould cause collisions if policy names or versions contain colons. While unlikely, this could lead to incorrect policy resolution.🔎 Alternative approach using a struct key or URL-safe separator
// Create a map of local policy file paths from manifest - localPolicyPaths := make(map[string]string) + type policyKey struct { + name string + version string + } + localPolicyPaths := make(map[policyKey]string) for _, manifestPolicy := range manifest.Policies { if manifestPolicy.IsLocal() { - key := manifestPolicy.Name + ":" + manifestPolicy.Version + key := policyKey{name: manifestPolicy.Name, version: manifestPolicy.Version} localPolicyPaths[key] = manifestPolicy.FilePath } }And update usage at line 60:
- key := lockPolicy.Name + ":" + lockPolicy.Version + key := policyKey{name: lockPolicy.Name, version: lockPolicy.Version}cli/src/utils/policy_cache.go (1)
165-201: Consider returning an error instead of panicking.Line 200 panics if a unique cache path cannot be generated after 100 attempts. While this scenario is extremely unlikely, returning an error would be more graceful and allow callers to handle the situation.
🔎 Proposed change to return error instead of panic
Update the function signature:
-func GenerateUniqueCachePath(policyName string, version string, index *PolicyIndex) string { +func GenerateUniqueCachePath(policyName string, version string, index *PolicyIndex) (string, error) {And update the return statements:
if path, versionExists := versions[version]; versionExists { // Already cached with this path - return path + return path, nil } } // Check for path collision with other policies basePath := filepath.Join(baseKebab, version) if !isPathCollidingWithOtherPolicy(basePath, policyName, index) { - return basePath + return basePath, nil } // Collision detected, generate unique suffix attempt := policyName for i := 0; i < 100; i++ { // Reasonable limit to prevent infinite loop hash := HashString(attempt) candidateKebab := fmt.Sprintf("%s-%s", baseKebab, hash) candidatePath := filepath.Join(candidateKebab, version) if !isPathCollidingWithOtherPolicy(candidatePath, policyName, index) { - return candidatePath + return candidatePath, nil } // Hash the hash for next iteration attempt = hash } // This should never happen, but just in case - panic(fmt.Sprintf("failed to generate unique cache path for policy %s after 100 attempts", policyName)) + return "", fmt.Errorf("failed to generate unique cache path for policy %s after 100 attempts", policyName) }Note: This will require updating all callers to handle the error return.
cli/src/cmd/gateway/api/root.go (1)
1-47: LGTM! Clean command wiring.The API command root follows standard Cobra patterns with proper initialization and subcommand registration (list, get, delete). The structure integrates well with the gateway command hierarchy.
Optional: Consider expanding APICmdExample to showcase more commands
The current example only demonstrates
ap gateway api list. Consider adding examples for get and delete operations to provide users with a more comprehensive quick reference:const ( APICmdLiteral = "api" APICmdExample = `# List all APIs -ap gateway api list` +ap gateway api list + +# Get an API by ID +ap gateway api get --id <api-id> + +# Delete an API +ap gateway api delete --id <api-id> --confirm` )cli/src/policy-manifest-lock.yaml.good (1)
1-12: Clarify the purpose of the .good file extension.The
.goodextension is unusual for a YAML file. This appears to be a test fixture or reference file. Consider adding a comment at the top of the file explaining its purpose, or renaming it to follow a more conventional naming pattern (e.g.,policy-manifest-lock.yaml.goldenor moving it to a test fixtures directory with a README).cli/src/internal/gateway/policy_cleaner.go (1)
34-46: Consider adding nil safety checks.The function will panic if
manifestormanifest.Policiesis nil. While the caller may ensure this never happens, adding a defensive check improves robustness.🔎 Suggested defensive check
func CleanPolicyManifestForPolicyHub(manifest *PolicyManifest) ([]byte, error) { + if manifest == nil { + return nil, fmt.Errorf("manifest cannot be nil") + } + cleanedPolicies := make([]CleanedPolicy, len(manifest.Policies))cli/README.md (2)
66-80: Add language specifiers to fenced code blocks.The fenced code blocks on lines 67-77 should specify a language for proper syntax highlighting and markdown compliance.
🔎 Suggested fix
-**Note:** `make build` and `make build-all` run all tests to completion and display a summary table: -``` +**Note:** `make build` and `make build-all` run all tests to completion and display a summary table: +```text =============================================== Test Results Summary
85-96: Add language specifier to the bash code block.The fenced code block on line 86 should specify
bashas the language.🔎 Suggested fix
### Testing The CLI includes automated tests that can be configured via `tests/test-config.yaml`: -```bash +```bash # Run all testsActually, it looks like it already has
bashspecified. Let me re-check the static analysis hint - it says line 86-86 is missing the language. Looking at the code, line 86 is indeed:So the opening fence at line 85 has bash, but perhaps there's another block. Let me look more carefully at the annotated code around line 86.
Looking at lines 85-96, the block starts with
```bashon line 85, so that should be fine. The static analysis might be incorrectly flagging it, or there's a different block. I'll skip this as it appears to be a false positive.cli/src/cmd/gateway/mcp/delete.go (1)
83-103: Minor: Response body read is unnecessary for success cases.The body is read for all status codes (lines 83-87), but for 200/204 success cases (lines 94-103), the body content is not used. While harmless, this is a minor inefficiency.
🔎 Suggested optimization
// Read the response body - body, err := io.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("failed to read response: %w", err) - } - // Handle different status codes if resp.StatusCode == 404 { + body, _ := io.ReadAll(resp.Body) return fmt.Errorf("MCP with ID '%s' not found", deleteMCPID) } if resp.StatusCode != 200 && resp.StatusCode != 204 { + body, err := io.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("failed to read response: %w", err) + } // Try to parse error message from responsecli/src/tests/TESTING.md (2)
9-16: Add language specifier to fenced code block.The fenced code block should specify a language (e.g.,
textor leave it as-is if it's meant to show directory structure).🔎 Suggested fix
## Test Structure -``` +```text tests/
115-118: Add language specifier to fenced code block.The fenced code block should specify a language (e.g.,
text) for proper rendering.🔎 Suggested fix
If tests fail, the build process will display: -``` +```text ⚠ Some tests failed. Check logs at: tests/logs/test-results.logcli/src/internal/policy/local.go (1)
73-77: Consider validating name/version consistency.
ValidateLocalPolicyZipreturns thenameandversionfrompolicy-definition.yaml, but they're discarded here. If the manifest specifies a different name/version than the ZIP contains, this inconsistency goes undetected and could cause confusion during policy resolution.Consider comparing the returned values against
policy.Nameandpolicy.Version, or at minimum logging a warning if they differ.🔎 Proposed enhancement
- // Validate zip structure using central util so validation can be changed later - _, _, err = utils.ValidateLocalPolicyZip(zipPath) + // Validate zip structure and verify name/version match + zipName, zipVersion, err := utils.ValidateLocalPolicyZip(zipPath) if err != nil { return nil, fmt.Errorf("policy %s: validation failed: %w\n\nLocal policies must be a zip file containing a policy-definition.yaml at the root. The policy-definition.yaml must include 'name' and 'version' fields.", policy.Name, err) } + if zipName != policy.Name { + return nil, fmt.Errorf("policy name mismatch: manifest specifies '%s' but zip contains '%s'", policy.Name, zipName) + } + if policy.Version != "" && zipVersion != policy.Version { + fmt.Printf("warning: version mismatch for %s: manifest '%s', zip '%s'\n", policy.Name, policy.Version, zipVersion) + }cli/src/utils/docker_utils.go (1)
46-55: Consider handling nillogFileparameter.If
logFileis nil, settingcmd.Stdout = logFileresults in output being discarded (which may be intentional). However, this implicit behavior could be unexpected. Consider documenting this behavior or explicitly handling the nil case.🔎 Option: Add nil check or document behavior
// RunDockerCommand runs a docker command and logs output to the provided file +// If logFile is nil, stdout and stderr are discarded. func RunDockerCommand(args []string, logFile *os.File) error { cmd := exec.Command("docker", args...) - cmd.Stdout = logFile - cmd.Stderr = logFile + if logFile != nil { + cmd.Stdout = logFile + cmd.Stderr = logFile + } if err := cmd.Run(); err != nil { return fmt.Errorf("docker command failed: %w", err) } return nil }cli/src/cmd/gateway/health.go (1)
109-115: Consider case-insensitive status comparison.The status comparison is case-sensitive. If the gateway returns
"Healthy"or"UP"instead of"healthy"or"up", the logic falls through to "unclear" status.🔎 Proposed fix for case-insensitive comparison
+import "strings" + // Default message based on status -if status == "healthy" || status == "up" { +statusLower := strings.ToLower(status) +if statusLower == "healthy" || statusLower == "up" { fmt.Println("Gateway is healthy.") -} else if status == "unhealthy" || status == "down" { +} else if statusLower == "unhealthy" || statusLower == "down" { fmt.Println("Gateway is unhealthy.") } else { fmt.Println("Gateway health status is unclear.") }cli/src/tests/cmd/gateway_mcp_generate_test.go (2)
97-111: Fragile readiness detection logic.The readiness check uses
len(logsStr) > 100as a fallback condition, which is arbitrary and could produce false positives (e.g., error output that's long enough). Also, thereadyvariable check at line 113-115 is unreachable since line 108 callst.Fatalwheni == 29.🔎 Proposed improvement
- if strings.Contains(logsStr, "Server started") || - strings.Contains(logsStr, "listening") || - strings.Contains(logsStr, "started") || - len(logsStr) > 100 { // Container is producing output + if strings.Contains(logsStr, "Server started") || + strings.Contains(logsStr, "listening") || + strings.Contains(logsStr, "started") { ready = true t.Log("✓ MCP server is ready") break } - if i == 29 { - t.Logf("Container logs:\n%s", logsStr) - t.Fatal("MCP server did not start within 30 seconds") - } time.Sleep(1 * time.Second) } if !ready { + checkCmd := exec.Command("docker", "logs", containerName) + logs, _ := checkCmd.CombinedOutput() + t.Logf("Container logs:\n%s", string(logs)) t.Fatal("MCP server did not become ready") }
93-95: Silently ignoring errors when saving debug logs.The errors from
os.MkdirAllandos.WriteFileare ignored. While not critical for test execution, this could mask issues when debugging test failures.🔎 Optional: Log errors without failing
// Save logs for debugging - os.MkdirAll("logs", 0755) - os.WriteFile("logs/mcp-container.log", logs, 0644) + if err := os.MkdirAll("logs", 0755); err != nil { + t.Logf("Warning: failed to create logs directory: %v", err) + } else if err := os.WriteFile("logs/mcp-container.log", logs, 0644); err != nil { + t.Logf("Warning: failed to write container logs: %v", err) + }cli/src/cmd/gateway/mcp/list.go (1)
100-117: Potential inconsistency betweenCountfield and actual slice length.The code checks
listResp.Count == 0for the empty case but then iterates overlistResp.MCPProxies. If the server returns mismatched values (e.g.,Count: 0but non-emptyMCPProxies), the behavior would be inconsistent. Consider using the slice length consistently.🔎 Proposed fix for consistency
// Display the MCPs - if listResp.Count == 0 { + if len(listResp.MCPProxies) == 0 { fmt.Println("No MCPs found on the gateway.") return nil }cli/src/internal/gateway/policy_manifest.go (1)
50-53: Redundant nil check.The check
manifest.Policies == nil || len(manifest.Policies) == 0is redundant sincelen(nil)returns 0 for a nil slice. Simplify to just the length check.🔎 Proposed fix
// Validate policies array - if manifest.Policies == nil || len(manifest.Policies) == 0 { + if len(manifest.Policies) == 0 { return fmt.Errorf("'policies' array is required and must not be empty") }cli/src/internal/gateway/docker_build.go (2)
90-108: Potential path issues and missing command timeout.
Line 91 uses string concatenation for the volume mount (
config.TempDir + ":/workspace"). IfTempDircontains special characters or spaces, this could cause issues.Docker commands have no timeout, which could cause the build process to hang indefinitely if Docker becomes unresponsive.
🔎 Proposed improvements
+import ( + "context" + "time" +) + func runGatewayBuilder(config DockerBuildConfig, logFile *os.File) error { - args := []string{"run", "--rm", "-v", config.TempDir + ":/workspace", config.GatewayBuilder} + volumeMount := fmt.Sprintf("%s:/workspace", config.TempDir) + args := []string{"run", "--rm", "-v", volumeMount, config.GatewayBuilder} if config.GatewayControllerBaseImage != "" { args = append(args, "-gateway-controller-base-image", config.GatewayControllerBaseImage) } if config.RouterBaseImage != "" { args = append(args, "-router-base-image", config.RouterBaseImage) } - cmd := exec.Command("docker", args...) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute) + defer cancel() + cmd := exec.CommandContext(ctx, "docker", args...) cmd.Stdout = logFile cmd.Stderr = logFile
140-189: Consider extracting repeated image tag construction.The image tag construction
fmt.Sprintf("%s/%s-%s:%s", config.ImageRepository, config.GatewayName, component, config.GatewayVersion)is repeated inbuildWithBuildx,buildWithDocker, andpushImages.🔎 Proposed helper function
+// getImageTag constructs the full image tag for a component +func getImageTag(config DockerBuildConfig, component string) string { + return fmt.Sprintf("%s/%s-%s:%s", config.ImageRepository, config.GatewayName, component, config.GatewayVersion) +} + func buildWithDocker(config DockerBuildConfig, components []string, logFile *os.File) error { fmt.Println(" → Building images...") for _, component := range components { - imageTag := fmt.Sprintf("%s/%s-%s:%s", config.ImageRepository, config.GatewayName, component, config.GatewayVersion) + imageTag := getImageTag(config, component)cli/src/cmd/gateway/api/list.go (2)
106-117: Consider displaying the UpdatedAt field.The
UpdatedAtfield is parsed from the API response but not displayed in the output. For consistency withCreatedAt(Line 113), consider adding it to the display.🔎 Proposed addition
fmt.Printf(" Status: %s\n", api.Status) fmt.Printf(" Created At: %s\n", api.CreatedAt) + fmt.Printf(" Updated At: %s\n", api.UpdatedAt) if i < len(listResp.APIs)-1 {
90-92: Use http.StatusOK constant instead of hardcoded 200.For better readability and maintainability, prefer using the
http.StatusOKconstant.🔎 Proposed change
- if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { return fmt.Errorf("failed to list APIs (status %d): %s", resp.StatusCode, string(body)) }cli/src/cmd/gateway/apply.go (1)
162-174: Consider using http.Status constants for clarity.The status code check could use standard library constants for better readability.
🔎 Proposed change
- if resp.StatusCode < 200 || resp.StatusCode >= 300 { + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { // Try to parse error message from responsecli/src/cmd/gateway/mcp/get.go (1)
139-145: Use http.Status constants instead of hardcoded values.For consistency and readability, prefer using
http.StatusNotFoundandhttp.StatusOKconstants.🔎 Proposed changes
- if resp.StatusCode == 404 { + if resp.StatusCode == http.StatusNotFound { return nil, fmt.Errorf("MCP with ID '%s' not found", id) } - if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("failed to get MCP (status %d): %s", resp.StatusCode, string(body)) }cli/src/cmd/gateway/image/build.go (1)
386-459: Consider consolidating display summary functions.
displayOfflineBuildSummaryanddisplayBuildSummaryshare significant code. Consider extracting common logic into a helper or merging them with conditional differences.cli/src/cmd/gateway/build.go (2)
166-196: Redundant required flag validation.Cobra's
MarkFlagRequired(lines 79-82) already validates these flags beforeRunis invoked. The manual checks instepPrepareBuildConfigare redundant and will never trigger unless the Cobra configuration is bypassed.🔎 Proposed simplification
// stepPrepareBuildConfig validates and prepares the build configuration func stepPrepareBuildConfig() error { fmt.Println("[2/5] Preparing Build Configuration") - // Validate required flags - if buildDockerRegistry == "" { - return fmt.Errorf("docker-registry is required") - } - if buildImageTag == "" { - return fmt.Errorf("image-tag is required") - } - if buildGatewayBuilder == "" { - return fmt.Errorf("gateway-builder is required") - } - // Display configuration fmt.Printf(" Docker Registry: %s\n", buildDockerRegistry)
308-319: Stub implementation with TODO comment.This step is a placeholder. Consider adding an `` tag or tracking issue for completing the Docker build context preparation.
Would you like me to open an issue to track the completion of gateway build artifact preparation?
cli/src/tests/cmd/gateway_image_build_test.go (1)
41-64: Consider caching test config.
isTestEnabledreads and parses the YAML config file on every call. For test efficiency, consider caching the config usingsync.Onceor a package-level variable initialized inTestMain.cli/src/internal/policy/hub.go (2)
342-352: Usestrings.Joininstead of customjoinhelper.The standard library provides
strings.Joinwhich does exactly this.🔎 Proposed fix
Remove the
joinfunction and replace usages:- errMsg := fmt.Sprintf("no policies were resolved successfully from PolicyHub\n\nRequested policies:\n%s\n\n...", - join(requestedPolicies, "\n"), + errMsg := fmt.Sprintf("no policies were resolved successfully from PolicyHub\n\nRequested policies:\n%s\n\n...", + strings.Join(requestedPolicies, "\n"),Note: This requires changing
requestedPoliciesto[]stringor building the string inline.
173-312: Complex function with multiple responsibilities.
downloadAndVerifyPolicieshandles index loading, cache checking, checksum verification, downloading, and index saving. Consider extracting smaller helpers (e.g.,processSinglePolicy) for better testability and readability.cli/src/internal/policyhub/client.go (3)
56-62: Recommend using a more specific type for the Error field.The
Errorfield usesinterface{}which provides no type safety. Consider using*stringor a structured error type to enable proper error handling and avoid nil-pointer dereferences.🔎 Proposed fix
type ResolveResponse struct { Data []PolicyData `json:"data"` - Error interface{} `json:"error"` + Error *string `json:"error,omitempty"` Meta Meta `json:"meta"` Success bool `json:"success"` }Then update the error check at Line 113:
if !response.Success { - return nil, fmt.Errorf("PolicyHub API returned error: %v", response.Error) + errMsg := "unknown error" + if response.Error != nil { + errMsg = *response.Error + } + return nil, fmt.Errorf("PolicyHub API returned error: %s", errMsg) }
70-78: Consider making the timeout configurable.The hardcoded 30-second timeout might be insufficient for large policy downloads over slow connections. Consider making it configurable via a parameter or environment variable.
🔎 Example implementation
-func NewPolicyHubClient() *PolicyHubClient { +func NewPolicyHubClient(timeout time.Duration) *PolicyHubClient { + if timeout == 0 { + timeout = 30 * time.Second + } return &PolicyHubClient{ httpClient: &http.Client{ - Timeout: 30 * time.Second, + Timeout: timeout, }, baseURL: utils.PolicyHubBaseURL, } }
128-140: Consider less restrictive directory permissions.The directory is created with
0700(owner-only access), which may cause issues in containerized environments or when running as different users. Consider0755unless there's a specific security requirement.🔎 Proposed change
- if err := os.MkdirAll(policiesDir, 0700); err != nil { + if err := os.MkdirAll(policiesDir, 0755); err != nil { return "", fmt.Errorf("failed to create policies directory: %w", err) }cli/src/utils/policy_utils.go (2)
107-111: Add semver validation for policy versions.The code adds a 'v' prefix to versions but doesn't validate the semver format. Invalid versions like "abc" or "1.2.3.4.5" would pass through unchecked.
🔎 Validation example
// Normalize version to include 'v' prefix for returned value yamlVersion := policyDef.Version if !strings.HasPrefix(yamlVersion, "v") { yamlVersion = "v" + yamlVersion } + + // Validate semver format (basic check) + versionPattern := regexp.MustCompile(`^v?\d+\.\d+\.\d+(-[a-zA-Z0-9.-]+)?$`) + if !versionPattern.MatchString(yamlVersion) { + return "", "", fmt.Errorf("invalid version format: %s (expected semver like v1.0.0)", yamlVersion) + } return policyDef.Name, yamlVersion, nil
498-570: Remove theCopyPolicyToTempGatewayImageBuildfunction immediately.The function has zero usages in the codebase. The replacement function
CopyPolicyToWorkspaceis already deployed and actively used incli/src/cmd/gateway/image/build.go. There is no migration work needed—the function can be safely deleted.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/src/tests/cmd/gateway_image_build_test.go(1 hunks)cli/src/tests/cmd/gateway_mcp_generate_test.go(1 hunks)cli/src/tests/resources/test-local-policy-manifest.yaml(1 hunks)cli/src/tests/resources/test-mixed-manifest.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cli/src/tests/resources/test-mixed-manifest.yaml
- cli/src/tests/resources/test-local-policy-manifest.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
cli/src/tests/cmd/gateway_mcp_generate_test.go (1)
cli/src/utils/docker_utils.go (1)
IsDockerAvailable(28-34)
🔇 Additional comments (9)
cli/src/tests/cmd/gateway_mcp_generate_test.go (6)
32-51: LGTM!The test initialization is well-structured with proper gating for test enablement, Docker availability, and binary existence. The error messages are clear and guide users appropriately.
53-75: LGTM!Docker container setup is properly implemented with cleanup of existing containers and clear error handling. The port mapping and logging provide good visibility for debugging.
77-85: LGTM!Proper use of defer to ensure container cleanup. Ignoring errors during cleanup is acceptable since the test is already completing.
122-125: LGTM!Output directory management is properly implemented with cleanup of previous test artifacts and deferred cleanup for the current test run.
127-143: LGTM!Command execution is properly implemented with clear error handling and output logging for debugging. The command arguments align with the MCP generate CLI interface.
145-167: LGTM!Output validation properly verifies that the command created the output directory and generated files. The logging of generated entries provides good visibility for debugging.
cli/src/tests/cmd/gateway_image_build_test.go (3)
31-64: LGTM! Good default-to-enabled design.The test configuration infrastructure is well-designed. Defaulting to enabled when the config is missing or malformed ensures tests run by default, which is the right behavior for CI/CD pipelines.
360-473: LGTM! Good two-phase test design.The test correctly simulates the offline mode workflow by first running online mode to generate the lock file, then testing offline mode. The use of
deferfor cleanup at Line 412 is good practice.
476-525: LGTM! Comprehensive help validation.The test properly validates that all important flags are documented in the help output.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/Makefile (1)
42-42: Add new targets to.PHONYdeclaration.The new targets
build-skip-testsandbuild-all-skip-testsshould be added to the.PHONYdeclaration to ensure they are always executed regardless of file existence.🔎 Proposed fix
-.PHONY: all build clean test deps help install build-all build-macos build-linux build-windows +.PHONY: all build clean test deps help install build-all build-macos build-linux build-windows build-skip-tests build-all-skip-tests
♻️ Duplicate comments (2)
cli/src/utils/constants.go (1)
57-62: Critical: Development URL still hardcoded as default.The PolicyHubBaseURLDefault points to a development environment (contains "dev" subdomain and "-dev" in domain). While Line 60 provides an environment variable override (WSO2AP_POLICYHUB_BASE_URL), the default will cause failures in production deployments where the environment variable is not set.
This issue was previously flagged and remains unresolved. The default URL should either:
- Point to a production endpoint, or
- Be empty with required environment variable validation at startup
cli/src/utils/policy_utils.go (1)
380-387: ZipSlip vulnerability mitigation is incomplete.This issue was flagged in a previous review. The current check occurs after
filepath.Join, but absolute paths inf.Name(e.g.,/etc/passwd) are not rejected before joining. Additionally,strings.HasPrefixfor path validation can be unreliable.
🧹 Nitpick comments (8)
cli/src/tests/cmd/test_helpers.go (1)
22-27: Relative path may break depending on test execution context.The path
filepath.Join("..", "test-config.yaml")assumes tests are executed from thecli/src/tests/cmddirectory. If tests are run from a different working directory (e.g., repository root viago test ./...), the config file won't be found and all tests will be silently skipped.Consider using
runtime.Callerto resolve the path relative to this source file, or embed the config.🔎 Proposed fix using runtime.Caller
import ( "os" "path/filepath" + "runtime" "gopkg.in/yaml.v3" )func isTestEnabled(name string) bool { - cfgPath := filepath.Join("..", "test-config.yaml") + _, thisFile, _, ok := runtime.Caller(0) + if !ok { + return false + } + cfgPath := filepath.Join(filepath.Dir(thisFile), "..", "test-config.yaml") data, err := os.ReadFile(cfgPath)cli/src/tests/cmd/gateway_mcp_generate_test.go (2)
56-57: Hardcoded container name may cause issues with parallel test execution.If multiple instances of this test run concurrently (e.g., different CI jobs sharing the same Docker daemon), the hardcoded
containerName := "everything-test"could cause conflicts. Consider using a unique suffix (e.g., process ID or UUID) if parallel execution is expected.
120-121: Consider documenting the rationale for the additional 2-second delay.The HTTP health check should suffice for readiness. If this delay is needed for a specific reason (e.g., internal initialization after the endpoint becomes available), a brief comment would help future maintainers understand why it's necessary.
cli/src/Makefile (1)
76-82: Good addition for development workflow.The
build-skip-teststarget provides a useful escape hatch for quick iteration during development. However, consider extracting the common build commands to reduce duplication betweenbuildandbuild-skip-teststargets.💡 Optional: Reduce duplication with a helper target
+## _do-build: Internal target for actual build commands +_do-build: + @echo "Building $(BINARY_NAME)..." + @mkdir -p $(BUILD_DIR) + $(GOBUILD) $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME) main.go + @echo "Build complete: $(BUILD_DIR)/$(BINARY_NAME)" + ## build: Build the binary for current platform (runs tests first) build: - @echo "" - @echo "Building $(BINARY_NAME)..." - @mkdir -p $(BUILD_DIR) - $(GOBUILD) $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME) main.go - @echo "Build complete: $(BUILD_DIR)/$(BINARY_NAME)" + # ... test logic ... + @$(MAKE) _do-build ## build-skip-tests: Build the binary without running tests -build-skip-tests: - @echo "Building $(BINARY_NAME) (skipping tests)..." - @mkdir -p $(BUILD_DIR) - $(GOBUILD) $(LDFLAGS) -o $(BUILD_DIR)/$(BINARY_NAME) main.go - @echo "Build complete: $(BUILD_DIR)/$(BINARY_NAME)" +build-skip-tests: _do-buildcli/src/internal/policy/hub.go (1)
360-370: Use standard librarystrings.Joininstead.The
joinhelper reimplements functionality already available in the standard library. Replace withstrings.Jointo reduce code duplication and improve maintainability.🔎 Suggested change
Remove the custom join function and use strings.Join:
-// join is a helper function to join strings with a separator -func join(items []string, sep string) string { - result := "" - for i, item := range items { - if i > 0 { - result += sep - } - result += item - } - return result -}Then update line 81 and 158 to use
strings.Join:- join(requestedPolicies, "\n"), + strings.Join(requestedPolicies, "\n"),cli/src/utils/policy_utils.go (3)
183-237: Consider handling symbolic links explicitly.
filepath.Walkfollows symbolic links, which could lead to:
- Including unintended content from outside the source directory
- Infinite loops with circular symlinks
For policy directories, this may be low risk, but consider using
filepath.WalkDirwithfs.DirEntry.Type()to detect and skip or handle symlinks appropriately.
627-653: Symbolic links are copied as regular files.When
entry.IsDir()is false for a symlink, it falls through tocopyFile, which will either fail or copy the symlink target's content rather than preserving the symlink. This is consistent withZipDirectory, but may cause unexpected behavior if policy directories contain symlinks.
526-530: Remove deprecatedCopyPolicyToTempGatewayImageBuildfunction.
CopyPolicyToTempGatewayImageBuildis marked deprecated in favor ofCopyPolicyToWorkspaceand is not referenced anywhere in the codebase. Consider removing it to reduce maintenance burden.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cli/src/Makefile(2 hunks)cli/src/cmd/gateway/apply.go(1 hunks)cli/src/cmd/gateway/image/build.go(1 hunks)cli/src/cmd/root.go(2 hunks)cli/src/internal/policy/hub.go(1 hunks)cli/src/internal/policy/local.go(1 hunks)cli/src/internal/policy/offline.go(1 hunks)cli/src/internal/policyhub/client.go(1 hunks)cli/src/tests/cmd/gateway_mcp_generate_test.go(1 hunks)cli/src/tests/cmd/test_helpers.go(1 hunks)cli/src/tests/test-config.yaml(1 hunks)cli/src/utils/constants.go(1 hunks)cli/src/utils/env.go(1 hunks)cli/src/utils/policy_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/src/internal/policy/offline.go
- cli/src/cmd/gateway/apply.go
- cli/src/cmd/gateway/image/build.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-03T08:19:08.315Z
Learnt from: RakhithaRR
Repo: wso2/api-platform PR: 275
File: cli/src/cmd/version.go:32-39
Timestamp: 2025-12-03T08:19:08.315Z
Learning: In the cli/src project (fusionctl CLI), the Version variable in cli/src/cmd/version.go is set via linker flags at build time from the Makefile without a "v" prefix (e.g., "0.0.1"), so the hardcoded "v" prefix in the version output format string is intentional and correct.
Applied to files:
cli/src/cmd/root.go
🧬 Code graph analysis (6)
cli/src/utils/env.go (1)
cli/src/utils/constants.go (2)
PolicyHubEnvVar(60-60)PolicyHubBaseURLDefault(61-61)
cli/src/tests/cmd/gateway_mcp_generate_test.go (2)
cli/src/utils/docker_utils.go (1)
IsDockerAvailable(28-34)sdk/gateway/policy/v1alpha/context.go (1)
Body(4-16)
cli/src/internal/policy/hub.go (5)
cli/src/internal/policyhub/client.go (1)
Checksum(36-39)cli/src/utils/env.go (1)
GetPolicyHubBaseURL(51-56)cli/src/utils/constants.go (1)
PolicyHubResolvePath(59-59)cli/src/utils/policy_cache.go (7)
LoadPolicyIndex(96-130)ValidateVersionFormat(80-93)GetPolicyFromIndex(229-236)GetPolicyCachePath(239-246)GenerateUniqueCachePath(167-201)AddPolicyToIndex(220-225)SavePolicyIndex(133-163)cli/src/utils/policy_utils.go (2)
FormatPolicyFileName(302-304)EnsureDir(307-314)
cli/src/internal/policyhub/client.go (2)
cli/src/utils/env.go (1)
GetPolicyHubBaseURL(51-56)cli/src/utils/constants.go (2)
PolicyHubResolvePath(59-59)PoliciesCachePath(27-27)
cli/src/cmd/root.go (1)
cli/src/utils/defaults.go (1)
BuildTime(25-25)
cli/src/internal/policy/local.go (3)
cli/src/internal/policy/types.go (2)
ManifestPolicy(29-34)ProcessedPolicy(68-76)cli/src/utils/policy_utils.go (3)
ZipDirectory(171-238)ValidateLocalPolicyZip(61-142)CalculateSHA256(145-159)cli/src/internal/policyhub/client.go (1)
Checksum(36-39)
🪛 checkmake (0.2.2)
cli/src/Makefile
[warning] 47-47: Target body for "build" exceeds allowed length of 5 (27).
(maxbodylength)
🔇 Additional comments (24)
cli/src/tests/test-config.yaml (1)
5-14: LGTM! Clean configuration structure.The test configuration is well-documented. Minor observation: the test naming convention is inconsistent (
TestFlagValuesUniquevsCmd-GatewayMcpGenerate). Consider standardizing on one style for maintainability. Also, thedependenciesfield (line 13-14) is defined here but not consumed byisTestEnabled()in test_helpers.go—this is fine for documentation purposes, but if you plan to gate tests based on dependencies, you'll need to extend the helper.cli/src/tests/cmd/test_helpers.go (1)
32-37: LGTM! Clean lookup logic.The iteration and matching logic is straightforward and correct.
cli/src/tests/cmd/gateway_mcp_generate_test.go (4)
33-44: LGTM! Good test enablement and Docker availability checks.The test properly gates execution based on the YAML config and Docker availability, with appropriate skip messages.
88-118: HTTP health check approach addresses previous review concerns.The readiness check now uses an HTTP probe instead of log length heuristics, and file operation errors are properly handled. This is a significant improvement over the previous implementation.
130-144: LGTM! Command execution and validation are well-structured.The test correctly invokes the CLI command and validates both exit status and output with clear error messages.
153-168: LGTM! Output validation is thorough.Good verification that files are actually generated, with helpful debug logging of the generated entries.
cli/src/Makefile (3)
83-85: LGTM! Tests now run before multi-platform builds.The explicit dependency on the
testtarget ensures tests are executed before building for all platforms, which is consistent with the single-platformbuildtarget behavior and follows best practices.
87-90: LGTM! Consistent with the single-platform skip-tests variant.The
build-all-skip-teststarget provides a useful option for CI/CD scenarios where tests are run in a separate pipeline step, allowing faster multi-platform builds when tests have already been validated.
19-19: Binary rename is properly propagated across the codebase.The binary name change from
apipctltoaphas been completely applied. No remaining references to the old binary name exist in documentation, configuration files, shell scripts, Go source files, or other locations in the repository. The Makefile consistently uses the new name throughout all build targets.cli/src/cmd/root.go (1)
31-63: LGTM! Consistent CLI renaming throughout.The renaming from "apipctl" to "ap" is consistently applied across all command definitions, help text, and examples. The version formatting follows the established pattern from the codebase where the "v" prefix is in the format string rather than the Version variable.
cli/src/utils/constants.go (2)
21-29: LGTM! CLI naming and configuration paths are well-structured.The CLI rename to "ap" is consistent with root.go, and the new WSO2AP configuration paths follow a logical structure under
.wso2ap/.
31-55: Gateway constants are well-organized.The gateway configuration defaults, REST API endpoint paths, environment variable names, and checksum verification flag are clearly defined and appropriately scoped.
cli/src/utils/env.go (1)
49-56: LGTM! Environment override pattern correctly implemented.The function properly implements the environment variable override pattern, checking WSO2AP_POLICYHUB_BASE_URL before falling back to the default. The implementation is clean and straightforward.
Note: The default value issue (dev URL in PolicyHubBaseURLDefault) is addressed separately in the constants.go review.
cli/src/internal/policy/hub.go (4)
59-96: Well-structured policy processing workflow.The orchestration logic properly handles empty inputs, provides detailed error messages when no policies are resolved, and cleanly separates the resolution and download phases.
98-172: Comprehensive error handling in PolicyHub resolution.The function includes good practices:
- Request deduplication to avoid redundant API calls
- Detailed error messages including the requested policies and PolicyHub responses
- Proper HTTP client timeout configuration
174-311: Thorough cache and checksum verification logic.The download and verification workflow correctly:
- Checks the local cache before downloading
- Conditionally verifies checksums based on configuration
- Handles cache misses and checksum mismatches gracefully
- Provides clear progress and summary messages
313-358: Robust download implementation with defensive validation.The function includes good defensive coding:
- Validates HTTP status codes with error context
- Checks Content-Type headers when present (line 332)
- Uses io.LimitReader to safely read error responses (line 323)
- Ensures destination directories exist before writing
cli/src/internal/policyhub/client.go (2)
64-117: Well-structured PolicyHub client.The client properly encapsulates HTTP interactions with PolicyHub, includes appropriate timeouts, sets correct headers, and validates both HTTP status and the API's success flag.
119-171: Clean helper functions for policy management.The directory management helpers use appropriate permissions (0700 for sensitive policy cache), and the path construction utilities are straightforward and correct.
cli/src/utils/policy_utils.go (5)
61-142: LGTM!The validation logic is well-structured with proper handling of nested directory layouts and macOS metadata exclusion. Temp directory cleanup is correctly deferred.
144-168: LGTM!Clean implementation of SHA-256 calculation and verification with proper resource cleanup.
306-366: LGTM!Directory management utilities are well-implemented with proper error handling. The pattern of "ensure exists, return path" is consistent across functions.
437-483: LGTM!The setup function properly cleans existing directories before recreation and creates the expected directory structure with the lock file.
489-524: LGTM!Clean implementation with proper temp directory cleanup and a clear return value for the workspace relative path.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/src/tests/cmd/gateway_mcp_generate_test.go (2)
66-98: Solid Docker container lifecycle management!The test properly cleans up existing containers before starting and ensures cleanup after completion via defer. The container management follows good practices for test isolation.
Consider adding a brief comment at line 73 explaining why the cleanup error is intentionally ignored (container might not exist from previous runs).
🔎 Optional enhancement
// Clean up any existing container with the same name cleanupCmd := exec.Command("docker", "rm", "-f", containerName) - cleanupCmd.Run() // Ignore errors if container doesn't exist + cleanupCmd.Run() // Intentionally ignore errors - container might not exist from previous run
132-133: Consider removing the extra initialization sleep.The 2-second sleep after a successful health check may be unnecessary. If the HTTP health check at
/mcpreliably indicates the server is ready to handle requests, this adds needless delay to test execution.🔎 Optional optimization
If the health check is truly reliable, consider removing this sleep:
- // Give the server a bit more time to fully initialize - time.Sleep(2 * time.Second) - // Create output directoryIf there's a specific initialization concern, consider making the health check endpoint more robust or adding a comment explaining what this wait addresses.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/tests/cmd/gateway_mcp_generate_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T08:52:52.909Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 142
File: platform-api/src/resources/openapi.yaml:905-969
Timestamp: 2025-11-12T08:52:52.909Z
Learning: In the wso2/api-platform repository, the team follows an API-first development approach where OpenAPI specs may document planned features before backend implementation is complete, allowing frontend development to proceed against the intended API contract without requiring changes later.
Applied to files:
cli/src/tests/cmd/gateway_mcp_generate_test.go
🧬 Code graph analysis (1)
cli/src/tests/cmd/gateway_mcp_generate_test.go (2)
cli/src/utils/docker_utils.go (1)
IsDockerAvailable(28-34)sdk/gateway/policy/v1alpha/context.go (1)
Body(4-16)
🔇 Additional comments (5)
cli/src/tests/cmd/gateway_mcp_generate_test.go (5)
33-44: Excellent test prerequisites handling!The test properly validates enablement config and Docker availability before proceeding, with clear skip messages. This ensures the test fails gracefully in environments where prerequisites aren't met.
46-64: Smart fallback binary build logic!The test gracefully handles missing binaries by building them on-the-fly, making it more robust for CI environments. The defensive check at line 61 ensures the build actually succeeded. Relative paths are correctly calculated from the test file location.
100-130: Great improvement to health check reliability!The HTTP-based health check properly addresses the previous review concerns about weak readiness heuristics. The logic correctly handles response bodies, status codes, and timeout scenarios with appropriate log dumping on failure.
135-156: Well-structured command execution and error handling!The test properly sets up the output directory, executes the command with appropriate flags, and handles errors with detailed logging. The deferred cleanup ensures test isolation.
158-183: Thorough output validation with helpful debugging!The test validates that the command successfully created output files and provides detailed logging of generated artifacts. The validation criteria (directory exists + at least one file) are appropriate for verifying the command succeeded without being overly prescriptive about exact output structure.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cli/src/utils/policy_utils.go (1)
319-326: ZipSlip vulnerability: Validate entry paths before joining.The current check validates paths after
filepath.Join, which is too late. Iff.Nameis an absolute path like/etc/passwd,filepath.Joinwill use it as-is on some platforms. Additionally, the deprecatedstrings.HasPrefixapproach for path comparison can be bypassed in edge cases.🔎 Proposed secure implementation
for _, f := range r.File { + // Validate entry name before joining - security check + if filepath.IsAbs(f.Name) { + return fmt.Errorf("archive contains absolute path: %s", f.Name) + } + if strings.Contains(f.Name, "..") { + return fmt.Errorf("archive contains path traversal: %s", f.Name) + } + // Construct the full path fpath := filepath.Join(dest, f.Name) - // Check for ZipSlip vulnerability - if !strings.HasPrefix(fpath, filepath.Clean(dest)+string(os.PathSeparator)) { - return fmt.Errorf("illegal file path: %s", fpath) + // Additional boundary check using filepath.Rel + relPath, err := filepath.Rel(dest, fpath) + if err != nil || strings.HasPrefix(relPath, "..") { + return fmt.Errorf("illegal file path: %s", f.Name) }
🧹 Nitpick comments (7)
cli/src/scripts/run-tests-with-summary.sh (1)
20-24: Consider adding a fallback for empty results.The grep/awk pipeline correctly parses Go test output, and
|| trueappropriately prevents failures when there are no matches. However, if no tests are found or the log is empty, the summary will be blank, which might be confusing.🔎 Optional enhancement to handle empty results
grep -E "^=== RUN|^--- PASS|^--- FAIL" tests/logs/test-results.log | \ awk 'BEGIN { test="" } \ /^=== RUN/ { test=$3 } \ /^--- PASS/ { printf "%-40s | ✓ PASS\n", test } \ - /^--- FAIL/ { printf "%-40s | ✗ FAIL\n", test }' || true + /^--- FAIL/ { printf "%-40s | ✗ FAIL\n", test }' || echo "No test results found"cli/src/tests/cmd/gateway_mcp_generate_test.go (3)
69-69: Consider pinning the Docker image version for test stability.The test uses
mcp/everything:latest, which can lead to non-deterministic test behavior if the image is updated upstream. For more reliable and reproducible tests, consider pinning to a specific version tag.🔎 Suggested improvement
- imageName := "mcp/everything:latest" + imageName := "mcp/everything:v1.0.0" // Pin to a specific stable versionOr make it configurable via environment variable with a pinned default:
- imageName := "mcp/everything:latest" + imageName := os.Getenv("MCP_EVERYTHING_IMAGE") + if imageName == "" { + imageName = "mcp/everything:v1.0.0" // Default to pinned version + }
101-103: Clarify the actual timeout behavior.The comment states "30 seconds" but with a 2-second HTTP client timeout and 1-second sleep per iteration, the actual total wait time could be up to 90 seconds (30 iterations × 3 seconds). Consider updating the comment to reflect the actual behavior or adjusting the loop count if 30 seconds is the intended maximum wait.
🔎 Suggested improvement
- // Wait for container to be ready using HTTP health check - t.Log("Waiting for MCP server to be ready (HTTP health check)...") + // Wait for container to be ready using HTTP health check (up to ~90 seconds) + t.Log("Waiting for MCP server to be ready (HTTP health check, up to 30 attempts)...") client := &http.Client{Timeout: 2 * time.Second}Or reduce the HTTP client timeout:
- client := &http.Client{Timeout: 2 * time.Second} + client := &http.Client{Timeout: 500 * time.Millisecond}
137-137: Consider logging errors from output directory cleanup.The
os.RemoveAllcall may fail due to permission issues or locked files, which could interfere with the test. While this is pre-test cleanup, logging failures would aid debugging.🔎 Suggested improvement
- os.RemoveAll(outputDir) // Clean up any previous test output + if err := os.RemoveAll(outputDir); err != nil { + t.Logf("Warning: failed to clean up previous output directory: %v", err) + } defer os.RemoveAll(outputDir)cli/src/internal/policy/hub.go (2)
358-368: Usestrings.Joinfrom the standard library.This custom
joinfunction duplicatesstrings.Joinfrom the standard library. Replace with the built-in function for clarity and consistency.🔎 Proposed fix
-// join is a helper function to join strings with a separator -func join(items []string, sep string) string { - result := "" - for i, item := range items { - if i > 0 { - result += sep - } - result += item - } - return result -}Then update the call sites (lines 79 and 156) to use
strings.Join:strings.Join(requestedPolicies, "\n")
126-139: Consider adding request context for cancellation support.The HTTP request is created without a context, which means it cannot be cancelled if the user interrupts the CLI. Consider using
http.NewRequestWithContextwithcontext.Background()or a passed-in context for better CLI UX.🔎 Example improvement
- req, err := http.NewRequest("POST", url, bytes.NewBuffer(requestBody)) + req, err := http.NewRequestWithContext(context.Background(), "POST", url, bytes.NewBuffer(requestBody))Add
"context"to the imports.cli/src/utils/policy_utils.go (1)
35-35: Remove unusedversionDirRegexvariable at line 35.The regex variable is declared but never used in this file or elsewhere in the codebase. Remove it:
-var versionDirRegex = regexp.MustCompile(`^v?\d+\.\d+\.\d+$`)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cli/src/Makefile(2 hunks)cli/src/internal/policy/hub.go(1 hunks)cli/src/internal/policy/local.go(1 hunks)cli/src/internal/policyhub/client.go(1 hunks)cli/src/scripts/run-tests-with-summary.sh(1 hunks)cli/src/tests/cmd/gateway_mcp_generate_test.go(1 hunks)cli/src/tests/cmd/test_helpers.go(1 hunks)cli/src/utils/constants.go(1 hunks)cli/src/utils/policy_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/src/tests/cmd/test_helpers.go
- cli/src/internal/policy/local.go
- cli/src/internal/policyhub/client.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-12T08:52:52.909Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 142
File: platform-api/src/resources/openapi.yaml:905-969
Timestamp: 2025-11-12T08:52:52.909Z
Learning: In the wso2/api-platform repository, the team follows an API-first development approach where OpenAPI specs may document planned features before backend implementation is complete, allowing frontend development to proceed against the intended API contract without requiring changes later.
Applied to files:
cli/src/tests/cmd/gateway_mcp_generate_test.gocli/src/utils/constants.gocli/src/utils/policy_utils.go
📚 Learning: 2025-12-03T08:19:08.315Z
Learnt from: RakhithaRR
Repo: wso2/api-platform PR: 275
File: cli/src/cmd/version.go:32-39
Timestamp: 2025-12-03T08:19:08.315Z
Learning: In the cli/src project (fusionctl CLI), the Version variable in cli/src/cmd/version.go is set via linker flags at build time from the Makefile without a "v" prefix (e.g., "0.0.1"), so the hardcoded "v" prefix in the version output format string is intentional and correct.
Applied to files:
cli/src/utils/policy_utils.go
🧬 Code graph analysis (1)
cli/src/utils/policy_utils.go (2)
cli/src/utils/constants.go (2)
TempPath(28-28)PoliciesCachePath(27-27)cli/src/utils/policy_cache.go (2)
PolicyIndex(35-38)GenerateUniqueCachePath(167-201)
🪛 checkmake (0.2.2)
cli/src/Makefile
[warning] 47-47: Target body for "build" exceeds allowed length of 5 (7).
(maxbodylength)
🔇 Additional comments (18)
cli/src/scripts/run-tests-with-summary.sh (3)
1-5: LGTM! Good error handling setup.The use of
pipefailensures the pipeline fails if any command in line 12 fails, while allowing the script to continue and generate a summary. The absence ofset -eis appropriate here since you want to produce a summary even when tests fail.
7-12: LGTM! Test execution is correct.The
teecommand properly captures both stdout and stderr (2>&1) while displaying output in real-time.
26-34: LGTM! Proper exit code handling.The failure detection correctly uses grep with null redirection to handle missing log files, and exits with appropriate codes (1 for failure, 0 for success).
cli/src/Makefile (4)
19-22: LGTM! Binary rename is straightforward.The rename from
apipctltoapis clearly intentional and aligns with the PR objectives.
56-61: LGTM! Useful addition for development workflow.The
build-skip-teststarget provides a fast-path for developers who want to iterate quickly without running tests on every build.
63-65: LGTM! Consistent test-first behavior.Explicitly running tests before multi-platform builds ensures consistency with the single-platform
buildtarget.
67-69: LGTM! Provides flexibility for CI/CD workflows.The
build-all-skip-teststarget is useful for scenarios where tests are run separately or have already passed in a previous step.cli/src/utils/constants.go (3)
21-29: Well-organized configuration constants.The WSO2AP configuration paths are cleanly structured and follow a consistent naming pattern under
.wso2ap/.
56-60: Good safety limits for ZIP extraction.The zip safety limits are reasonable defaults that protect against zip bomb attacks: 1000 files max, 20MB per file, and 100MB total uncompressed size.
62-67: Dev URL is now configurable via environment variable.Per past review discussion, the
PolicyHubBaseURLDefaultcan be overridden via theWSO2AP_POLICYHUB_BASE_URLenvironment variable. This is acceptable, though consider updating the default to a production URL before release, or at minimum document this requirement clearly.cli/src/internal/policy/hub.go (3)
48-54: Good: Reused shared Checksum type.The
PolicyHubDatastruct now usespolicyhub.Checksumfrom the shared package, addressing the previous review comment about code duplication.
311-356: Potential double-read of response body on content-type validation failure.Lines 329-336 read the response body to include in the error message when content-type validation fails. However, the body stream is still open, and later code at line 351 would attempt to copy the same body. This is correct since the early return prevents reaching line 351, but the logic flow could be clearer.
The implementation is functionally correct as-is.
56-94: Clear orchestration logic with good error messaging.The
ProcessHubPoliciesfunction has well-structured error handling with actionable error messages that include context about what policies were requested and possible failure reasons.cli/src/utils/policy_utils.go (5)
45-128: Thorough ZIP validation with proper cleanup.
ValidateLocalPolicyZipproperly usesdefer os.RemoveAllfor cleanup and handles the common case where a zip contains a single top-level directory. The version normalization at lines 122-125 correctly ensures a consistentvprefix.
156-224: ZipDirectory implementation looks correct.The function properly handles relative paths, directories, and uses
zip.Deflatefor compression. The deferredClose()calls are in the correct order.
466-492: CopyDir does not handle symbolic links.If a directory contains symbolic links, they will be followed and the target content copied rather than preserving the symlink. For untrusted input, this could be a security concern (symlink following attacks). For controlled input, this is typically fine.
Is this function expected to handle untrusted input that might contain symlinks? If so, consider adding symlink detection:
if entry.Type()&os.ModeSymlink != 0 { // Handle or skip symlinks continue }
226-243: ToKebabCase and FormatPolicyFileName are clean implementations.Simple, focused functions that do one thing well.
376-422: SetupTempGatewayImageBuildDir has proper cleanup and setup logic.The function correctly removes existing directories before creating fresh ones, preventing stale state issues. The lock file copy is straightforward.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cli/src/Makefile (1)
63-69: Inconsistent test execution betweenbuildandbuild-all.The
buildtarget usesscripts/run-tests-with-summary.shwhilebuild-alluses thetesttarget directly. This may lead to different test output or behavior:
make build→ runsscripts/run-tests-with-summary.sh(with summary/logging)make build-all→ runs$(GOTEST) -v ./tests/...via thetesttargetConsider aligning both targets to use the same test approach for consistency.
🔎 Option: Use the test script in build-all
## build-all: Build binaries for all platforms (macOS, Linux, Windows) - runs tests first -build-all: test build-macos build-linux build-windows +build-all: build-macos build-linux build-windows + @bash scripts/run-tests-with-summary.sh @echo "All platform builds complete"Or alternatively, run tests first separately:
## build-all: Build binaries for all platforms (macOS, Linux, Windows) - runs tests first -build-all: test build-macos build-linux build-windows +build-all: + @bash scripts/run-tests-with-summary.sh + @$(MAKE) build-macos build-linux build-windows @echo "All platform builds complete"cli/src/tests/cmd/gateway_mcp_generate_test.go (1)
134-135: Consider removing the post-health-check sleep if the endpoint is reliable.The 2-second sleep after a successful health check may be unnecessary if the
/mcpendpoint accurately signals full server readiness. If the health check truly validates that the server can handle generate commands, this sleep just slows down test execution.However, if you've observed that the MCP server needs this extra initialization time despite returning HTTP 200, keep it and consider adding a comment explaining why (e.g., "MCP server health endpoint returns 200 before command handlers are ready").
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/src/Makefile(2 hunks)cli/src/tests/cmd/gateway_mcp_generate_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T08:52:52.909Z
Learnt from: thivindu
Repo: wso2/api-platform PR: 142
File: platform-api/src/resources/openapi.yaml:905-969
Timestamp: 2025-11-12T08:52:52.909Z
Learning: In the wso2/api-platform repository, the team follows an API-first development approach where OpenAPI specs may document planned features before backend implementation is complete, allowing frontend development to proceed against the intended API contract without requiring changes later.
Applied to files:
cli/src/tests/cmd/gateway_mcp_generate_test.go
🧬 Code graph analysis (1)
cli/src/tests/cmd/gateway_mcp_generate_test.go (2)
cli/src/utils/docker_utils.go (1)
IsDockerAvailable(28-34)sdk/gateway/policy/v1alpha/context.go (1)
Body(4-16)
🪛 checkmake (0.2.2)
cli/src/Makefile
[warning] 48-48: Target body for "build" exceeds allowed length of 5 (6).
(maxbodylength)
🔇 Additional comments (7)
cli/src/Makefile (3)
19-22: LGTM!The binary rename from
apipctltoapis clean and consistently updated in both the comment and the variable.
47-54: LGTM!The integration of
scripts/run-tests-with-summary.shfor test execution with summary output before building is a reasonable enhancement. The static analysis warning about body length (6 lines) is a minor style preference and acceptable here.
56-61: LGTM!The
build-skip-teststarget provides a useful development workflow option when tests have already been verified, with a clear message indicating tests are being skipped.cli/src/tests/cmd/gateway_mcp_generate_test.go (4)
33-64: Excellent test setup with robust environment checks.The test properly handles various environment conditions:
- Gracefully skips when disabled or Docker unavailable
- Automatically builds the binary if missing, reducing CI setup requirements
- Includes clear error messages at each step
This defensive approach makes the test reliable across different environments.
66-98: Container lifecycle properly managed.The Docker container handling demonstrates good practices:
- Proactive cleanup of existing containers prevents name conflicts
- Deferred cleanup ensures resources are released even on test failure
- Clear logging aids debugging
100-132: Robust health check with proper error handling.The health check implementation correctly addresses previous review feedback:
- Strict 200-299 status code validation prevents false positives from client errors
- File operation errors are properly logged
- Timeout handling includes diagnostic log dump
This makes the test reliable and debuggable.
137-185: Command execution and output validation are well-implemented.The test properly:
- Isolates output with cleanup before and after execution
- Captures and logs command output for debugging
- Validates that the generate command produces expected artifacts
- Lists generated files for inspection
The validation is appropriate for an integration test, confirming the command succeeds and produces output without overly constraining the specific content.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cli/src/tests/test-config.yaml (1)
1-14: LGTM! Clean and straightforward test configuration.The YAML structure is valid, the comments are helpful, and the test definitions are clear and consistent. Having the Docker-dependent test disabled by default is sensible for environments where Docker may not be available.
Optional: Consider adding schema validation
For future maintainability, you could add a JSON schema or a validation mechanism to ensure test configurations conform to the expected structure. This would help catch configuration errors early, especially as more tests are added.
Example validation points:
nameanddescriptionare required stringsenabledis a required booleandependenciesis an optional array of stringsThis is a nice-to-have improvement and not critical for the current implementation.
2 participants
Purpose
$Subject
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.