From bee862e91fcf737a590678c45cf48ff1b9a0dc24 Mon Sep 17 00:00:00 2001 From: ikjeong Date: Tue, 18 Nov 2025 13:41:01 +0000 Subject: [PATCH 1/6] refactor: migrate integration tests to ValidateChanges - Add createGitChangeFromFile helper using git diff - Update 18 integration tests to use ValidateChanges instead of Validate - Remove legacy Validate(path string) method and Result type - Update test_validator.go to use ValidateChanges - Unify validation through ValidateChanges for consistency with CLI --- internal/validator/validator.go | 217 --------------------- test_validator.go | 32 ++- tests/integration/helper.go | 58 ++++-- tests/integration/validator_policy_test.go | 97 +++++++-- 4 files changed, 151 insertions(+), 253 deletions(-) diff --git a/internal/validator/validator.go b/internal/validator/validator.go index 72a9885..2d43ba8 100644 --- a/internal/validator/validator.go +++ b/internal/validator/validator.go @@ -65,186 +65,6 @@ type Violation struct { Column int } -// Result represents validation result -type Result struct { - Violations []Violation - Passed bool -} - -// Validate validates the given path -func (v *Validator) Validate(path string) (*Result, error) { - if v.policy == nil { - return nil, fmt.Errorf("policy is not loaded") - } - - result := &Result{ - Violations: make([]Violation, 0), - Passed: true, - } - - if v.verbose { - fmt.Printf("šŸ” Validating %s against %d rule(s)...\n", path, len(v.policy.Rules)) - } - - // Check RBAC permissions if enabled - if v.policy.Enforce.RBACConfig != nil && v.policy.Enforce.RBACConfig.Enabled { - // Get current git user - username, err := git.GetCurrentUser() - if err != nil { - if v.verbose { - fmt.Printf("āš ļø RBAC check skipped: %v\n", err) - } - } else { - if v.verbose { - fmt.Printf("šŸ” Checking RBAC permissions for user: %s\n", username) - } - - // Get files to check - fileInfo, err := os.Stat(path) - if err == nil && !fileInfo.IsDir() { - // Validate file permissions - rbacResult, err := roles.ValidateFilePermissions(username, []string{path}) - if err != nil { - if v.verbose { - fmt.Printf("āš ļø RBAC validation failed: %v\n", err) - } - } else if !rbacResult.Allowed { - // Add RBAC violations - for _, deniedFile := range rbacResult.DeniedFiles { - result.Violations = append(result.Violations, Violation{ - RuleID: "rbac-permission-denied", - Severity: "error", - Message: fmt.Sprintf("User '%s' does not have permission to modify this file", username), - File: deniedFile, - Line: 0, - Column: 0, - }) - } - result.Passed = false - - if v.verbose { - fmt.Printf("āŒ RBAC: %d file(s) denied for user %s\n", len(rbacResult.DeniedFiles), username) - } - } else if v.verbose { - fmt.Printf("āœ“ RBAC: User %s has permission to modify all files\n", username) - } - } - } - } - - // For each enabled rule, execute validation - for _, rule := range v.policy.Rules { - if !rule.Enabled { - continue - } - - // Determine which engine to use - engineName := getEngineName(rule) - if engineName == "" { - if v.verbose { - fmt.Printf("āš ļø Rule %s has no engine specified, skipping\n", rule.ID) - } - continue - } - - // Get files that match this rule's selector - files, err := v.selectFilesForRule(path, &rule) - if err != nil { - fmt.Printf("āš ļø Failed to select files for rule %s: %v\n", rule.ID, err) - continue - } - - if len(files) == 0 { - if v.verbose { - fmt.Printf(" Rule %s: no matching files\n", rule.ID) - } - continue - } - - // Get or create engine - engine, err := v.registry.Get(engineName) - if err != nil { - fmt.Printf("āš ļø Engine %s not found for rule %s: %v\n", engineName, rule.ID, err) - continue - } - - // Initialize engine if needed - if err := v.initializeEngine(engine); err != nil { - fmt.Printf("āš ļø Failed to initialize engine %s: %v\n", engineName, err) - continue - } - - // Convert schema.PolicyRule to core.Rule - coreRule := convertToCoreRule(rule) - - // Execute validation - if v.verbose { - fmt.Printf(" Rule %s (%s): checking %d file(s)...\n", rule.ID, engineName, len(files)) - } - - validationResult, err := engine.Validate(v.ctx, coreRule, files) - if err != nil { - fmt.Printf("āš ļø Validation failed for rule %s: %v\n", rule.ID, err) - continue - } - - // Collect violations - if validationResult != nil && len(validationResult.Violations) > 0 { - for _, coreViolation := range validationResult.Violations { - violation := Violation{ - RuleID: rule.ID, - Severity: rule.Severity, - Message: coreViolation.Message, - File: coreViolation.File, - Line: coreViolation.Line, - Column: coreViolation.Column, - } - - // Use custom message if provided - if rule.Message != "" { - violation.Message = rule.Message - } - - result.Violations = append(result.Violations, violation) - } - - if v.verbose { - fmt.Printf(" āŒ Found %d violation(s)\n", len(validationResult.Violations)) - } - } else if v.verbose { - fmt.Printf(" āœ“ Passed\n") - } - } - - // Determine overall pass/fail - result.Passed = len(result.Violations) == 0 - - if v.verbose { - if result.Passed { - fmt.Printf("\nāœ… Validation passed: no violations found\n") - } else { - fmt.Printf("\nāŒ Validation failed: %d violation(s) found\n", len(result.Violations)) - } - } - - return result, nil -} - -// CanAutoFix checks if violations can be auto-fixed -func (v *Result) CanAutoFix() bool { - for _, violation := range v.Violations { - // Check if rule has autofix enabled - _ = violation - } - return false -} - -// AutoFix attempts to automatically fix violations -func (v *Validator) AutoFix(result *Result) error { - // TODO: Implement auto-fix logic - return fmt.Errorf("auto-fix not implemented yet") -} - // Helper functions // getEngineName extracts the engine name from a rule @@ -255,43 +75,6 @@ func getEngineName(rule schema.PolicyRule) string { return "" } -// selectFilesForRule selects files that match the rule's selector -func (v *Validator) selectFilesForRule(basePath string, rule *schema.PolicyRule) ([]string, error) { - // If path is a specific file, check if it matches the selector - fileInfo, err := os.Stat(basePath) - if err != nil { - return nil, err - } - - if !fileInfo.IsDir() { - // Single file - check if it matches selector - if rule.When != nil { - lang := GetLanguageFromFile(basePath) - if len(rule.When.Languages) > 0 { - matched := false - for _, ruleLang := range rule.When.Languages { - if ruleLang == lang { - matched = true - break - } - } - if !matched { - return []string{}, nil - } - } - } - return []string{basePath}, nil - } - - // Directory - use selector to find files - if rule.When == nil { - // No selector, use all files in directory - return v.selector.SelectFiles(nil) - } - - return v.selector.SelectFiles(rule.When) -} - // initializeEngine initializes an engine if not already initialized func (v *Validator) initializeEngine(engine core.Engine) error { // Create engine config diff --git a/test_validator.go b/test_validator.go index 8f2ea97..cc612fe 100644 --- a/test_validator.go +++ b/test_validator.go @@ -1,8 +1,10 @@ package main import ( + "context" "fmt" "os" + "os/exec" "github.com/DevSymphony/sym-cli/internal/validator" "github.com/DevSymphony/sym-cli/pkg/schema" @@ -79,16 +81,38 @@ func main() { fmt.Printf("Testing: %s\n", file) fmt.Printf("ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•\n\n") - result, err := v.Validate(file) + // Create GitChange from file using git diff + cmd := exec.Command("git", "diff", "--no-index", "/dev/null", file) + diffOutput, err := cmd.CombinedOutput() + // git diff --no-index returns exit code 1 when there are differences (expected) + if err != nil { + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + // Expected - continue + } else { + fmt.Printf("āŒ Failed to generate diff: %v\n\n", err) + continue + } + } + + changes := []validator.GitChange{{ + FilePath: file, + Status: "A", + Diff: string(diffOutput), + }} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) if err != nil { fmt.Printf("āŒ Validation error: %v\n\n", err) continue } - if result.Passed { - fmt.Printf("\nāœ… PASSED: No violations\n\n") + if len(result.Violations) == 0 { + fmt.Printf("\nāœ… PASSED: No violations\n") + fmt.Printf(" Checked: %d, Passed: %d, Failed: %d\n\n", result.Checked, result.Passed, result.Failed) } else { - fmt.Printf("\nāŒ FAILED: %d violation(s) found:\n", len(result.Violations)) + fmt.Printf("\nāŒ FAILED: %d violation(s) found\n", len(result.Violations)) + fmt.Printf(" Checked: %d, Passed: %d, Failed: %d\n", result.Checked, result.Passed, result.Failed) for i, violation := range result.Violations { fmt.Printf("\n%d. [%s] %s\n", i+1, violation.Severity, violation.RuleID) fmt.Printf(" File: %s:%d:%d\n", violation.File, violation.Line, violation.Column) diff --git a/tests/integration/helper.go b/tests/integration/helper.go index f5b02cd..3ae23d3 100644 --- a/tests/integration/helper.go +++ b/tests/integration/helper.go @@ -2,6 +2,7 @@ package integration import ( "os" + "os/exec" "path/filepath" "testing" @@ -63,15 +64,44 @@ func createTestValidator(t *testing.T, pol *schema.CodePolicy) *validator.Valida return v } +// createGitChangeFromFile creates a GitChange from a test file using git diff +func createGitChangeFromFile(t *testing.T, filePath string) validator.GitChange { + t.Helper() + + // Use git diff --no-index to generate a unified diff + // This treats the file as newly added (comparing /dev/null to the file) + cmd := exec.Command("git", "diff", "--no-index", "/dev/null", filePath) + output, err := cmd.CombinedOutput() + + // git diff --no-index returns exit code 1 when there are differences (which is expected) + // We only care about the output, not the exit code + if err != nil { + // Check if it's just the expected exit code 1 + if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + // This is expected - there are differences between /dev/null and the file + } else { + // This is an actual error + t.Fatalf("Failed to generate diff for %s: %v", filePath, err) + } + } + + return validator.GitChange{ + FilePath: filePath, + Status: "A", // Treat as Added file + Diff: string(output), + } +} + // assertViolationsDetected asserts that violations are found and logs them -func assertViolationsDetected(t *testing.T, result *validator.Result) { +func assertViolationsDetected(t *testing.T, result *validator.ValidationResult) { t.Helper() - assert.False(t, result.Passed, "Should detect violations") assert.Greater(t, len(result.Violations), 0, "Should have violations") + assert.Greater(t, result.Failed, 0, "Should have failed checks") - // Log violations for debugging + // Log violations and metrics for debugging if len(result.Violations) > 0 { - t.Logf("Found %d violation(s):", len(result.Violations)) + t.Logf("Found %d violation(s): Checked=%d, Passed=%d, Failed=%d", + len(result.Violations), result.Checked, result.Passed, result.Failed) for i, v := range result.Violations { t.Logf(" %d. [%s] %s at %s:%d:%d (severity: %s)", i+1, v.RuleID, v.Message, v.File, v.Line, v.Column, v.Severity) @@ -80,18 +110,20 @@ func assertViolationsDetected(t *testing.T, result *validator.Result) { } // assertNoPolicyViolations asserts that no violations are found -func assertNoPolicyViolations(t *testing.T, result *validator.Result) { +func assertNoPolicyViolations(t *testing.T, result *validator.ValidationResult) { t.Helper() - if !result.Passed || len(result.Violations) > 0 { + if len(result.Violations) > 0 { // Log violations if any for debugging - if len(result.Violations) > 0 { - t.Logf("Unexpected violations found:") - for i, v := range result.Violations { - t.Logf(" %d. [%s] %s at %s:%d:%d", - i+1, v.RuleID, v.Message, v.File, v.Line, v.Column) - } + t.Logf("Unexpected violations found: Checked=%d, Passed=%d, Failed=%d", + result.Checked, result.Passed, result.Failed) + for i, v := range result.Violations { + t.Logf(" %d. [%s] %s at %s:%d:%d", + i+1, v.RuleID, v.Message, v.File, v.Line, v.Column) } } - assert.True(t, result.Passed, "Should pass validation") assert.Equal(t, 0, len(result.Violations), "Should have no violations") + assert.Equal(t, 0, result.Failed, "Should have no failed checks") + if result.Checked > 0 { + assert.Equal(t, result.Checked, result.Passed, "All checks should pass") + } } diff --git a/tests/integration/validator_policy_test.go b/tests/integration/validator_policy_test.go index a36a576..53e44f9 100644 --- a/tests/integration/validator_policy_test.go +++ b/tests/integration/validator_policy_test.go @@ -1,9 +1,11 @@ package integration import ( + "context" "path/filepath" "testing" + "github.com/DevSymphony/sym-cli/internal/validator" "github.com/stretchr/testify/require" ) @@ -26,7 +28,10 @@ func TestValidator_JavaScript_Pattern_Violations(t *testing.T) { // Test naming violations t.Run("NamingViolations", func(t *testing.T) { filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/pattern/naming-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) }) @@ -34,7 +39,10 @@ func TestValidator_JavaScript_Pattern_Violations(t *testing.T) { // Test security violations t.Run("SecurityViolations", func(t *testing.T) { filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/pattern/security-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) }) @@ -49,7 +57,10 @@ func TestValidator_JavaScript_Pattern_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/pattern/valid.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -68,7 +79,10 @@ func TestValidator_JavaScript_Length_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/length/length-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -82,7 +96,10 @@ func TestValidator_JavaScript_Length_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/length/valid.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -101,7 +118,10 @@ func TestValidator_JavaScript_Style_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/style/style-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -115,7 +135,10 @@ func TestValidator_JavaScript_Style_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/style/valid.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -134,7 +157,10 @@ func TestValidator_JavaScript_AST_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/ast/naming-violations.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -148,7 +174,10 @@ func TestValidator_JavaScript_AST_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/javascript/ast/valid.js") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -167,7 +196,10 @@ func TestValidator_TypeScript_TypeChecker_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/typescript/typechecker/type-errors.ts") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -181,7 +213,10 @@ func TestValidator_TypeScript_TypeChecker_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/typescript/typechecker/valid.ts") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -200,7 +235,10 @@ func TestValidator_Java_Pattern_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/pattern/NamingViolations.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -214,7 +252,10 @@ func TestValidator_Java_Pattern_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/pattern/ValidNaming.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -233,7 +274,10 @@ func TestValidator_Java_Length_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/length/LengthViolations.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -247,7 +291,10 @@ func TestValidator_Java_Length_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/length/ValidLength.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -266,7 +313,10 @@ func TestValidator_Java_Style_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/style/StyleViolations.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -280,7 +330,10 @@ func TestValidator_Java_Style_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/style/ValidStyle.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } @@ -299,7 +352,10 @@ func TestValidator_Java_AST_Violations(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/ast/AstViolations.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertViolationsDetected(t, result) } @@ -313,7 +369,10 @@ func TestValidator_Java_AST_Valid(t *testing.T) { v := createTestValidator(t, policy) filePath := filepath.Join(getTestdataDir(t), "testdata/java/ast/ValidAst.java") - result, err := v.Validate(filePath) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) require.NoError(t, err) assertNoPolicyViolations(t, result) } From 0ed5faba89dc9442119072f3a2e01fc2b4051da0 Mon Sep 17 00:00:00 2001 From: ikjeong Date: Tue, 18 Nov 2025 14:03:05 +0000 Subject: [PATCH 2/6] test: Add comprehensive RBAC integration tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add environment variable support to git.GetCurrentUser() - Checks GIT_AUTHOR_NAME env var first (for testing) - Falls back to git config user.name - Add RBAC-specific test helpers - setupRBACEnvironment() - Sets git user via env var - createRBACTestValidator() - Creates validator with RBAC config - assertRBACViolation() - Verifies RBAC violations occurred - assertNoRBACViolation() - Verifies no RBAC violations - Update createGitChangeFromFile() to use relative paths - Converts absolute paths to relative from repo root - Matches behavior of real GetGitChanges() function - Add 10 RBAC integration test cases - Admin full access - Developer allowed/denied file permissions - Viewer denied all access - Mixed permissions in single commit - Deleted files skipped - RBAC disabled mode - Unknown user handling - Glob pattern matching - Add RBAC testdata structure - Complete test directory with sample files - Policy configuration with roles and permissions - Test files at various access levels All tests passing āœ“ --- internal/git/repo.go | 9 + testdata/rbac/code-policy.json | 71 ++++++ testdata/rbac/config/settings.json | 15 ++ testdata/rbac/src/api/client.js | 21 ++ testdata/rbac/src/components/Button.js | 12 + testdata/rbac/src/core/engine.js | 19 ++ testdata/rbac/tests/test.js | 10 + tests/integration/helper.go | 82 ++++++- tests/integration/validator_rbac_test.go | 282 +++++++++++++++++++++++ 9 files changed, 520 insertions(+), 1 deletion(-) create mode 100644 testdata/rbac/code-policy.json create mode 100644 testdata/rbac/config/settings.json create mode 100644 testdata/rbac/src/api/client.js create mode 100644 testdata/rbac/src/components/Button.js create mode 100644 testdata/rbac/src/core/engine.js create mode 100644 testdata/rbac/tests/test.js create mode 100644 tests/integration/validator_rbac_test.go diff --git a/internal/git/repo.go b/internal/git/repo.go index ae458b2..a916773 100644 --- a/internal/git/repo.go +++ b/internal/git/repo.go @@ -2,6 +2,7 @@ package git import ( "fmt" + "os" "os/exec" "regexp" "strings" @@ -56,7 +57,15 @@ func IsGitRepo() bool { } // GetCurrentUser returns the current git user name +// Checks GIT_AUTHOR_NAME environment variable first (for testing), +// then falls back to git config user.name func GetCurrentUser() (string, error) { + // Check environment variable first (for testing) + if user := os.Getenv("GIT_AUTHOR_NAME"); user != "" { + return user, nil + } + + // Fallback to git config cmd := exec.Command("git", "config", "--get", "user.name") output, err := cmd.Output() if err != nil { diff --git a/testdata/rbac/code-policy.json b/testdata/rbac/code-policy.json new file mode 100644 index 0000000..627da68 --- /dev/null +++ b/testdata/rbac/code-policy.json @@ -0,0 +1,71 @@ +{ + "version": "1.0.0", + "rbac": { + "roles": { + "admin": { + "permissions": [ + { + "path": "**/*", + "read": true, + "write": true, + "execute": false + } + ] + }, + "developer": { + "permissions": [ + { + "path": "src/**", + "read": true, + "write": true, + "execute": false + }, + { + "path": "tests/**", + "read": true, + "write": true, + "execute": false + }, + { + "path": "src/core/**", + "read": true, + "write": false, + "execute": false + }, + { + "path": "src/api/**", + "read": true, + "write": false, + "execute": false + } + ] + }, + "viewer": { + "permissions": [ + { + "path": "**/*", + "read": true, + "write": false, + "execute": false + } + ] + } + } + }, + "rules": [], + "enforce": { + "stages": [ + "pre-commit" + ], + "fail_on": [ + "error" + ], + "rbac": { + "enabled": true, + "stages": [ + "pre-commit" + ], + "on_violation": "block" + } + } +} diff --git a/testdata/rbac/config/settings.json b/testdata/rbac/config/settings.json new file mode 100644 index 0000000..3086182 --- /dev/null +++ b/testdata/rbac/config/settings.json @@ -0,0 +1,15 @@ +{ + "app": { + "name": "Test Application", + "version": "1.0.0", + "environment": "development" + }, + "api": { + "baseURL": "https://api.example.com", + "timeout": 30000 + }, + "features": { + "authentication": true, + "logging": true + } +} diff --git a/testdata/rbac/src/api/client.js b/testdata/rbac/src/api/client.js new file mode 100644 index 0000000..38cd34b --- /dev/null +++ b/testdata/rbac/src/api/client.js @@ -0,0 +1,21 @@ +// API client module - Developer CANNOT modify this file +class APIClient { + constructor(baseURL) { + this.baseURL = baseURL; + } + + async get(endpoint) { + const response = await fetch(`${this.baseURL}${endpoint}`); + return response.json(); + } + + async post(endpoint, data) { + const response = await fetch(`${this.baseURL}${endpoint}`, { + method: 'POST', + body: JSON.stringify(data), + }); + return response.json(); + } +} + +module.exports = APIClient; diff --git a/testdata/rbac/src/components/Button.js b/testdata/rbac/src/components/Button.js new file mode 100644 index 0000000..2c2ac2c --- /dev/null +++ b/testdata/rbac/src/components/Button.js @@ -0,0 +1,12 @@ +// Button component - Developer can modify this file +class Button { + constructor(label) { + this.label = label; + } + + render() { + return ``; + } +} + +module.exports = Button; diff --git a/testdata/rbac/src/core/engine.js b/testdata/rbac/src/core/engine.js new file mode 100644 index 0000000..c54e77a --- /dev/null +++ b/testdata/rbac/src/core/engine.js @@ -0,0 +1,19 @@ +// Core engine module - Developer CANNOT modify this file +class Engine { + constructor(config) { + this.config = config; + this.running = false; + } + + start() { + this.running = true; + console.log("Engine started"); + } + + stop() { + this.running = false; + console.log("Engine stopped"); + } +} + +module.exports = Engine; diff --git a/testdata/rbac/tests/test.js b/testdata/rbac/tests/test.js new file mode 100644 index 0000000..4953ace --- /dev/null +++ b/testdata/rbac/tests/test.js @@ -0,0 +1,10 @@ +// Test file - Developer can modify this file +const Button = require('../src/components/Button'); + +describe('Button', () => { + it('should render button with label', () => { + const button = new Button('Click me'); + const html = button.render(); + expect(html).toBe(''); + }); +}); diff --git a/tests/integration/helper.go b/tests/integration/helper.go index 3ae23d3..a1fc100 100644 --- a/tests/integration/helper.go +++ b/tests/integration/helper.go @@ -85,8 +85,25 @@ func createGitChangeFromFile(t *testing.T, filePath string) validator.GitChange } } + // Convert absolute path to relative path from repo root + // FilePath should be relative like "testdata/rbac/src/components/Button.js" + // not absolute like "/workspace/testdata/rbac/src/components/Button.js" + relativePath := filePath + if filepath.IsAbs(filePath) { + // Get repo root + cwd, _ := os.Getwd() + projectRoot := filepath.Join(cwd, "../..") + absProjectRoot, _ := filepath.Abs(projectRoot) + + // Make path relative to project root + relPath, err := filepath.Rel(absProjectRoot, filePath) + if err == nil { + relativePath = relPath + } + } + return validator.GitChange{ - FilePath: filePath, + FilePath: relativePath, Status: "A", // Treat as Added file Diff: string(output), } @@ -127,3 +144,66 @@ func assertNoPolicyViolations(t *testing.T, result *validator.ValidationResult) assert.Equal(t, result.Checked, result.Passed, "All checks should pass") } } + +// setupRBACEnvironment sets up the environment for RBAC testing +// by setting the GIT_AUTHOR_NAME environment variable +func setupRBACEnvironment(t *testing.T, username string) { + t.Helper() + t.Setenv("GIT_AUTHOR_NAME", username) + t.Logf("Set GIT_AUTHOR_NAME=%s for RBAC testing", username) +} + +// createRBACTestValidator creates a validator with RBAC testdata policy +// and sets up the git user environment +func createRBACTestValidator(t *testing.T, username string) *validator.Validator { + t.Helper() + setupRBACEnvironment(t, username) + pol := loadPolicyFromTestdata(t, "testdata/rbac/code-policy.json") + return createTestValidator(t, pol) +} + +// assertRBACViolation asserts that an RBAC violation occurred +// for the expected user +func assertRBACViolation(t *testing.T, result *validator.ValidationResult, expectedUser string) { + t.Helper() + + // Find RBAC violations (RuleID is "rbac-permission-denied") + rbacViolations := []validator.Violation{} + for _, v := range result.Violations { + if v.RuleID == "rbac-permission-denied" { + rbacViolations = append(rbacViolations, v) + } + } + + assert.Greater(t, len(rbacViolations), 0, "Should have RBAC violations") + + // Log RBAC violations for debugging + if len(rbacViolations) > 0 { + t.Logf("Found %d RBAC violation(s) for user '%s':", len(rbacViolations), expectedUser) + for i, v := range rbacViolations { + t.Logf(" %d. %s (file: %s)", i+1, v.Message, v.File) + } + } +} + +// assertNoRBACViolation asserts that no RBAC violations occurred +func assertNoRBACViolation(t *testing.T, result *validator.ValidationResult) { + t.Helper() + + // Check for RBAC violations (RuleID is "rbac-permission-denied") + rbacViolations := []validator.Violation{} + for _, v := range result.Violations { + if v.RuleID == "rbac-permission-denied" { + rbacViolations = append(rbacViolations, v) + } + } + + if len(rbacViolations) > 0 { + t.Logf("Unexpected RBAC violations found:") + for i, v := range rbacViolations { + t.Logf(" %d. %s (file: %s)", i+1, v.Message, v.File) + } + } + + assert.Equal(t, 0, len(rbacViolations), "Should have no RBAC violations") +} diff --git a/tests/integration/validator_rbac_test.go b/tests/integration/validator_rbac_test.go new file mode 100644 index 0000000..4954e70 --- /dev/null +++ b/tests/integration/validator_rbac_test.go @@ -0,0 +1,282 @@ +package integration + +import ( + "context" + "path/filepath" + "testing" + + "github.com/DevSymphony/sym-cli/internal/validator" +) + +// TestValidator_RBAC_AdminFullAccess verifies that admin role has full write access +func TestValidator_RBAC_AdminFullAccess(t *testing.T) { + v := createRBACTestValidator(t, "alice") // alice is admin + + // Admin should be able to modify any file + testFiles := []string{ + "testdata/rbac/src/components/Button.js", + "testdata/rbac/src/core/engine.js", + "testdata/rbac/src/api/client.js", + "testdata/rbac/tests/test.js", + "testdata/rbac/config/settings.json", + } + + for _, file := range testFiles { + filePath := filepath.Join(getTestdataDir(t), file) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed for %s: %v", file, err) + } + + assertNoRBACViolation(t, result) + t.Logf("āœ“ Admin successfully modified: %s", file) + } +} + +// TestValidator_RBAC_DeveloperAllowedFiles verifies that developer can modify allowed files +func TestValidator_RBAC_DeveloperAllowedFiles(t *testing.T) { + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Developer should be able to modify src/components/* and tests/* + testFiles := []string{ + "testdata/rbac/src/components/Button.js", + "testdata/rbac/tests/test.js", + } + + for _, file := range testFiles { + filePath := filepath.Join(getTestdataDir(t), file) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed for %s: %v", file, err) + } + + assertNoRBACViolation(t, result) + t.Logf("āœ“ Developer successfully modified: %s", file) + } +} + +// TestValidator_RBAC_DeveloperDeniedCoreFiles verifies that developer cannot modify core files +func TestValidator_RBAC_DeveloperDeniedCoreFiles(t *testing.T) { + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Developer should NOT be able to modify src/core/* + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/core/engine.js") + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + assertRBACViolation(t, result, "charlie") + t.Logf("āœ“ Developer correctly blocked from modifying core file") +} + +// TestValidator_RBAC_DeveloperDeniedAPIFiles verifies that developer cannot modify API files +func TestValidator_RBAC_DeveloperDeniedAPIFiles(t *testing.T) { + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Developer should NOT be able to modify src/api/* + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/api/client.js") + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + assertRBACViolation(t, result, "charlie") + t.Logf("āœ“ Developer correctly blocked from modifying API file") +} + +// TestValidator_RBAC_ViewerDenied verifies that viewer role has no write access +func TestValidator_RBAC_ViewerDenied(t *testing.T) { + v := createRBACTestValidator(t, "frank") // frank is viewer + + // Viewer should NOT be able to modify any file + testFiles := []string{ + "testdata/rbac/src/components/Button.js", + "testdata/rbac/tests/test.js", + "testdata/rbac/config/settings.json", + } + + for _, file := range testFiles { + filePath := filepath.Join(getTestdataDir(t), file) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed for %s: %v", file, err) + } + + assertRBACViolation(t, result, "frank") + t.Logf("āœ“ Viewer correctly blocked from modifying: %s", file) + } +} + +// TestValidator_RBAC_MixedPermissions verifies mixed file permissions in a single commit +func TestValidator_RBAC_MixedPermissions(t *testing.T) { + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Mix of allowed and denied files + allowedFile := filepath.Join(getTestdataDir(t), "testdata/rbac/src/components/Button.js") + deniedFile := filepath.Join(getTestdataDir(t), "testdata/rbac/src/core/engine.js") + + changes := []validator.GitChange{ + createGitChangeFromFile(t, allowedFile), + createGitChangeFromFile(t, deniedFile), + } + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + // Should have RBAC violation for the denied file + assertRBACViolation(t, result, "charlie") + t.Logf("āœ“ Mixed permissions correctly detected RBAC violation") +} + +// TestValidator_RBAC_DeletedFilesSkipped verifies that deleted files don't trigger RBAC +func TestValidator_RBAC_DeletedFilesSkipped(t *testing.T) { + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Deleted files should not trigger RBAC violations + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/core/engine.js") + + changes := []validator.GitChange{ + { + FilePath: filePath, + Status: "D", // Deleted + Diff: "--- a/src/core/engine.js\n+++ /dev/null\n", + }, + } + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + assertNoRBACViolation(t, result) + t.Logf("āœ“ Deleted files correctly skipped RBAC validation") +} + +// TestValidator_RBAC_Disabled verifies that RBAC can be disabled +func TestValidator_RBAC_Disabled(t *testing.T) { + setupRBACEnvironment(t, "charlie") // charlie is developer + + // Load a policy with RBAC disabled + pol := loadPolicyFromTestdata(t, "testdata/rbac/code-policy.json") + + // Disable RBAC for this test + if pol.Enforce.RBACConfig != nil { + pol.Enforce.RBACConfig.Enabled = false + } + + v := createTestValidator(t, pol) + + // Developer should be able to modify core files when RBAC is disabled + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/core/engine.js") + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + assertNoRBACViolation(t, result) + t.Logf("āœ“ RBAC disabled - no violations detected") +} + +// TestValidator_RBAC_UnknownUser verifies behavior with unknown users +func TestValidator_RBAC_UnknownUser(t *testing.T) { + v := createRBACTestValidator(t, "unknown_user") // User not in roles.json + + // Unknown user should have no permissions (default to viewer-like behavior) + filePath := filepath.Join(getTestdataDir(t), "testdata/rbac/src/components/Button.js") + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed: %v", err) + } + + // Unknown user should be denied (no role = no permissions) + assertRBACViolation(t, result, "unknown_user") + t.Logf("āœ“ Unknown user correctly denied access") +} + +// TestValidator_RBAC_GlobPatternMatching verifies glob pattern matching works correctly +func TestValidator_RBAC_GlobPatternMatching(t *testing.T) { + v := createRBACTestValidator(t, "charlie") // charlie is developer + + // Test various glob patterns + testCases := []struct { + file string + shouldAllow bool + description string + }{ + { + file: "testdata/rbac/src/components/Button.js", + shouldAllow: true, + description: "src/** allows src/components/*", + }, + { + file: "testdata/rbac/src/core/engine.js", + shouldAllow: false, + description: "src/core/** deny overrides src/** allow", + }, + { + file: "testdata/rbac/src/api/client.js", + shouldAllow: false, + description: "src/api/** deny overrides src/** allow", + }, + { + file: "testdata/rbac/tests/test.js", + shouldAllow: true, + description: "tests/** is allowed", + }, + } + + for _, tc := range testCases { + filePath := filepath.Join(getTestdataDir(t), tc.file) + changes := []validator.GitChange{createGitChangeFromFile(t, filePath)} + + ctx := context.Background() + result, err := v.ValidateChanges(ctx, changes) + + if err != nil { + t.Fatalf("ValidateChanges failed for %s: %v", tc.file, err) + } + + if tc.shouldAllow { + assertNoRBACViolation(t, result) + t.Logf("āœ“ Glob pattern test passed: %s (allowed)", tc.description) + } else { + assertRBACViolation(t, result, "charlie") + t.Logf("āœ“ Glob pattern test passed: %s (denied)", tc.description) + } + } +} From 47cb704454425530a9413d28f90fe66cd5d3cf17 Mon Sep 17 00:00:00 2001 From: ikjeong Date: Tue, 18 Nov 2025 14:32:41 +0000 Subject: [PATCH 3/6] fix: Resolve Java integration test file path issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Java integration tests (7/8) were failing with EOF parsing errors: - Checkstyle/PMD couldn't find test files - Relative paths were resolved incorrectly - WorkDir mismatch between test execution and validator ## Root Cause 1. Tests run from `/workspace/tests/integration` 2. Test files located at `/workspace/testdata/java/**` 3. Validator WorkDir set to test directory 4. Adapters tried `/workspace/tests/integration/testdata/java/**` (incorrect) ## Solution ### A. File Path Handling **Checkstyle Executor** (`internal/adapter/checkstyle/executor.go`) - Added file existence verification before execution - Support both absolute and relative paths - Clear error messages when files not found **PMD Executor** (`internal/adapter/pmd/executor.go`) - Same file path improvements as Checkstyle **Test Helper** (`tests/integration/helper.go`) - Changed to use absolute file paths in GitChange objects - Set Validator WorkDir to repo root during creation - Removed complex relative path conversion logic ### B. Error Handling **Checkstyle Parser** (`internal/adapter/checkstyle/parser.go`) - Improved empty output handling - Better error messages for non-zero exit codes - Prevents misleading EOF errors ### C. RBAC Compatibility **Validator** (`internal/validator/validator.go`) - Convert absolute paths to relative for RBAC checking - Ensures RBAC patterns still match correctly - Maintains backward compatibility ### D. Cleanup **go.mod** - Fixed import order (go mod tidy) ## Test Results **Java Tests**: 8/8 PASS āœ“ - Pattern violations/valid: 2/2 - Length violations/valid: 2/2 - Style violations/valid: 2/2 - AST violations/valid: 2/2 **RBAC Tests**: 10/10 PASS āœ“ (no regression) **CI Mode**: All tests pass with `-short` flag āœ“ --- go.mod | 2 +- internal/adapter/checkstyle/executor.go | 24 +++++++++++++- internal/adapter/checkstyle/parser.go | 8 +++++ internal/adapter/pmd/executor.go | 23 +++++++++++++- internal/validator/validator.go | 11 ++++++- tests/integration/helper.go | 42 ++++++++++++++++--------- 6 files changed, 91 insertions(+), 19 deletions(-) diff --git a/go.mod b/go.mod index c815c07..eaae966 100644 --- a/go.mod +++ b/go.mod @@ -9,8 +9,8 @@ require ( ) require ( - github.com/modelcontextprotocol/go-sdk v1.1.0 github.com/manifoldco/promptui v0.9.0 + github.com/modelcontextprotocol/go-sdk v1.1.0 github.com/stretchr/testify v1.11.1 ) diff --git a/internal/adapter/checkstyle/executor.go b/internal/adapter/checkstyle/executor.go index b33f3f6..8f6d2a6 100644 --- a/internal/adapter/checkstyle/executor.go +++ b/internal/adapter/checkstyle/executor.go @@ -36,7 +36,29 @@ func (a *Adapter) execute(ctx context.Context, config []byte, files []string) (* "-c", configFile, "-f", "xml", // XML output format } - args = append(args, files...) + + // Ensure all file paths are absolute + // Convert relative paths using WorkDir as base + absFiles := make([]string, len(files)) + for i, file := range files { + var absPath string + if filepath.IsAbs(file) { + // Already absolute, use as-is + absPath = file + } else { + // Relative path, resolve against WorkDir + absPath = filepath.Join(a.WorkDir, file) + } + + // Verify file exists + if _, err := os.Stat(absPath); os.IsNotExist(err) { + return nil, fmt.Errorf("file not found: %s", file) + } + + absFiles[i] = absPath + } + + args = append(args, absFiles...) // Execute start := time.Now() diff --git a/internal/adapter/checkstyle/parser.go b/internal/adapter/checkstyle/parser.go index 7e2b3f7..20e5554 100644 --- a/internal/adapter/checkstyle/parser.go +++ b/internal/adapter/checkstyle/parser.go @@ -40,6 +40,14 @@ func parseOutput(output *adapter.ToolOutput) ([]adapter.Violation, error) { return []adapter.Violation{}, nil } + // If no output but non-zero exit code, something went wrong + if output.Stdout == "" { + if output.Stderr != "" { + return nil, fmt.Errorf("checkstyle failed (exit code %d): %s", output.ExitCode, output.Stderr) + } + return nil, fmt.Errorf("checkstyle failed with exit code %d but produced no output", output.ExitCode) + } + // Parse XML output var result CheckstyleOutput if err := xml.Unmarshal([]byte(output.Stdout), &result); err != nil { diff --git a/internal/adapter/pmd/executor.go b/internal/adapter/pmd/executor.go index 25c14c3..b946aee 100644 --- a/internal/adapter/pmd/executor.go +++ b/internal/adapter/pmd/executor.go @@ -32,10 +32,31 @@ func (a *Adapter) execute(ctx context.Context, config []byte, files []string) (* // Build command pmdPath := a.getPMDPath() + // Ensure all file paths are absolute + // Convert relative paths using WorkDir as base + absFiles := make([]string, len(files)) + for i, file := range files { + var absPath string + if filepath.IsAbs(file) { + // Already absolute, use as-is + absPath = file + } else { + // Relative path, resolve against WorkDir + absPath = filepath.Join(a.WorkDir, file) + } + + // Verify file exists + if _, err := os.Stat(absPath); os.IsNotExist(err) { + return nil, fmt.Errorf("file not found: %s", file) + } + + absFiles[i] = absPath + } + // PMD command format: pmd check -d -R -f json args := []string{ "check", - "-d", strings.Join(files, ","), // Comma-separated file list + "-d", strings.Join(absFiles, ","), // Comma-separated file list "-R", rulesetFile, "-f", "json", // JSON output format "--no-cache", // Disable cache for consistent results diff --git a/internal/validator/validator.go b/internal/validator/validator.go index 2d43ba8..d35950d 100644 --- a/internal/validator/validator.go +++ b/internal/validator/validator.go @@ -163,10 +163,19 @@ func (v *Validator) ValidateChanges(ctx context.Context, changes []GitChange) (* } // Collect all changed files + // Convert absolute paths to relative paths for RBAC checking changedFiles := make([]string, 0, len(changes)) for _, change := range changes { if change.Status != "D" { // Skip deleted files - changedFiles = append(changedFiles, change.FilePath) + filePath := change.FilePath + // Convert absolute path to relative path from workDir + if filepath.IsAbs(filePath) { + relPath, err := filepath.Rel(v.workDir, filePath) + if err == nil { + filePath = relPath + } + } + changedFiles = append(changedFiles, filePath) } } diff --git a/tests/integration/helper.go b/tests/integration/helper.go index a1fc100..ff26358 100644 --- a/tests/integration/helper.go +++ b/tests/integration/helper.go @@ -55,7 +55,27 @@ func loadPolicyFromTestdata(t *testing.T, relativePath string) *schema.CodePolic // createTestValidator creates a validator with given policy and registers cleanup func createTestValidator(t *testing.T, pol *schema.CodePolicy) *validator.Validator { t.Helper() + + // Save current directory + originalDir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current directory: %v", err) + } + + // Change to repo root so Validator.workDir is set correctly + repoRoot := getTestdataDir(t) + if err := os.Chdir(repoRoot); err != nil { + t.Fatalf("Failed to change to repo root: %v", err) + } + + // Create validator (it will use repo root as workDir) v := validator.NewValidator(pol, testing.Verbose()) + + // Change back to original directory + if err := os.Chdir(originalDir); err != nil { + t.Fatalf("Failed to change back to original directory: %v", err) + } + t.Cleanup(func() { if err := v.Close(); err != nil { t.Logf("Warning: failed to close validator: %v", err) @@ -85,25 +105,17 @@ func createGitChangeFromFile(t *testing.T, filePath string) validator.GitChange } } - // Convert absolute path to relative path from repo root - // FilePath should be relative like "testdata/rbac/src/components/Button.js" - // not absolute like "/workspace/testdata/rbac/src/components/Button.js" - relativePath := filePath - if filepath.IsAbs(filePath) { - // Get repo root + // Use absolute path for FilePath + // The adapters (checkstyle/pmd) will handle path resolution correctly + absPath := filePath + if !filepath.IsAbs(filePath) { + // If relative, make it absolute cwd, _ := os.Getwd() - projectRoot := filepath.Join(cwd, "../..") - absProjectRoot, _ := filepath.Abs(projectRoot) - - // Make path relative to project root - relPath, err := filepath.Rel(absProjectRoot, filePath) - if err == nil { - relativePath = relPath - } + absPath, _ = filepath.Abs(filepath.Join(cwd, filePath)) } return validator.GitChange{ - FilePath: relativePath, + FilePath: absPath, Status: "A", // Treat as Added file Diff: string(output), } From e38c44d8d3ce9e77bf951579fcec434b88b9fd85 Mon Sep 17 00:00:00 2001 From: ikjeong Date: Tue, 18 Nov 2025 14:52:43 +0000 Subject: [PATCH 4/6] test: Add testing.Short() checks to RBAC integration tests All 10 RBAC integration tests now check testing.Short() and skip in CI, ensuring consistency with other integration tests and preventing them from running in the CI 'Unit Test' job. --- tests/integration/validator_rbac_test.go | 40 ++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/integration/validator_rbac_test.go b/tests/integration/validator_rbac_test.go index 4954e70..ad634fe 100644 --- a/tests/integration/validator_rbac_test.go +++ b/tests/integration/validator_rbac_test.go @@ -10,6 +10,10 @@ import ( // TestValidator_RBAC_AdminFullAccess verifies that admin role has full write access func TestValidator_RBAC_AdminFullAccess(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "alice") // alice is admin // Admin should be able to modify any file @@ -39,6 +43,10 @@ func TestValidator_RBAC_AdminFullAccess(t *testing.T) { // TestValidator_RBAC_DeveloperAllowedFiles verifies that developer can modify allowed files func TestValidator_RBAC_DeveloperAllowedFiles(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "charlie") // charlie is developer // Developer should be able to modify src/components/* and tests/* @@ -65,6 +73,10 @@ func TestValidator_RBAC_DeveloperAllowedFiles(t *testing.T) { // TestValidator_RBAC_DeveloperDeniedCoreFiles verifies that developer cannot modify core files func TestValidator_RBAC_DeveloperDeniedCoreFiles(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "charlie") // charlie is developer // Developer should NOT be able to modify src/core/* @@ -84,6 +96,10 @@ func TestValidator_RBAC_DeveloperDeniedCoreFiles(t *testing.T) { // TestValidator_RBAC_DeveloperDeniedAPIFiles verifies that developer cannot modify API files func TestValidator_RBAC_DeveloperDeniedAPIFiles(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "charlie") // charlie is developer // Developer should NOT be able to modify src/api/* @@ -103,6 +119,10 @@ func TestValidator_RBAC_DeveloperDeniedAPIFiles(t *testing.T) { // TestValidator_RBAC_ViewerDenied verifies that viewer role has no write access func TestValidator_RBAC_ViewerDenied(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "frank") // frank is viewer // Viewer should NOT be able to modify any file @@ -130,6 +150,10 @@ func TestValidator_RBAC_ViewerDenied(t *testing.T) { // TestValidator_RBAC_MixedPermissions verifies mixed file permissions in a single commit func TestValidator_RBAC_MixedPermissions(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "charlie") // charlie is developer // Mix of allowed and denied files @@ -155,6 +179,10 @@ func TestValidator_RBAC_MixedPermissions(t *testing.T) { // TestValidator_RBAC_DeletedFilesSkipped verifies that deleted files don't trigger RBAC func TestValidator_RBAC_DeletedFilesSkipped(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "charlie") // charlie is developer // Deleted files should not trigger RBAC violations @@ -181,6 +209,10 @@ func TestValidator_RBAC_DeletedFilesSkipped(t *testing.T) { // TestValidator_RBAC_Disabled verifies that RBAC can be disabled func TestValidator_RBAC_Disabled(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + setupRBACEnvironment(t, "charlie") // charlie is developer // Load a policy with RBAC disabled @@ -210,6 +242,10 @@ func TestValidator_RBAC_Disabled(t *testing.T) { // TestValidator_RBAC_UnknownUser verifies behavior with unknown users func TestValidator_RBAC_UnknownUser(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "unknown_user") // User not in roles.json // Unknown user should have no permissions (default to viewer-like behavior) @@ -230,6 +266,10 @@ func TestValidator_RBAC_UnknownUser(t *testing.T) { // TestValidator_RBAC_GlobPatternMatching verifies glob pattern matching works correctly func TestValidator_RBAC_GlobPatternMatching(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + v := createRBACTestValidator(t, "charlie") // charlie is developer // Test various glob patterns From 69666eb67f355a6fd5fdd1e96ac95aa53ba5b563 Mon Sep 17 00:00:00 2001 From: ikjeong Date: Tue, 18 Nov 2025 14:56:56 +0000 Subject: [PATCH 5/6] fix: Add explicit return after t.Fatal to satisfy staticcheck SA5011 Adds explicit return statements after t.Fatal() calls in nil checks to help the linter understand that execution stops, preventing SA5011 warnings about possible nil pointer dereferences. --- internal/adapter/eslint/adapter_test.go | 1 + internal/adapter/prettier/adapter_test.go | 1 + internal/adapter/subprocess_test.go | 1 + 3 files changed, 3 insertions(+) diff --git a/internal/adapter/eslint/adapter_test.go b/internal/adapter/eslint/adapter_test.go index 2dda823..495b089 100644 --- a/internal/adapter/eslint/adapter_test.go +++ b/internal/adapter/eslint/adapter_test.go @@ -14,6 +14,7 @@ func TestNewAdapter(t *testing.T) { adapter := NewAdapter("", "") if adapter == nil { t.Fatal("NewAdapter() returned nil") + return } if adapter.ToolsDir == "" { diff --git a/internal/adapter/prettier/adapter_test.go b/internal/adapter/prettier/adapter_test.go index a5558f8..5ca6d78 100644 --- a/internal/adapter/prettier/adapter_test.go +++ b/internal/adapter/prettier/adapter_test.go @@ -14,6 +14,7 @@ func TestNewAdapter(t *testing.T) { a := NewAdapter("", "") if a == nil { t.Fatal("NewAdapter() returned nil") + return } if a.ToolsDir == "" { diff --git a/internal/adapter/subprocess_test.go b/internal/adapter/subprocess_test.go index 918b875..6a35ae0 100644 --- a/internal/adapter/subprocess_test.go +++ b/internal/adapter/subprocess_test.go @@ -10,6 +10,7 @@ func TestNewSubprocessExecutor(t *testing.T) { executor := NewSubprocessExecutor() if executor == nil { t.Fatal("NewSubprocessExecutor() returned nil") + return } if executor.Timeout != 2*time.Minute { From 2cd2f174676ceb012099cc0e81cfa7eba50313f9 Mon Sep 17 00:00:00 2001 From: ikjeong Date: Tue, 18 Nov 2025 15:04:57 +0000 Subject: [PATCH 6/6] fix: Add explicit return in tsc adapter test nil check Completes staticcheck SA5011 fixes by adding return after t.Fatal in tsc adapter test, ensuring all adapter tests satisfy the linter. --- internal/adapter/tsc/adapter_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/adapter/tsc/adapter_test.go b/internal/adapter/tsc/adapter_test.go index 2d815e9..c6dbedc 100644 --- a/internal/adapter/tsc/adapter_test.go +++ b/internal/adapter/tsc/adapter_test.go @@ -13,6 +13,7 @@ func TestNewAdapter(t *testing.T) { adapter := NewAdapter("", "") if adapter == nil { t.Fatal("NewAdapter() returned nil") + return } // Should have default ToolsDir