docs: plan to centralize hardcoded configuration #482
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,270 @@ | ||||||||
| # Plan: Centralize Hardcoded Configuration | ||||||||
|
|
||||||||
| > **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`). | ||||||||
|
|
||||||||
| ## Problem | ||||||||
|
|
||||||||
| 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. | ||||||||
|
|
||||||||
| --- | ||||||||
|
|
||||||||
| ## Inventory of Hardcoded Values | ||||||||
|
|
||||||||
| ### Category A — Analysis Parameters (high user value) | ||||||||
|
|
||||||||
| | # | Value | File | Line | Controls | | ||||||||
| |---|-------|------|------|----------| | ||||||||
| | A1 | `maxDepth = 5` | `domain/analysis/impact.js` | 111 | `fn-impact` transitive caller depth | | ||||||||
| | A2 | `maxDepth = 3` | `domain/analysis/impact.js` | 31, 144 | BFS default depth for impact/diff-impact | | ||||||||
| | A3 | `maxDepth = 3` | `features/audit.js` | 102 | Audit blast-radius depth | | ||||||||
| | A4 | `maxDepth = 3` | `features/check.js` | 220 | CI check blast-radius depth | | ||||||||
| | A5 | `maxDepth = 10` | `features/sequence.js` | 91 | Sequence diagram traversal depth | | ||||||||
| | A6 | `FALSE_POSITIVE_CALLER_THRESHOLD = 20` | `domain/analysis/module-map.js` | 37 | Generic function false-positive filter | | ||||||||
| | A7 | `resolution = 1.0` | `graph/algorithms/louvain.js` | 17 | Louvain community detection granularity | | ||||||||
| | A8 | `driftThreshold = 0.3` | `features/structure.js` | 581 | Structure cohesion drift warning | | ||||||||
| | A9 | `maxCallers >= 10` | `domain/analysis/brief.js` | 38 | `brief` high-risk tier threshold | | ||||||||
| | A10 | `maxCallers >= 3` | `domain/analysis/brief.js` | 39 | `brief` medium-risk tier threshold | | ||||||||
| | A11 | `maxDepth = 5` | `domain/analysis/brief.js` | 47 | `brief` transitive caller BFS depth | | ||||||||
| | A12 | `maxDepth = 5` | `domain/analysis/brief.js` | 73 | `brief` transitive importer BFS depth | | ||||||||
|
|
||||||||
| ### Category B — Risk & Scoring Weights (medium-high user value) | ||||||||
|
|
||||||||
| | # | Value | File | Line | Controls | | ||||||||
| |---|-------|------|------|----------| | ||||||||
| | 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 | | ||||||||
| | 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 | | ||||||||
| | B3 | `DEFAULT_ROLE_WEIGHT = 0.5` | `graph/classifiers/risk.js` | 30 | Fallback role weight | | ||||||||
|
|
||||||||
| ### Category C — Search & Embedding (already partially in config) | ||||||||
|
|
||||||||
| | # | Value | File | Line | Controls | | ||||||||
| |---|-------|------|------|----------| | ||||||||
| | C1 | `limit = 15` | `domain/search/search/hybrid.js` | 12 | Hybrid search default limit | | ||||||||
| | C2 | `rrfK = 60` | `domain/search/search/hybrid.js` | 13 | RRF fusion constant | | ||||||||
| | C3 | `limit = 15` | `domain/search/search/semantic.js` | 12 | Semantic search default limit | | ||||||||
| | C4 | `minScore = 0.2` | `domain/search/search/semantic.js` | 13, 52 | Minimum similarity threshold | | ||||||||
| | C5 | `SIMILARITY_WARN_THRESHOLD = 0.85` | `domain/search/search/semantic.js` | 71 | Duplicate query warning | | ||||||||
| | C6 | Batch sizes per model | `domain/search/models.js` | 66-75 | Embedding batch sizes | | ||||||||
|
Contributor
There was a problem hiding this comment.
This means C6 will remain hardcoded after all six phases complete, contrary to the plan's goal of eliminating all magic numbers from the inventory. Every other entry in Category C has a concrete key assigned. There are two clean resolutions:
Either way, the ambiguity should be resolved so Phase 4 implementers don't discover an undesigned item mid-PR.
Contributor
Author
There was a problem hiding this comment. Fixed — moved C6 (embedding batch sizes) to Category F with explicit rationale: model-specific implementation details rarely tuned by end-users, analogous to watcher debounce (F3). Removed ambiguous "could be" wording from Phase 4 and updated models.js entry to state batch sizes stay hardcoded. |
||||||||
|
|
||||||||
| ### Category D — Display & Truncation (low-medium user value) | ||||||||
|
|
||||||||
| | # | Value | File | Line | Controls | | ||||||||
| |---|-------|------|------|----------| | ||||||||
| | D1 | `MAX_COL_WIDTH = 40` | `presentation/result-formatter.js` | 82 | Table column width | | ||||||||
| | D2 | `50 lines` | `shared/file-utils.js` | 23 | Source context excerpt length | | ||||||||
| | D3 | `100 chars` | `shared/file-utils.js` | 48, 63 | Summary/docstring truncation | | ||||||||
| | D4 | `10 / 20 lines` | `shared/file-utils.js` | 36, 54 | JSDoc scan depth | | ||||||||
|
Contributor
There was a problem hiding this comment.
The plan records D4 as
If an implementer in Phase 5 wires Consider splitting into two keys: display: {
jsdocEndScanLines: 10, // D4a: lines to scan for block-end marker
jsdocOpenScanLines: 20, // D4b: lines to scan for /** opening
...
}Or, at minimum, note in Phase 5 that line 54 uses
Contributor
Author
There was a problem hiding this comment. Fixed — split D4 into two distinct entries: D4a ( |
||||||||
| | D5 | `5 lines` | `shared/file-utils.js` | 76 | Multi-line signature gather | | ||||||||
|
|
||||||||
| ### Category E — MCP Pagination (medium user value) | ||||||||
|
|
||||||||
| | # | Value | File | Line | Controls | | ||||||||
| |---|-------|------|------|----------| | ||||||||
| | E1 | `MCP_DEFAULTS` (22 entries) | `shared/paginate.js` | 9-34 | Per-tool default page sizes | | ||||||||
| | ~~E2~~ | ~~`MCP_MAX_LIMIT = 1000`~~ | — | — | Moved to Category F (see below) | | ||||||||
|
|
||||||||
| ### Category F — Infrastructure (low user value, keep hardcoded) | ||||||||
|
|
||||||||
| | # | Value | File | Line | Controls | | ||||||||
| |---|-------|------|------|----------| | ||||||||
| | F1 | `CACHE_TTL_MS = 86400000` | `infrastructure/update-check.js` | 10 | Version check cache (24h) | | ||||||||
| | F2 | `FETCH_TIMEOUT_MS = 3000` | `infrastructure/update-check.js` | 11 | Version check HTTP timeout | | ||||||||
| | F3 | `debounce = 300` | `domain/graph/watcher.js` | 80 | File watcher debounce (ms) | | ||||||||
| | F4 | `maxBuffer = 10MB` | `features/check.js` | 260 | Git diff buffer | | ||||||||
| | F5 | `volume / 3000` | `features/complexity.js` | 85 | Halstead bugs formula (standard) | | ||||||||
| | F6 | `timeout = 10_000` | `infrastructure/config.js` | 110 | apiKeyCommand timeout | | ||||||||
| | F7 | `MCP_MAX_LIMIT = 1000` | `shared/paginate.js` | 37 | Hard abuse-prevention cap — server-side safety boundary, not a tuning knob | | ||||||||
|
|
||||||||
| --- | ||||||||
|
|
||||||||
| ## Design | ||||||||
|
|
||||||||
| ### Proposed `DEFAULTS` additions in `src/infrastructure/config.js` | ||||||||
|
|
||||||||
| ```js | ||||||||
| export const DEFAULTS = { | ||||||||
| // ... existing fields ... | ||||||||
|
|
||||||||
| analysis: { | ||||||||
| defaultDepth: 3, // A2: BFS depth for impact/diff-impact | ||||||||
|
Contributor
There was a problem hiding this comment.
The key A user who wants to raise blast-radius depth across the board would be surprised to find only Consider renaming to
Suggested change
The Phase 2 wiring instructions would need the same update (
Contributor
Author
There was a problem hiding this comment. Fixed — renamed |
||||||||
| fnImpactDepth: 5, // A1: fn-impact transitive depth | ||||||||
| auditDepth: 3, // A3: audit blast-radius depth | ||||||||
| sequenceDepth: 10, // A5: sequence diagram depth | ||||||||
| falsePositiveCallers: 20, // A6: generic function filter threshold | ||||||||
| briefCallerDepth: 5, // A11: brief transitive caller BFS depth | ||||||||
| briefImporterDepth: 5, // A12: brief transitive importer BFS depth | ||||||||
| briefHighRiskCallers: 10, // A9: brief high-risk tier threshold | ||||||||
| briefMediumRiskCallers: 3, // A10: brief medium-risk tier threshold | ||||||||
|
Comment on lines
+98
to
+103
Contributor
There was a problem hiding this comment.
The existing const driftThreshold = config.build?.driftThreshold ?? 0.2;
if (nodeDrift > driftThreshold || edgeDrift > driftThreshold) { ... }This threshold asks "has the graph structure changed significantly between builds?" — it's a build-pipeline concern, not a community-detection concern. Moving it to
Consider:
Contributor
Author
There was a problem hiding this comment. Fixed — kept |
||||||||
| }, | ||||||||
|
|
||||||||
| community: { | ||||||||
| resolution: 1.0, // A7: Louvain resolution (only Louvain params here) | ||||||||
| }, | ||||||||
|
|
||||||||
| // build.driftThreshold stays in `build` (already wired in finalize.js line 52) | ||||||||
| // — it's a build-pipeline concern, not community detection | ||||||||
|
|
||||||||
| structure: { | ||||||||
| cohesionThreshold: 0.3, // A8: structure cohesion drift warning | ||||||||
| }, | ||||||||
|
|
||||||||
| risk: { | ||||||||
| weights: { // B1 | ||||||||
| fanIn: 0.25, | ||||||||
| complexity: 0.3, | ||||||||
| churn: 0.2, | ||||||||
| role: 0.15, | ||||||||
| mi: 0.1, | ||||||||
| }, | ||||||||
| roleWeights: { // B2 | ||||||||
| core: 1.0, | ||||||||
| utility: 0.9, | ||||||||
| entry: 0.8, | ||||||||
| adapter: 0.5, | ||||||||
| leaf: 0.2, | ||||||||
| dead: 0.1, | ||||||||
| }, | ||||||||
| defaultRoleWeight: 0.5, // B3 | ||||||||
| }, | ||||||||
|
Comment on lines
+117
to
+134
Contributor
There was a problem hiding this comment.
The existing // user config:
{ "risk": { "weights": { "complexity": 0.4, "churn": 0.1 } } }
// result after mergeConfig:
{ risk: { weights: { complexity: 0.4, churn: 0.1 }, // ← fanIn, role, mi are GONE
roleWeights: { ... }, defaultRoleWeight: 0.5 } }The dropped weights default to The Example Phase 1 must include making
Contributor
Author
There was a problem hiding this comment. Fixed — upgraded the mergeConfig note from "may need recursive merge" to a hard Phase 1 prerequisite. The plan now explicitly states that the current 1-level shallow merge will silently drop sibling keys on partial nested overrides, producing NaN risk scores, and that this must be fixed before any nested config keys are wired. |
||||||||
|
|
||||||||
| display: { | ||||||||
| maxColWidth: 40, // D1 | ||||||||
| excerptLines: 50, // D2 | ||||||||
| summaryMaxChars: 100, // D3 | ||||||||
| jsdocScanLines: 10, // D4 | ||||||||
| signatureGatherLines: 5, // D5 | ||||||||
| }, | ||||||||
|
|
||||||||
| mcp: { | ||||||||
| defaults: { /* E1: current MCP_DEFAULTS object */ }, | ||||||||
| // MCP_MAX_LIMIT stays hardcoded (Category F) — server-side safety boundary | ||||||||
| }, | ||||||||
|
Comment on lines
+151
to
+154
Contributor
There was a problem hiding this comment.
Unlike the other constants in the plan (thresholds, depths, weights), this cap is a server-side safety boundary, not a tuning knob. Consider keeping
Contributor
Author
There was a problem hiding this comment. Fixed — moved MCP_MAX_LIMIT to Category F (keep hardcoded). Removed it from the proposed DEFAULTS schema, updated the Phase 5 file list to note it stays hardcoded, and added it to the "What stays hardcoded" section with explanation: it's a server-side safety boundary, not a tuning knob. |
||||||||
| }; | ||||||||
| ``` | ||||||||
|
|
||||||||
| ### What stays hardcoded (Category F) | ||||||||
|
|
||||||||
| - **Halstead `volume / 3000`** — industry-standard formula, not a tuning knob | ||||||||
| - **Git `maxBuffer`** — platform concern, not analysis behavior | ||||||||
| - **`apiKeyCommand` timeout** — security boundary, not user-facing | ||||||||
| - **Update check TTL/timeout** — implementation detail | ||||||||
| - **Watcher debounce** — could be configurable later but low priority | ||||||||
| - **`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 | ||||||||
|
|
||||||||
| --- | ||||||||
|
|
||||||||
| ## Implementation Plan | ||||||||
|
|
||||||||
| ### Phase 1 — Extend DEFAULTS schema (1 PR) | ||||||||
|
|
||||||||
| **Files:** `src/infrastructure/config.js`, `tests/unit/config.test.js` | ||||||||
|
|
||||||||
| 1. Add `analysis`, `community`, `structure`, `risk`, `display`, `mcp` sections to `DEFAULTS` | ||||||||
| 2. Keep `build.driftThreshold` where it is (already wired in `finalize.js` — no migration needed) | ||||||||
| 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 | ||||||||
| 4. Add tests: loading config with overrides for each new section | ||||||||
|
|
||||||||
| ### Phase 2 — Wire analysis parameters (1 PR) | ||||||||
|
|
||||||||
| **Files to change:** | ||||||||
| - `src/domain/analysis/impact.js` → read `config.analysis.defaultDepth` / `config.analysis.fnImpactDepth` | ||||||||
| - `src/features/audit.js` → read `config.analysis.auditDepth` | ||||||||
| - `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.defaultDepth`) | ||||||||
| - `src/features/sequence.js` → read `config.analysis.sequenceDepth` | ||||||||
| - `src/domain/analysis/module-map.js` → read `config.analysis.falsePositiveCallers` | ||||||||
| - `src/domain/analysis/brief.js` → read `config.analysis.briefCallerDepth`, `config.analysis.briefImporterDepth`, `config.analysis.briefHighRiskCallers`, `config.analysis.briefMediumRiskCallers` (PR #480) | ||||||||
|
|
||||||||
| **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. | ||||||||
|
|
||||||||
| **Tests:** Update integration tests to verify custom config values flow through. | ||||||||
|
|
||||||||
| ### Phase 3 — Wire risk & community parameters (1 PR) | ||||||||
|
|
||||||||
| **Files to change:** | ||||||||
| - `src/graph/classifiers/risk.js` → read `config.risk.weights`, `config.risk.roleWeights`, `config.risk.defaultRoleWeight` | ||||||||
| - `src/graph/algorithms/louvain.js` → accept `resolution` parameter, default from config | ||||||||
| - `src/features/structure.js` → read `config.structure.cohesionThreshold` | ||||||||
|
|
||||||||
| **Pattern:** These modules don't currently receive config. Options: | ||||||||
| 1. **Preferred:** Accept an `options` parameter that callers populate from config | ||||||||
| 2. **Alternative:** Import `loadConfig` directly (adds coupling but simpler) | ||||||||
|
|
||||||||
| **Tests:** Unit tests for risk scoring with custom weights. Integration test for Louvain with custom resolution. | ||||||||
|
|
||||||||
|
Comment on lines
+207
to
+208
Contributor
There was a problem hiding this comment.
C3 ( After all six phases complete, C3 remains hardcoded, contrary to the plan's stated goal of eliminating every inventory item. An implementer following Phase 4 line-by-line would wire
Suggested change
Contributor
Author
There was a problem hiding this comment. Fixed — added config.search.topK (C3) to the semantic.js wiring instruction in Phase 4. The line now reads: read config.search.defaultMinScore, config.search.topK (C3), and config.search.similarityWarnThreshold (C5). |
||||||||
| ### Phase 4 — Wire search parameters (1 PR) | ||||||||
|
|
||||||||
| **Files to change:** | ||||||||
| - `src/domain/search/search/hybrid.js` → read `config.search.rrfK`, `config.search.topK` | ||||||||
| - `src/domain/search/search/semantic.js` → read `config.search.defaultMinScore` | ||||||||
| - `src/domain/search/models.js` → batch sizes could be config-overridable per model | ||||||||
|
|
||||||||
| **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. | ||||||||
|
|
||||||||
| ### Phase 5 — Wire display & MCP parameters (1 PR) | ||||||||
|
|
||||||||
| **Files to change:** | ||||||||
| - `src/presentation/result-formatter.js` → read `config.display.maxColWidth` | ||||||||
| - `src/shared/file-utils.js` → read `config.display.excerptLines`, etc. | ||||||||
| - `src/shared/paginate.js` → read `config.mcp.defaults` (`MCP_MAX_LIMIT` stays hardcoded — security boundary) | ||||||||
|
|
||||||||
| **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. | ||||||||
|
|
||||||||
| ### Phase 6 — Documentation & migration (1 PR) | ||||||||
|
|
||||||||
| 1. Update `README.md` configuration section with the full schema | ||||||||
| 2. Add a `docs/configuration.md` reference with all keys, types, defaults, and descriptions | ||||||||
|
Contributor
There was a problem hiding this comment.
Phase 6, item 5 instructs adding a CLAUDE.md Configuration section documenting "the full list of configurable sections" — but the enumerated list (
Suggested change
Contributor
Author
There was a problem hiding this comment. Fixed — added structure to the Phase 6 configurable sections list. It now reads: analysis, community, structure, risk, display, mcp, search, check, coChange, manifesto. |
||||||||
| 3. Document the `structure.cohesionThreshold` key and its relationship to A8 | ||||||||
| 4. Add a JSON Schema file (`.codegraphrc.schema.json`) for IDE autocomplete | ||||||||
| 5. Add a **Configuration** section to `CLAUDE.md` that documents: | ||||||||
| - The `.codegraphrc.json` config file and its location | ||||||||
| - The full list of configurable sections (`analysis`, `community`, `risk`, `display`, `mcp`, `search`, `check`, `coChange`, `manifesto`) | ||||||||
| - Key tunable parameters and their defaults (depth limits, risk weights, thresholds) | ||||||||
| - How `mergeConfig` works (partial overrides deep-merge with defaults) | ||||||||
| - Env var overrides (`CODEGRAPH_LLM_*`) | ||||||||
| - Guidance: when adding new behavioral constants, always add them to `DEFAULTS` in `config.js` and wire them through — never introduce new hardcoded magic numbers | ||||||||
|
|
||||||||
| --- | ||||||||
|
|
||||||||
| ## Migration & Backward Compatibility | ||||||||
|
|
||||||||
| - All new config keys have defaults matching current hardcoded values → **zero breaking changes** | ||||||||
| - Existing `.codegraphrc.json` files continue to work unchanged | ||||||||
| - `mergeConfig` will be updated to deep-merge recursively (Phase 1 prerequisite), so users only need to specify the keys they want to override | ||||||||
| - `build.driftThreshold` stays in place — no migration needed | ||||||||
|
|
||||||||
| ## Example `.codegraphrc.json` after this work | ||||||||
|
|
||||||||
| ```json | ||||||||
| { | ||||||||
| "analysis": { | ||||||||
| "fnImpactDepth": 8, | ||||||||
| "falsePositiveCallers": 30 | ||||||||
| }, | ||||||||
| "risk": { | ||||||||
| "weights": { | ||||||||
| "complexity": 0.4, | ||||||||
| "churn": 0.1 | ||||||||
| } | ||||||||
| }, | ||||||||
| "community": { "resolution": 1.5 }, | ||||||||
| "structure": { "cohesionThreshold": 0.25 }, | ||||||||
| "display": { | ||||||||
| "maxColWidth": 60 | ||||||||
| } | ||||||||
| } | ||||||||
| ``` | ||||||||
|
|
||||||||
| --- | ||||||||
|
|
||||||||
| ## Estimated Scope | ||||||||
|
|
||||||||
| | Phase | Files changed | New tests | Risk | | ||||||||
| |-------|--------------|-----------|------| | ||||||||
| | 1 — Schema | 2 | 3-4 | Low | | ||||||||
| | 2 — Analysis wiring | 6 | 4-5 | Low | | ||||||||
| | 3 — Risk/community | 3 | 2-3 | Medium (parameter threading) | | ||||||||
| | 4 — Search wiring | 3 | 2 | Low (config keys already exist) | | ||||||||
| | 5 — Display/MCP | 3 | 2 | Medium (shared utility coupling) | | ||||||||
| | 6 — Docs + CLAUDE.md | 5 | 0 | None | | ||||||||
|
|
||||||||
| **Total: ~22 files changed, 6 PRs, one concern per PR.** | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIMILARITY_WARN_THRESHOLDhas no config key and no wiring phaseSIMILARITY_WARN_THRESHOLD = 0.85atsrc/domain/search/search/semantic.js:71is included in the Category C inventory (C5), but it is absent from the proposedDEFAULTSschema additions and from the Phase 4 file list. Phase 4 only says "wire existing config keys" — there is no existingconfig.search.similarityWarnThresholdkey to wire, so C5 would remain hardcoded after all six phases complete, contrary to the plan's stated goal.A new key should be added to the DEFAULTS design block and wired in Phase 4:
Phase 4 should be updated to mention
src/domain/search/search/semantic.js→ replaceSIMILARITY_WARN_THRESHOLDwithconfig.search.similarityWarnThreshold.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed — added
search.similarityWarnThreshold: 0.85to the proposed DEFAULTS schema, and updated Phase 4 to explicitly wire C5 by replacing the hardcodedSIMILARITY_WARN_THRESHOLDin semantic.js withconfig.search.similarityWarnThreshold.