🔍 Duplicate Code Detected: Memory allowed-extensions Parsing Loop
Analysis of commit 2b5c6d4 (#67506)
Assignee: @copilot
Summary
Two memory-subsystem config parsers — one for cache-memory and one for drive-memory — contain a near-identical loop that reads the allowed-extensions list, validates each entry with isValidFileExtension, and appends it to the entry's AllowedExtensions slice. The bodies differ only by the entry struct type (*CacheMemoryEntry vs *DriveMemoryEntry) and local variable names; the logic, validation, and error message are byte-for-byte identical.
Impact Analysis
- Maintainability: Any change to extension validation (e.g. allowing a new character class, tightening the error message, or supporting a new config shape) must be made in two places, which is easy to miss.
- Bug Risk: The two copies can silently drift — a fix applied to one memory type but not the other produces inconsistent validation behaviour for
cache-memory vs drive-memory.
- Code Bloat: ~22 lines are duplicated verbatim;
repo-memory already uses a different helper (parseRepoMemoryStringList), so the codebase currently has three divergent approaches to the same concept.
Duplication Details
Pattern: allowed-extensions list parse + per-item validation
- Severity: Medium
- Occurrences: 2 (plus a third divergent variant in
repo_memory.go)
- Locations:
pkg/workflow/cache_config.go — parseCacheMemoryAllowedExtensions (lines 272–294)pkg/workflow/drive_memory_config.go — parseDriveMemoryAllowedExtensions (lines 148–173)
- Code Sample (
cache_config.go):
func parseCacheMemoryAllowedExtensions(cacheMap map[string]any, entry *CacheMemoryEntry) error {
allowedExts, exists := cacheMap["allowed-extensions"]
if !exists {
return nil
}
extArray, ok := allowedExts.([]any)
if !ok {
return nil
}
entry.AllowedExtensions = make([]string, 0, len(extArray))
for _, ext := range extArray {
extStr, ok := ext.(string)
if !ok {
continue
}
if !isValidFileExtension(extStr) {
return fmt.Errorf("invalid allowed-extension %q: must start with '.' followed by alphanumeric characters only (e.g. .json)", extStr)
}
entry.AllowedExtensions = append(entry.AllowedExtensions, extStr)
}
return nil
}
- Code Sample (
drive_memory_config.go, functionally identical):
func parseDriveMemoryAllowedExtensions(raw map[string]any, entry *DriveMemoryEntry) error {
value, exists := raw["allowed-extensions"]
if !exists {
return nil
}
values, ok := value.([]any)
if !ok {
return nil
}
entry.AllowedExtensions = make([]string, 0, len(values))
for _, value := range values {
extension, ok := value.(string)
if !ok {
continue
}
if !isValidFileExtension(extension) {
return fmt.Errorf("invalid allowed-extension %q: must start with '.' followed by alphanumeric characters only (e.g. .json)", extension)
}
entry.AllowedExtensions = append(entry.AllowedExtensions, extension)
}
return nil
}
Refactoring Recommendations
-
Extract a shared helper that returns the parsed slice
- Add e.g.
parseMemoryAllowedExtensions(configMap map[string]any) ([]string, error) (suggested home: pkg/workflow/memory_validation_config.go, alongside the other shared memory helpers, or cache_config.go since isValidFileExtension already lives there).
- Have both callers assign the result:
exts, err := parseMemoryAllowedExtensions(m); entry.AllowedExtensions = exts.
- Returning the slice (rather than mutating a typed
*Entry) avoids coupling the helper to either struct type and lets repo-memory adopt it too.
- Estimated effort: ~1 hour (small, mechanical, well covered by existing
cache_memory_*_test.go / drive_memory_test.go).
- Benefits: single source of truth for extension validation and the shared error message; removes drift risk between memory types.
-
(Optional) Converge repo-memory
- Evaluate whether
parseRepoMemoryStringList for allowed-extensions can route through the same helper so all three memory types validate extensions identically.
Implementation Checklist
Analysis Metadata
- Analyzed Files: memory-subsystem source files under
pkg/workflow/ (cache/drive/repo/comment/validation)
- Detection Method: Serena semantic code analysis + pattern search
- Commit: 2b5c6d4
- Scope note: repository is a single squashed commit, so analysis focused on the commit's stated theme (memory schema contract + safe-output runtime)
Generated by 🔍 Duplicate Code Detector · pi · opus48 · 104.2 AIC · ⌖ 45.4 AIC · ⊞ 1.5K · ◷
🔍 Duplicate Code Detected: Memory
allowed-extensionsParsing LoopAnalysis of commit 2b5c6d4 (#67506)
Assignee:
@copilotSummary
Two memory-subsystem config parsers — one for
cache-memoryand one fordrive-memory— contain a near-identical loop that reads theallowed-extensionslist, validates each entry withisValidFileExtension, and appends it to the entry'sAllowedExtensionsslice. The bodies differ only by the entry struct type (*CacheMemoryEntryvs*DriveMemoryEntry) and local variable names; the logic, validation, and error message are byte-for-byte identical.Impact Analysis
cache-memoryvsdrive-memory.repo-memoryalready uses a different helper (parseRepoMemoryStringList), so the codebase currently has three divergent approaches to the same concept.Duplication Details
Pattern:
allowed-extensionslist parse + per-item validationrepo_memory.go)pkg/workflow/cache_config.go—parseCacheMemoryAllowedExtensions(lines 272–294)pkg/workflow/drive_memory_config.go—parseDriveMemoryAllowedExtensions(lines 148–173)cache_config.go):drive_memory_config.go, functionally identical):Refactoring Recommendations
Extract a shared helper that returns the parsed slice
parseMemoryAllowedExtensions(configMap map[string]any) ([]string, error)(suggested home:pkg/workflow/memory_validation_config.go, alongside the other shared memory helpers, orcache_config.gosinceisValidFileExtensionalready lives there).exts, err := parseMemoryAllowedExtensions(m); entry.AllowedExtensions = exts.*Entry) avoids coupling the helper to either struct type and letsrepo-memoryadopt it too.cache_memory_*_test.go/drive_memory_test.go).(Optional) Converge
repo-memoryparseRepoMemoryStringListforallowed-extensionscan route through the same helper so all three memory types validate extensions identically.Implementation Checklist
parseMemoryAllowedExtensionshelpercache_config.goanddrive_memory_config.goto call itrepo_memory.goonto the helpermake fmtandmake test-unitAnalysis Metadata
pkg/workflow/(cache/drive/repo/comment/validation)