Skip to content

Fix merge issues from PR #3 (--config CLI flag)#6

Merged
tomohiro-owada merged 1 commit intomainfrom
fix/config-cli-merge-issues
Dec 15, 2025
Merged

Fix merge issues from PR #3 (--config CLI flag)#6
tomohiro-owada merged 1 commit intomainfrom
fix/config-cli-merge-issues

Conversation

@tomohiro-owada
Copy link
Copy Markdown
Owner

@tomohiro-owada tomohiro-owada commented Dec 15, 2025

User description

Summary

Fixes build and test failures introduced by merge conflicts in PR #3.

Changes

  • config.go: Fix undefined variable configFileconfigPath
  • config_test.go: Restore complete TestLoadConfig_CustomPath test (was truncated)
  • sync.go: Remove unused path/filepath import
  • integration_test.go: Update DocumentsDirDocumentPatterns (6 occurrences)

Test plan

  • All unit tests pass
  • All integration tests pass
  • Build succeeds

🤖 Generated with Claude Code


PR Type

Bug fix, Tests


Description

  • Fix undefined variable configFileconfigPath in config.go

  • Restore complete TestLoadConfig_CustomPath test (was truncated)

  • Update integration tests: DocumentsDirDocumentPatterns (6 occurrences)

  • Remove unused path/filepath import from sync.go


Diagram Walkthrough

flowchart LR
  A["Merge Conflict Issues"] --> B["Variable Name Fix"]
  A --> C["Test Restoration"]
  A --> D["API Migration"]
  A --> E["Import Cleanup"]
  B --> F["config.go: configFile → configPath"]
  C --> G["config_test.go: Restore TestLoadConfig_CustomPath"]
  D --> H["integration_test.go: DocumentsDir → DocumentPatterns"]
  E --> I["sync.go: Remove unused import"]
Loading

File Walkthrough

Relevant files
Bug fix
config.go
Fix undefined variable in config loading                                 

internal/config/config.go

  • Fix undefined variable reference: configFileconfigPath in fprintf
    call
  • Corrects variable name to match function parameter
+1/-1     
Tests
config_test.go
Restore and update config tests for new API                           

internal/config/config_test.go

  • Restore complete TestLoadConfig_CustomPath test that was truncated
  • Update test config to use document_patterns instead of deprecated
    documents_dir
  • Update all test assertions to verify DocumentPatterns array instead of
    DocumentsDir string
  • Reorganize test functions for better logical flow
+52/-46 
integration_test.go
Update integration tests for DocumentPatterns API               

integration_test.go

  • Update 6 test functions to use DocumentPatterns array instead of
    DocumentsDir string
  • Affected tests: TestEndToEnd_FirstRun, TestEndToEnd_Reindex,
    TestEndToEnd_MultipleFiles, TestEndToEnd_DeleteDocument,
    TestEndToEnd_Sync, TestEndToEnd_EmptyDirectory
  • Change pattern: cfg.DocumentsDir = testDircfg.DocumentPatterns =
    []string{testDir}
+6/-6     
Miscellaneous
sync.go
Remove unused import statement                                                     

internal/indexer/sync.go

  • Remove unused path/filepath import
  • Cleanup from merge conflict resolution
+0/-1     

- Fix undefined variable: configFile -> configPath in config.go
- Fix truncated test: restore complete TestLoadConfig_CustomPath
- Remove unused import: path/filepath from sync.go
- Update integration tests: DocumentsDir -> DocumentPatterns

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@qodo-code-review
Copy link
Copy Markdown

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Audit logging: The new info log of configuration loading is present but tests and other changes do not
demonstrate comprehensive logging of critical actions beyond config load, which may be
acceptable if outside PR scope.

Referred Code
fmt.Fprintf(os.Stderr, "[INFO] Loaded configuration from %s\n", configPath)
return cfg, nil

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation: The PR touches configuration loading and defaults but does not show validation of external
config file contents beyond presence/migration, which may exist elsewhere.

Referred Code
// Validate that at least one pattern is configured
if len(cfg.DocumentPatterns) == 0 {
	cfg.DocumentPatterns = []string{"./documents"}
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Improve test for backward compatibility

Refactor TestLoadConfig_BackwardsCompatibility to use sub-tests and add a new
test case. The new test should verify that document_patterns takes precedence
when both documents_dir and document_patterns are present in the configuration
file.

internal/config/config_test.go [248-291]

 func TestLoadConfig_BackwardsCompatibility(t *testing.T) {
-	// Test that old documents_dir format is migrated to document_patterns
-	tmpDir := t.TempDir()
-	originalDir, _ := os.Getwd()
-	defer os.Chdir(originalDir)
+	t.Run("migrates from documents_dir", func(t *testing.T) {
+		// Test that old documents_dir format is migrated to document_patterns
+		tmpDir := t.TempDir()
+		originalDir, _ := os.Getwd()
+		defer os.Chdir(originalDir)
 
-	os.Chdir(tmpDir)
+		os.Chdir(tmpDir)
 
-	// Create test config with old format
-	oldConfig := `{
-   "documents_dir": "./old_docs",
-   "db_path": "./test.db",
-   "chunk_size": 300,
-   "search_top_k": 10,
-   "compute": {
-     "device": "cpu",
-     "fallback_to_cpu": false
-   },
-   "model": {
-     "name": "test-model",
-     "dimensions": 256
-   }
- }`
+		// Create test config with old format
+		oldConfig := `{
+	   "documents_dir": "./old_docs",
+	   "db_path": "./test.db"
+	 }`
 
-	if err := os.WriteFile("config.json", []byte(oldConfig), 0644); err != nil {
-		t.Fatal(err)
-	}
+		if err := os.WriteFile("config.json", []byte(oldConfig), 0644); err != nil {
+			t.Fatal(err)
+		}
 
-	cfg, err := Load("")
-	if err != nil {
-		t.Fatalf("Expected no error, got %v", err)
-	}
+		cfg, err := Load("")
+		if err != nil {
+			t.Fatalf("Expected no error, got %v", err)
+		}
 
-	// Verify migration
-	if len(cfg.DocumentPatterns) != 1 {
-		t.Errorf("Expected 1 document pattern after migration, got %d", len(cfg.DocumentPatterns))
-	}
-	if cfg.DocumentPatterns[0] != "./old_docs" {
-		t.Errorf("Expected pattern './old_docs', got %s", cfg.DocumentPatterns[0])
-	}
-	if cfg.DocumentsDir != "" {
-		t.Errorf("Expected deprecated DocumentsDir to be cleared, got %s", cfg.DocumentsDir)
-	}
+		// Verify migration
+		if len(cfg.DocumentPatterns) != 1 {
+			t.Errorf("Expected 1 document pattern after migration, got %d", len(cfg.DocumentPatterns))
+		}
+		if cfg.DocumentPatterns[0] != "./old_docs" {
+			t.Errorf("Expected pattern './old_docs', got %s", cfg.DocumentPatterns[0])
+		}
+		if cfg.DocumentsDir != "" {
+			t.Errorf("Expected deprecated DocumentsDir to be cleared, got %s", cfg.DocumentsDir)
+		}
+	})
+
+	t.Run("prefers document_patterns when both are present", func(t *testing.T) {
+		tmpDir := t.TempDir()
+		originalDir, _ := os.Getwd()
+		defer os.Chdir(originalDir)
+		os.Chdir(tmpDir)
+
+		mixedConfig := `{
+			"documents_dir": "./old_docs",
+			"document_patterns": ["./new_patterns"]
+		}`
+		if err := os.WriteFile("config.json", []byte(mixedConfig), 0644); err != nil {
+			t.Fatal(err)
+		}
+
+		cfg, err := Load("")
+		if err != nil {
+			t.Fatalf("Expected no error, got %v", err)
+		}
+
+		if len(cfg.DocumentPatterns) != 1 {
+			t.Fatalf("Expected 1 document pattern, got %d", len(cfg.DocumentPatterns))
+		}
+		if cfg.DocumentPatterns[0] != "./new_patterns" {
+			t.Errorf("Expected document_patterns to be preferred, got %s", cfg.DocumentPatterns[0])
+		}
+	})
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a missing edge case in the new backward compatibility test, where both old and new config fields might exist, and proposes a robust test to ensure the new field takes precedence.

Medium
  • More

@tomohiro-owada tomohiro-owada deleted the fix/config-cli-merge-issues branch December 15, 2025 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant