Skip to content

Commit 07e8bc0

Browse files
carlos-almcarlos-alm
authored
docs: plan to centralize hardcoded configuration (#482)
* docs: add plan to centralize hardcoded configuration values Inventory 78+ hardcoded magic numbers across the codebase and design a 6-phase plan to route them through the existing .codegraphrc.json config system. Includes PR #480 brief command thresholds. * fix: address Greptile review feedback on config centralization plan (#482) - Fix magic-number count: ~70 individual values (34 entries), add derivation - Make recursive mergeConfig a hard Phase 1 prerequisite (not optional) - Move MCP_MAX_LIMIT to Category F (keep hardcoded — security boundary) - Merge Phase 7 into Phase 6 to fix phase count mismatch * fix: address round-2 Greptile review feedback (#482) - Keep build.driftThreshold in `build` namespace (not community) - Add `structure.cohesionThreshold` for A8 instead - Fix stale line numbers for A9-A12 (brief.js) - Split briefBfsDepth into briefCallerDepth + briefImporterDepth - Clarify check.js uses config.check.depth as sole authoritative key * fix: address round-3 Greptile review feedback (#482) - Add C5 (similarityWarnThreshold) to DEFAULTS schema and Phase 4 wiring - Split D4 into D4a (jsdocEndScanLines: 10) and D4b (jsdocOpenScanLines: 20) - Update Phase 5 file-utils.js wiring to reference both D4 keys * fix: address round-4 Greptile review feedback (#482) - Move C6 (embedding batch sizes) to Category F with rationale: model-specific implementation details rarely tuned by end-users - Rename analysis.defaultDepth to analysis.impactDepth for consistency with other per-feature depth keys (auditDepth, sequenceDepth, etc.) - Remove ambiguous Phase 4 wording about models.js configurability * docs: add native engine depth constants to config inventory (F7-F11) Reflects MAX_WALK_DEPTH constants introduced by #484 (fix for #481) and the pre-existing MAX_VISIT_DEPTH in dataflow.rs plus MCP_MAX_LIMIT. All are safety boundaries that should stay hardcoded. * fix: address round-5 Greptile review feedback (#482) - Add config.search.topK (C3) to semantic.js wiring in Phase 4 - Add structure to the Phase 6 CLAUDE.md configurable sections list
1 parent fdc61a8 commit 07e8bc0

1 file changed

Lines changed: 285 additions & 0 deletions

File tree

‎docs/tasks/PLAN_centralize_config.md‎

Lines changed: 285 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,285 @@
1+
# Plan: Centralize Hardcoded Configuration
2+
3+
> **Goal:** Eliminate magic numbers scattered across the codebase by routing all tunable parameters through the existing `.codegraphrc.json` config system (`DEFAULTS` in `src/infrastructure/config.js`).
4+
5+
## Problem
6+
7+
The config system already exists and handles env overrides, but ~70 individual behavioral constants (34 inventory entries expanding to ~70 discrete values when counting sub-keys in B1, B2, and E1) are hardcoded in individual modules and never read from config. Users cannot tune thresholds, depths, weights, or limits without editing source code.
8+
9+
---
10+
11+
## Inventory of Hardcoded Values
12+
13+
### Category A — Analysis Parameters (high user value)
14+
15+
| # | Value | File | Line | Controls |
16+
|---|-------|------|------|----------|
17+
| A1 | `maxDepth = 5` | `domain/analysis/impact.js` | 111 | `fn-impact` transitive caller depth |
18+
| A2 | `maxDepth = 3` | `domain/analysis/impact.js` | 31, 144 | BFS default depth for impact/diff-impact |
19+
| A3 | `maxDepth = 3` | `features/audit.js` | 102 | Audit blast-radius depth |
20+
| A4 | `maxDepth = 3` | `features/check.js` | 220 | CI check blast-radius depth |
21+
| A5 | `maxDepth = 10` | `features/sequence.js` | 91 | Sequence diagram traversal depth |
22+
| A6 | `FALSE_POSITIVE_CALLER_THRESHOLD = 20` | `domain/analysis/module-map.js` | 37 | Generic function false-positive filter |
23+
| A7 | `resolution = 1.0` | `graph/algorithms/louvain.js` | 17 | Louvain community detection granularity |
24+
| A8 | `driftThreshold = 0.3` | `features/structure.js` | 581 | Structure cohesion drift warning |
25+
| A9 | `maxCallers >= 10` | `domain/analysis/brief.js` | 38 | `brief` high-risk tier threshold |
26+
| A10 | `maxCallers >= 3` | `domain/analysis/brief.js` | 39 | `brief` medium-risk tier threshold |
27+
| A11 | `maxDepth = 5` | `domain/analysis/brief.js` | 47 | `brief` transitive caller BFS depth |
28+
| A12 | `maxDepth = 5` | `domain/analysis/brief.js` | 73 | `brief` transitive importer BFS depth |
29+
30+
### Category B — Risk & Scoring Weights (medium-high user value)
31+
32+
| # | Value | File | Line | Controls |
33+
|---|-------|------|------|----------|
34+
| B1 | `fanIn: 0.25, complexity: 0.3, churn: 0.2, role: 0.15, mi: 0.1` | `graph/classifiers/risk.js` | 10-14 | Risk score weighting |
35+
| B2 | `core: 1.0, utility: 0.9, entry: 0.8, adapter: 0.5, leaf: 0.2, dead: 0.1` | `graph/classifiers/risk.js` | 21-27 | Role importance weights |
36+
| B3 | `DEFAULT_ROLE_WEIGHT = 0.5` | `graph/classifiers/risk.js` | 30 | Fallback role weight |
37+
38+
### Category C — Search & Embedding (already partially in config)
39+
40+
| # | Value | File | Line | Controls |
41+
|---|-------|------|------|----------|
42+
| C1 | `limit = 15` | `domain/search/search/hybrid.js` | 12 | Hybrid search default limit |
43+
| C2 | `rrfK = 60` | `domain/search/search/hybrid.js` | 13 | RRF fusion constant |
44+
| C3 | `limit = 15` | `domain/search/search/semantic.js` | 12 | Semantic search default limit |
45+
| C4 | `minScore = 0.2` | `domain/search/search/semantic.js` | 13, 52 | Minimum similarity threshold |
46+
| C5 | `SIMILARITY_WARN_THRESHOLD = 0.85` | `domain/search/search/semantic.js` | 71 | Duplicate query warning |
47+
| ~~C6~~ | ~~Batch sizes per model~~ | — | — | Moved to Category F (see below) |
48+
49+
### Category D — Display & Truncation (low-medium user value)
50+
51+
| # | Value | File | Line | Controls |
52+
|---|-------|------|------|----------|
53+
| D1 | `MAX_COL_WIDTH = 40` | `presentation/result-formatter.js` | 82 | Table column width |
54+
| D2 | `50 lines` | `shared/file-utils.js` | 23 | Source context excerpt length |
55+
| D3 | `100 chars` | `shared/file-utils.js` | 48, 63 | Summary/docstring truncation |
56+
| D4a | `10 lines` | `shared/file-utils.js` | 36 | JSDoc block-end scan depth (upward scan for `*/`) |
57+
| D4b | `20 lines` | `shared/file-utils.js` | 54 | JSDoc opening scan depth (upward scan for `/**`) |
58+
| D5 | `5 lines` | `shared/file-utils.js` | 76 | Multi-line signature gather |
59+
60+
### Category E — MCP Pagination (medium user value)
61+
62+
| # | Value | File | Line | Controls |
63+
|---|-------|------|------|----------|
64+
| E1 | `MCP_DEFAULTS` (22 entries) | `shared/paginate.js` | 9-34 | Per-tool default page sizes |
65+
| ~~E2~~ | ~~`MCP_MAX_LIMIT = 1000`~~ | — | — | Moved to Category F (see below) |
66+
67+
### Category F — Infrastructure (low user value, keep hardcoded)
68+
69+
| # | Value | File | Line | Controls |
70+
|---|-------|------|------|----------|
71+
| F1 | `CACHE_TTL_MS = 86400000` | `infrastructure/update-check.js` | 10 | Version check cache (24h) |
72+
| F2 | `FETCH_TIMEOUT_MS = 3000` | `infrastructure/update-check.js` | 11 | Version check HTTP timeout |
73+
| F3 | `debounce = 300` | `domain/graph/watcher.js` | 80 | File watcher debounce (ms) |
74+
| F4 | `maxBuffer = 10MB` | `features/check.js` | 260 | Git diff buffer |
75+
| F5 | `volume / 3000` | `features/complexity.js` | 85 | Halstead bugs formula (standard) |
76+
| F6 | `timeout = 10_000` | `infrastructure/config.js` | 110 | apiKeyCommand timeout |
77+
| F7 | `MCP_MAX_LIMIT = 1000` | `shared/paginate.js` | 37 | Hard abuse-prevention cap — server-side safety boundary, not a tuning knob |
78+
| F8 | Batch sizes per model | `domain/search/models.js` | 66-75 | Embedding batch sizes — model-specific implementation details rarely tuned by end-users, analogous to watcher debounce (F3) |
79+
| F9 | `MAX_VISIT_DEPTH = 200` | `crates/.../dataflow.rs` | 11 | Dataflow AST visit recursion limit — stack overflow prevention |
80+
| F10 | `MAX_WALK_DEPTH = 200` | `crates/.../extractors/helpers.rs` | 6 | Extractor AST walk recursion limit — stack overflow prevention (#481) |
81+
| F11 | `MAX_WALK_DEPTH = 200` | `crates/.../complexity.rs` | 6 | Complexity walk recursion limit — stack overflow prevention (#481) |
82+
| F12 | `MAX_WALK_DEPTH = 200` | `crates/.../cfg.rs` | 5 | CFG process_if recursion limit — stack overflow prevention (#481) |
83+
84+
---
85+
86+
## Design
87+
88+
### Proposed `DEFAULTS` additions in `src/infrastructure/config.js`
89+
90+
```js
91+
export const DEFAULTS = {
92+
// ... existing fields ...
93+
94+
analysis: {
95+
impactDepth: 3, // A2: BFS depth for impact/diff-impact
96+
fnImpactDepth: 5, // A1: fn-impact transitive depth
97+
auditDepth: 3, // A3: audit blast-radius depth
98+
sequenceDepth: 10, // A5: sequence diagram depth
99+
falsePositiveCallers: 20, // A6: generic function filter threshold
100+
briefCallerDepth: 5, // A11: brief transitive caller BFS depth
101+
briefImporterDepth: 5, // A12: brief transitive importer BFS depth
102+
briefHighRiskCallers: 10, // A9: brief high-risk tier threshold
103+
briefMediumRiskCallers: 3, // A10: brief medium-risk tier threshold
104+
},
105+
106+
community: {
107+
resolution: 1.0, // A7: Louvain resolution (only Louvain params here)
108+
},
109+
110+
// build.driftThreshold stays in `build` (already wired in finalize.js line 52)
111+
// — it's a build-pipeline concern, not community detection
112+
113+
structure: {
114+
cohesionThreshold: 0.3, // A8: structure cohesion drift warning
115+
},
116+
117+
risk: {
118+
weights: { // B1
119+
fanIn: 0.25,
120+
complexity: 0.3,
121+
churn: 0.2,
122+
role: 0.15,
123+
mi: 0.1,
124+
},
125+
roleWeights: { // B2
126+
core: 1.0,
127+
utility: 0.9,
128+
entry: 0.8,
129+
adapter: 0.5,
130+
leaf: 0.2,
131+
dead: 0.1,
132+
},
133+
defaultRoleWeight: 0.5, // B3
134+
},
135+
136+
display: {
137+
maxColWidth: 40, // D1
138+
excerptLines: 50, // D2
139+
summaryMaxChars: 100, // D3
140+
jsdocEndScanLines: 10, // D4a: lines to scan upward for block-end marker (*/)
141+
jsdocOpenScanLines: 20, // D4b: lines to scan upward for /** opening
142+
signatureGatherLines: 5, // D5
143+
},
144+
145+
search: {
146+
// defaultMinScore, rrfK, topK already exist in DEFAULTS —
147+
// add the missing C5 key:
148+
similarityWarnThreshold: 0.85, // C5: duplicate-query warning in multiSearchData
149+
},
150+
151+
mcp: {
152+
defaults: { /* E1: current MCP_DEFAULTS object */ },
153+
// MCP_MAX_LIMIT stays hardcoded (Category F) — server-side safety boundary
154+
},
155+
};
156+
```
157+
158+
### What stays hardcoded (Category F)
159+
160+
- **Halstead `volume / 3000`** — industry-standard formula, not a tuning knob
161+
- **Git `maxBuffer`** — platform concern, not analysis behavior
162+
- **`apiKeyCommand` timeout** — security boundary, not user-facing
163+
- **Update check TTL/timeout** — implementation detail
164+
- **Watcher debounce** — could be configurable later but low priority
165+
- **`MCP_MAX_LIMIT`** — server-side abuse-prevention cap; making it user-configurable via `.codegraphrc.json` would allow any process with project directory write access to raise it arbitrarily, defeating its security purpose
166+
- **Embedding batch sizes** — model-specific implementation details (per-model map shape); rarely tuned by end-users, analogous to watcher debounce
167+
- **Native engine `MAX_WALK_DEPTH` / `MAX_VISIT_DEPTH` (200)** — stack overflow safety boundaries in Rust extractors, complexity, CFG, and dataflow modules; raising them risks process crashes on deeply nested ASTs
168+
169+
---
170+
171+
## Implementation Plan
172+
173+
### Phase 1 — Extend DEFAULTS schema (1 PR)
174+
175+
**Files:** `src/infrastructure/config.js`, `tests/unit/config.test.js`
176+
177+
1. Add `analysis`, `community`, `structure`, `risk`, `display`, `mcp` sections to `DEFAULTS`
178+
2. Keep `build.driftThreshold` where it is (already wired in `finalize.js` — no migration needed)
179+
3. **Hard prerequisite:** Update `mergeConfig` to perform recursive (deep) merging — at minimum 2 levels deep. The current implementation only merges 1 level deep, which means partial user overrides of nested objects like `risk.weights` (e.g. `{ "complexity": 0.4, "churn": 0.1 }`) will **silently drop** un-specified sibling keys (`fanIn`, `role`, `mi`), producing `NaN` risk scores. This must be fixed before any nested config keys are wired in subsequent phases
180+
4. Add tests: loading config with overrides for each new section
181+
182+
### Phase 2 — Wire analysis parameters (1 PR)
183+
184+
**Files to change:**
185+
- `src/domain/analysis/impact.js` → read `config.analysis.impactDepth` / `config.analysis.fnImpactDepth`
186+
- `src/features/audit.js` → read `config.analysis.auditDepth`
187+
- `src/features/check.js` → replace hardcoded `3` with `config.check.depth` (already in DEFAULTS, sole authoritative key for check depth — do **not** chain with `config.analysis.impactDepth`)
188+
- `src/features/sequence.js` → read `config.analysis.sequenceDepth`
189+
- `src/domain/analysis/module-map.js` → read `config.analysis.falsePositiveCallers`
190+
- `src/domain/analysis/brief.js` → read `config.analysis.briefCallerDepth`, `config.analysis.briefImporterDepth`, `config.analysis.briefHighRiskCallers`, `config.analysis.briefMediumRiskCallers` (PR #480)
191+
192+
**Pattern:** Each module calls `loadConfig()` (or receives config as a parameter). Replace the hardcoded value with `config.analysis.X ?? FALLBACK`. The fallback ensures backward compatibility if config is missing.
193+
194+
**Tests:** Update integration tests to verify custom config values flow through.
195+
196+
### Phase 3 — Wire risk & community parameters (1 PR)
197+
198+
**Files to change:**
199+
- `src/graph/classifiers/risk.js` → read `config.risk.weights`, `config.risk.roleWeights`, `config.risk.defaultRoleWeight`
200+
- `src/graph/algorithms/louvain.js` → accept `resolution` parameter, default from config
201+
- `src/features/structure.js` → read `config.structure.cohesionThreshold`
202+
203+
**Pattern:** These modules don't currently receive config. Options:
204+
1. **Preferred:** Accept an `options` parameter that callers populate from config
205+
2. **Alternative:** Import `loadConfig` directly (adds coupling but simpler)
206+
207+
**Tests:** Unit tests for risk scoring with custom weights. Integration test for Louvain with custom resolution.
208+
209+
### Phase 4 — Wire search parameters (1 PR)
210+
211+
**Files to change:**
212+
- `src/domain/search/search/hybrid.js` → read `config.search.rrfK`, `config.search.topK`
213+
- `src/domain/search/search/semantic.js` → read `config.search.defaultMinScore`, `config.search.topK` (C3), and `config.search.similarityWarnThreshold` (C5, replaces hardcoded `SIMILARITY_WARN_THRESHOLD`)
214+
- `src/domain/search/models.js` → batch sizes stay hardcoded (moved to Category F — model-specific implementation details)
215+
216+
**Note:** `config.search` already exists with `defaultMinScore`, `rrfK`, `topK`. The modules just don't read from it — they duplicate the values. This phase wires the existing config keys.
217+
218+
### Phase 5 — Wire display & MCP parameters (1 PR)
219+
220+
**Files to change:**
221+
- `src/presentation/result-formatter.js` → read `config.display.maxColWidth`
222+
- `src/shared/file-utils.js` → read `config.display.excerptLines`, `config.display.jsdocEndScanLines` (D4a, 10 lines), `config.display.jsdocOpenScanLines` (D4b, 20 lines — note different default values), `config.display.summaryMaxChars`, `config.display.signatureGatherLines`
223+
- `src/shared/paginate.js` → read `config.mcp.defaults` (`MCP_MAX_LIMIT` stays hardcoded — security boundary)
224+
225+
**Consideration:** `file-utils.js` and `paginate.js` are low-level shared utilities. They shouldn't call `loadConfig()` directly. Instead, pass display/mcp settings down from callers, or use a module-level config cache set at startup.
226+
227+
### Phase 6 — Documentation & migration (1 PR)
228+
229+
1. Update `README.md` configuration section with the full schema
230+
2. Add a `docs/configuration.md` reference with all keys, types, defaults, and descriptions
231+
3. Document the `structure.cohesionThreshold` key and its relationship to A8
232+
4. Add a JSON Schema file (`.codegraphrc.schema.json`) for IDE autocomplete
233+
5. Add a **Configuration** section to `CLAUDE.md` that documents:
234+
- The `.codegraphrc.json` config file and its location
235+
- The full list of configurable sections (`analysis`, `community`, `structure`, `risk`, `display`, `mcp`, `search`, `check`, `coChange`, `manifesto`)
236+
- Key tunable parameters and their defaults (depth limits, risk weights, thresholds)
237+
- How `mergeConfig` works (partial overrides deep-merge with defaults)
238+
- Env var overrides (`CODEGRAPH_LLM_*`)
239+
- Guidance: when adding new behavioral constants, always add them to `DEFAULTS` in `config.js` and wire them through — never introduce new hardcoded magic numbers
240+
241+
---
242+
243+
## Migration & Backward Compatibility
244+
245+
- All new config keys have defaults matching current hardcoded values → **zero breaking changes**
246+
- Existing `.codegraphrc.json` files continue to work unchanged
247+
- `mergeConfig` will be updated to deep-merge recursively (Phase 1 prerequisite), so users only need to specify the keys they want to override
248+
- `build.driftThreshold` stays in place — no migration needed
249+
250+
## Example `.codegraphrc.json` after this work
251+
252+
```json
253+
{
254+
"analysis": {
255+
"fnImpactDepth": 8,
256+
"falsePositiveCallers": 30
257+
},
258+
"risk": {
259+
"weights": {
260+
"complexity": 0.4,
261+
"churn": 0.1
262+
}
263+
},
264+
"community": { "resolution": 1.5 },
265+
"structure": { "cohesionThreshold": 0.25 },
266+
"display": {
267+
"maxColWidth": 60
268+
}
269+
}
270+
```
271+
272+
---
273+
274+
## Estimated Scope
275+
276+
| Phase | Files changed | New tests | Risk |
277+
|-------|--------------|-----------|------|
278+
| 1 — Schema | 2 | 3-4 | Low |
279+
| 2 — Analysis wiring | 6 | 4-5 | Low |
280+
| 3 — Risk/community | 3 | 2-3 | Medium (parameter threading) |
281+
| 4 — Search wiring | 3 | 2 | Low (config keys already exist) |
282+
| 5 — Display/MCP | 3 | 2 | Medium (shared utility coupling) |
283+
| 6 — Docs + CLAUDE.md | 5 | 0 | None |
284+
285+
**Total: ~22 files changed, 6 PRs, one concern per PR.**

0 commit comments

Comments
 (0)