feat: centralize hardcoded config into DEFAULTS with recursive deep merge#506
feat: centralize hardcoded config into DEFAULTS with recursive deep merge#506carlos-alm merged 8 commits intomainfrom
Conversation
…erge Route ~50 behavioral constants through the existing .codegraphrc.json config system so users can tune thresholds, depths, weights, and limits without editing source code. - Add analysis, community, structure, risk, display, mcp sections to DEFAULTS - Fix mergeConfig to recurse so partial overrides of nested objects (e.g. risk.weights.complexity) preserve unspecified sibling keys - Wire analysis depths: impact, fn-impact, audit, sequence, brief, module-map - Wire risk scoring: scoreRisk accepts roleWeights/defaultRoleWeight via opts; triage passes config-based weights through - Wire search: hybrid/semantic read topK, rrfK, minScore, similarityWarnThreshold - Wire display: result-formatter maxColWidth, file-utils excerpt/scan/gather params - Wire MCP: initMcpDefaults() at server startup for config-based page sizes - Document config guidance in CLAUDE.md - Add 14 new unit tests for DEFAULTS shape, recursive merge, and config loading Impact: 24 functions changed, 18 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Configuration Centralization
|
Greptile SummaryThis PR centralizes ~50 scattered behavioral constants into a structured Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
RC[".codegraphrc.json"] -->|"loadConfig()"| MC["mergeConfig(DEFAULTS, userConfig)\n(recursive deep-merge)"]
MC --> CFG["config object"]
CFG -->|"config.analysis"| AN["Analysis modules\nbriefData · fnImpactData\ndiffImpactData · auditData\nsequenceData · moduleMapData"]
CFG -->|"config.risk"| TR["triageData\n→ scoreRisk(items, weights, riskOpts)"]
CFG -->|"config.search"| SR["hybridSearchData\nsemanticSearchData"]
CFG -->|"config.display"| DI["file-utils\nreadSourceRange · extractSummary\nextractSignature"]
CFG -->|"config.structure"| ST["moduleBoundariesData\n(cohesionThreshold)"]
CFG -->|"config.community"| CM["communitiesData\n(resolution)"]
CFG -->|"config.mcp.defaults"| MCP["initMcpDefaults()\n→ effectiveLimit()"]
CFG -->|"config.display.maxColWidth"| FMT["printAutoTable"]
TR -.->|"opts.roleWeights = config.risk.roleWeights\n(11-entry set)"| RW["scoreRisk()"]
RW -.->|"fallback (no opts)"| RWCONST["ROLE_WEIGHTS constant\n⚠ only 7 entries — stale"]
style RWCONST fill:#ffcccc,stroke:#cc0000
Last reviewed commit: "fix: thread config.d..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function readSourceRange(repoRoot, file, startLine, endLine, opts = {}) { | ||
| try { | ||
| const absPath = safePath(repoRoot, file); | ||
| if (!absPath) return null; | ||
| const content = fs.readFileSync(absPath, 'utf-8'); | ||
| const lines = content.split('\n'); | ||
| const excerptLines = opts.excerptLines ?? 50; | ||
| const start = Math.max(0, (startLine || 1) - 1); | ||
| const end = Math.min(lines.length, endLine || startLine + 50); | ||
| const end = Math.min(lines.length, endLine || startLine + excerptLines); | ||
| return lines.slice(start, end).join('\n'); | ||
| } catch (e) { | ||
| debug(`readSourceRange failed for ${file}: ${e.message}`); |
There was a problem hiding this comment.
display config opts never passed by callers
The display config section in DEFAULTS (excerptLines, summaryMaxChars, jsdocEndScanLines, jsdocOpenScanLines, signatureGatherLines) was added to enable user-tunable display behavior, and the function signatures here correctly accept an opts argument. However, every call site in src/domain/analysis/context.js and src/domain/analysis/exports.js invokes these functions without any opts:
// context.js line 37
const summary = cLines ? extractSummary(cLines, c.line) : null;
// context.js line 103-104
signature: fileLines ? extractSignature(fileLines, s.line) : null,
summary: fileLines ? extractSummary(fileLines, s.line) : null,
// context.js line 331
: readSourceRange(repoRoot, node.file, node.line, node.end_line);Because no opts is ever passed, the functions always fall back to the hardcoded defaults in the parameter list (e.g. opts.excerptLines ?? 50), meaning any value a user sets under display in .codegraphrc.json is silently ignored. Similarly, opts.display in result-formatter.js is never populated from config anywhere in the CLI command handlers, so display.maxColWidth is also unreachable.
The display config section as merged from loadConfig() needs to be threaded through to these call sites, otherwise these config keys are effectively dead.
There was a problem hiding this comment.
Fixed. Added loadConfig() calls in contextData, explainData (context.js) and exportsData (exports.js), extracting config.display and threading it as the opts parameter through all internal calls to readSourceRange, extractSummary, and extractSignature — including buildCallees, explainFileImpl, explainFunctionImpl, explainCallees, and exportsFileImpl. Pattern used: const config = opts.config || loadConfig(); const displayOpts = config.display || {}; at each entry point, then passed down.
| community: { | ||
| resolution: 1.0, | ||
| }, |
There was a problem hiding this comment.
community.resolution is never consumed from config
community.resolution is added to DEFAULTS (and will be merged into user config), but src/features/communities.js reads the resolution value directly from opts.resolution:
// features/communities.js line 147
const resolution = opts.resolution ?? 1.0;The communities feature is not in the changed files for this PR and no wiring from config.community.resolution to opts.resolution was added. As a result, a user setting { "community": { "resolution": 2.0 } } in .codegraphrc.json will have no effect — the DEFAULTS entry is unreachable by the actual feature code.
There was a problem hiding this comment.
Fixed. communitiesData now loads config and uses a three-level fallback: opts.resolution ?? config.community?.resolution ?? 1.0. This makes the community.resolution DEFAULTS entry actually consumable via .codegraphrc.json.
| /** Resolved MCP defaults (may include config overrides). Set via initMcpDefaults(). */ | ||
| let resolvedDefaults = MCP_DEFAULTS; | ||
|
|
||
| /** | ||
| * Initialize MCP defaults from config. Call once at server startup. | ||
| * @param {object} [configMcpDefaults] - config.mcp.defaults overrides | ||
| */ | ||
| export function initMcpDefaults(configMcpDefaults) { | ||
| resolvedDefaults = getMcpDefaults(configMcpDefaults); | ||
| } |
There was a problem hiding this comment.
resolvedDefaults is a module-level variable mutated by initMcpDefaults. Since ES modules are cached by the Node.js module registry, a call to initMcpDefaults in one test leaks into all subsequent tests that import effectiveLimit — there is no teardown/reset mechanism.
Consider exporting a resetMcpDefaults helper (or a getter pattern) so tests can clean up after themselves:
export function resetMcpDefaults() {
resolvedDefaults = MCP_DEFAULTS;
}Alternatively, pass resolvedDefaults as a parameter to effectiveLimit to avoid the shared mutable state entirely.
There was a problem hiding this comment.
Fixed. Added resetMcpDefaults() export that sets resolvedDefaults = MCP_DEFAULTS. Tests can call this in afterEach to avoid cross-test pollution from initMcpDefaults().
The file-utils functions readSourceRange, extractSummary, and extractSignature accept display opts but callers in context.js and exports.js never passed them, making the centralized display config dead code. Load config.display in contextData, explainData, and exportsData, then thread it through to all internal call sites. Impact: 9 functions changed, 5 affected
The community.resolution default was defined in DEFAULTS but never consumed. communitiesData read opts.resolution ?? 1.0 directly, ignoring the config. Now falls back through opts.resolution ?? config.community?.resolution ?? 1.0. Impact: 1 functions changed, 1 affected
The module-level resolvedDefaults in middleware.js persists across tests, causing test isolation issues. Add a resetMcpDefaults() export that restores resolvedDefaults back to MCP_DEFAULTS so tests can clean up after calling initMcpDefaults(). Impact: 1 functions changed, 0 affected
Replace || with ?? for minScore, limit, rrfK, and topK in semantic.js and hybrid.js so that explicit 0 values are respected instead of being treated as falsy and falling through to defaults. Impact: 3 functions changed, 1 affected
|
All four review findings have been addressed in separate commits:
All 1922 tests pass, lint is clean. |
Replace || with ?? so an explicit threshold of 0.0 (meaning include all directories) is not silently discarded in favor of the config or default value. Impact: 1 functions changed, 0 affected
CLI commands pass opts to outputResult but never populate opts.display from config, making display.maxColWidth unreachable via .codegraphrc.json. Now outputResult falls back to loadConfig().display when opts.display is not provided by the caller. Impact: 1 functions changed, 2 affected
|
Addressed the two remaining issues from Greptile's summary:
All 1922 tests pass, lint clean. |
1 participant
Summary
.codegraphrc.jsonconfig system so users can tune thresholds, depths, weights, and limits without editing source codemergeConfigto recurse so partial overrides of nested objects (e.g.risk.weights.complexity) preserve unspecified sibling keys instead of silently dropping themTest plan
.codegraphrc.jsonoverrides flow through to affected commands (manual)