Skip to content

refactor: extract MAX_WALK_DEPTH constant to extractors/helpers.ts#620

Merged
carlos-alm merged 3 commits intomainfrom
refactor/titan-extract-max-walk-depth
Mar 26, 2026
Merged

refactor: extract MAX_WALK_DEPTH constant to extractors/helpers.ts#620
carlos-alm merged 3 commits intomainfrom
refactor/titan-extract-max-walk-depth

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • Extracts duplicated depth >= 200 guard into shared MAX_WALK_DEPTH constant in helpers.ts
  • Addresses DRY violations (Pillar III) flagged across 6 extractor files
  • No behavior change -- purely mechanical extraction

Titan Audit Context

  • Phase: abstraction (Forge Phase 4)
  • Domain: extractors
  • Commits: 1
  • Depends on: none

Changes

  • src/extractors/helpers.ts -- exports MAX_WALK_DEPTH = 200
  • src/extractors/javascript.ts -- imports and uses MAX_WALK_DEPTH
  • src/extractors/python.ts -- imports and uses MAX_WALK_DEPTH
  • src/extractors/go.ts -- imports and uses MAX_WALK_DEPTH
  • src/extractors/rust.ts -- imports and uses MAX_WALK_DEPTH
  • src/extractors/csharp.ts -- imports and uses MAX_WALK_DEPTH
  • src/extractors/php.ts -- imports and uses MAX_WALK_DEPTH

Metrics Impact

  • Blast radius: 22 (gate-validated)
  • Eliminates 6 duplicated magic numbers

Test plan

  • CI passes (lint + build + tests)
  • codegraph check --cycles --boundaries passes
  • No new functions above complexity thresholds

@claude
Copy link
Copy Markdown

claude bot commented Mar 26, 2026

Claude finished @carlos-alm's task —— View job


I'll review this PR according to the structured 5-phase analysis framework.

Review Progress

  • Phase 1: Root Cause Analysis
  • Phase 2: Configuration Change Detection
  • Phase 3: Test Integrity Check
  • Phase 4: Code Quality Review
  • Phase 5: Backlog Compliance Assessment
  • Final Assessment and Rating

Phase 1: Root Cause Analysis ✅

Problem Being Solved: DRY violation - the magic number 200 was hardcoded as depth >= 200 in 6 different extractor files. This creates maintenance debt and inconsistency risks.

Why This Approach: Extracting to a shared constant MAX_WALK_DEPTH in helpers.ts is the correct solution - centralizes the value, makes intent explicit, and ensures consistency across all extractors.

Risk Assessment: Very low risk - purely mechanical refactoring with no behavior change.

Phase 2: Configuration Change Detection ✅

No configuration changes detected. This PR only touches source code in the extractors directory. No changes to:

  • Biome/linting configuration
  • Test timeouts or coverage thresholds
  • Build configuration
  • TypeScript settings

Phase 3: Test Integrity ✅

No test modifications. The refactoring is purely internal constant extraction that maintains identical behavior. All existing tests should continue to pass as the depth limit logic is unchanged.

Phase 4: Code Quality Review ✅

Excellent refactoring quality:

  1. Perfect mechanical extraction: All 6 extractors (javascript.ts, python.ts, go.ts, rust.ts, csharp.ts, php.ts) consistently replaced if (depth >= 200) with if (depth >= MAX_WALK_DEPTH)

  2. Well-documented constant: Added clear JSDoc explaining the purpose - preventing stack overflow on deeply nested ASTs

  3. Consistent import pattern: All files properly import the constant from ./helpers.js

  4. No behavior change: The constant value remains 200, ensuring identical runtime behavior

  5. Logical placement: Added to helpers.ts where other shared extractor utilities live

The implementation is textbook-perfect constant extraction with zero issues found.

Phase 5: Backlog Compliance Assessment ✅

Zero-dep: ✓ - No new runtime dependencies added
Foundation-aligned: ✓ - Maintains existing tree-sitter/TypeScript architecture
Problem-fit: 3 - Useful refactoring but doesn't directly address core "lost AI" problem
Breaking: No - Pure internal refactoring with identical behavior

Tier: 1 (zero-dep ✓ + foundation-aligned ✓)

Final Assessment

## Root Cause Analysis
**Problem Being Solved**: Eliminates DRY violation of hardcoded depth limit (200) across 6 extractor files
**Why This Approach**: Shared constant ensures consistency and makes maintenance easier
**Risk Assessment**: Minimal - mechanical refactoring with no behavior change

## Backlog Compliance
- Zero-dep: ✓ — no new dependencies
- Foundation-aligned: ✓ — builds on existing tree-sitter architecture
- Problem-fit: 3 — useful refactoring, doesn't solve core AI navigation problem
- Breaking: No — identical runtime behavior
- Tier: 1

## Critical Concerns
None. This is exemplary refactoring work.

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: Perfect execution of DRY principle elimination. The constant is well-documented, consistently applied across all extractors, and maintains identical behavior. This reduces maintenance debt and improves code clarity. Only reason it's not 5-star is that it's a routine refactoring rather than exceptional functionality.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR is a clean, purely mechanical DRY refactoring. It extracts the repeated depth >= 200 stack-overflow guard into a single named constant MAX_WALK_DEPTH = 200 exported from src/extractors/helpers.ts, then replaces all six inline occurrences across the language extractor files with a reference to that constant. There is no behavior change.\n\n- helpers.ts gains MAX_WALK_DEPTH = 200 with a clear JSDoc comment explaining its purpose as a Category F safety boundary (stack-overflow guard, not user-tunable).\n- All six extractor files are updated consistently: one import added, one magic number replaced.\n- A grep for remaining depth >= 200 occurrences in the extractors directory returns no results — the extraction is complete and exhaustive.\n- No issues found.

Confidence Score: 5/5

Safe to merge — purely mechanical extraction of a duplicated magic number into a shared constant with zero behavior change.

All six call sites are updated consistently, no depth >= 200 literals remain, the constant is correctly placed in helpers.ts as a Category F boundary (confirmed by prior thread), and the JSDoc comment is accurate. Nothing to block merge.

No files require special attention.

Important Files Changed

Filename Overview
src/extractors/helpers.ts Adds the exported MAX_WALK_DEPTH = 200 constant with a clear JSDoc comment; no other changes to the file.
src/extractors/javascript.ts Imports MAX_WALK_DEPTH from helpers and replaces the lone depth >= 200 guard inside extractTypeMapWalk.
src/extractors/python.ts Imports MAX_WALK_DEPTH from helpers and replaces the depth >= 200 guard in extractPythonTypeMapDepth.
src/extractors/go.ts Imports MAX_WALK_DEPTH from helpers and replaces the depth >= 200 guard in extractGoTypeMapDepth.
src/extractors/rust.ts Imports MAX_WALK_DEPTH from helpers and replaces the depth >= 200 guard in extractRustTypeMapDepth.
src/extractors/csharp.ts Imports MAX_WALK_DEPTH from helpers and replaces the depth >= 200 guard in extractCSharpTypeMapDepth.
src/extractors/php.ts Imports MAX_WALK_DEPTH from helpers and replaces the depth >= 200 guard in extractPhpTypeMapDepth.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    H["src/extractors/helpers.ts\nexport const MAX_WALK_DEPTH = 200"]
    H --> JS["javascript.ts\nextractTypeMapWalk → walk()"]
    H --> PY["python.ts\nextractPythonTypeMapDepth()"]
    H --> GO["go.ts\nextractGoTypeMapDepth()"]
    H --> RS["rust.ts\nextractRustTypeMapDepth()"]
    H --> CS["csharp.ts\nextractCSharpTypeMapDepth()"]
    H --> PHP["php.ts\nextractPhpTypeMapDepth()"]
    JS --> G["depth >= MAX_WALK_DEPTH → return"]
    PY --> G
    GO --> G
    RS --> G
    CS --> G
    PHP --> G
Loading

Reviews (2): Last reviewed commit: "refactor: extract MAX_WALK_DEPTH constan..." | Re-trigger Greptile

* Maximum recursion depth for tree-sitter AST walkers.
* Shared across all language extractors to prevent stack overflow on deeply nested ASTs.
*/
export const MAX_WALK_DEPTH = 200;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Placement vs. DEFAULTS in config.ts

CLAUDE.md says: "All tunable behavioral constants... never introduce new hardcoded magic numbers in individual modules. Category F values (safety boundaries, standard formulas, platform concerns) are the only exception."

A stack-overflow guard that prevents the process from crashing does read as a Category F safety boundary, so placing it here is defensible. However, 200 is also a value a power-user might reasonably want to override for unusually deep ASTs (e.g. generated code, machine-generated PHP templates). If it ever needs to be configurable, it would have to be pulled out of helpers.ts and into DEFAULTS at that point anyway.

Worth a quick team decision: is this intentionally Category F (hard-coded, never user-overridable), or should it live in DEFAULTS under, say, analysis.maxWalkDepth from the start?

Context Used: CLAUDE.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. This is intentionally Category F — it's a stack-overflow safety guard, not a user-tunable behavior knob. A value of 200 is already far beyond any realistic hand-written AST nesting depth; if someone hits it, the issue is the input (e.g. generated code) rather than the configuration. If a future need arises to make it configurable, promoting it to \ at that point is straightforward. For now, keeping it in \ with the other extractor utilities is the right home.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to formatting above: promoting it to DEFAULTS.analysis.maxWalkDepth would be straightforward if needed. For now, helpers.ts is the right home as a Category F safety boundary.

@carlos-alm
Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm carlos-alm merged commit 5bbccbe into main Mar 26, 2026
12 checks passed
@carlos-alm carlos-alm deleted the refactor/titan-extract-max-walk-depth branch March 26, 2026 07:53
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant