refactor: adopt dead helpers across codebase#895
Conversation
Wire up extracted helpers from Titan runs that existed but were never consumed, reducing boilerplate and improving error specificity. - Adopt named_child_text across 27 sites in 11 Rust extractors - Migrate cpp.rs from hand-rolled find_cpp_parent_class to find_enclosing_type_name - Add toSymbolRef helper in shared/normalize.ts, adopt at 15 mapping sites - Wire ParseError in parser.ts for structured PARSE_FAILED error codes - Wire BoundaryError in boundaries.ts to distinguish config/DB failures from clean results - Add --modules/--threshold flags to codegraph structure command - Wire batchQuery in CLI batch command, removing duplicated routing logic - Route detect-changes pending analysis through unified runAnalyses engine
|
Claude finished @carlos-alm's task —— View job Code Review AnalysisReviewing PR #895: "refactor: adopt dead helpers across codebase" Todo List:
Root Cause AnalysisProblem Being Solved: This PR addresses code quality debt by adopting previously unused helper functions across the codebase. The helpers ( Why This Approach: The approach is well-justified - rather than removing the helpers, the PR completes their adoption, reducing code duplication and standardizing patterns across extractors. This follows the principle of DRY (Don't Repeat Yourself) and improves maintainability. Risk Assessment: LOW RISK - The changes are primarily mechanical refactoring that replaces inline boilerplate with existing tested helpers. The error-throwing changes ( Backlog Compliance
Phase 2: Configuration Changes ✓No high-risk configuration changes detected:
Phase 3: Test Integrity ✓Tests are strengthened, not weakened:
Phase 4: Code Quality ✓Rust Extractor Changes (
TypeScript Utilities (
Error Handling Improvements:
CLI Enhancements:
Potential Issues: None identified
Critical ConcernsNone. This is a well-executed technical debt cleanup. Final Recommendation
Minor suggestions for future PRs:
|
Greptile SummaryThis PR completes the adoption of previously-dead helpers across the codebase: Confidence Score: 5/5Safe to merge — all changes are mechanical dead-helper adoption with no behavioral regressions; the only open finding is a P2 range check suggestion. All 28 changed files are refactors of existing, previously-dead helpers. The previously-flagged src/cli/commands/structure.ts — minor threshold range validation gap (P2)
|
| Filename | Overview |
|---|---|
| src/shared/normalize.ts | Adds toSymbolRef utility — a simple 4-field picker that replaces inline object literals at 15 call sites. Clean addition with no risk. |
| src/cli/commands/batch.ts | Removes the local isMultiBatch type guard and MultiBatchItem interface in favor of the equivalent logic already present in batchQuery; routing is identical. |
| src/cli/commands/structure.ts | Wires --modules/--threshold flags calling the pre-existing moduleBoundariesData/formatModuleBoundaries; NaN is checked but threshold range [0,1] is not validated. |
| src/features/boundaries.ts | Changes silent debug+return to throw BoundaryError for invalid config and DB failures; callers updated accordingly with try/catch surfacing warn status. |
| src/features/manifesto.ts | Wraps evaluateBoundaries in try/catch and reports status: 'warn' on error (previously 'pass'), fixing the previously-flagged silent-pass issue. |
| src/domain/graph/builder/stages/detect-changes.ts | Replaces two separate conditional imports with a single runAnalyses call using explicit flags; behavior is equivalent. |
| src/domain/parser.ts | Wraps bare re-throw in a structured ParseError with file and cause for required-parser failures; non-required parsers still warn-and-skip. |
| src/domain/analysis/dependencies.ts | Adopts toSymbolRef for callees/candidates; deliberately skips it for callers (extra viaHierarchy field) — correctly commented. |
| crates/codegraph-core/src/extractors/cpp.rs | Removes hand-rolled find_cpp_parent_class (14 lines) and replaces its single call site with find_enclosing_type_name; semantically identical. |
| crates/codegraph-core/src/extractors/helpers.rs | find_enclosing_type_name and extract_constructor_name/extract_call_name now use named_child_text internally; logic is equivalent. |
| tests/unit/boundaries.test.ts | Test updated to expect BoundaryError throw instead of empty-result return on invalid config; correctly reflects the new behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Rust ["Rust extractors (11 files)"]
R1["named_child_text\nreplaces child_by_field_name boilerplate\n(27 sites)"]
R2["find_enclosing_type_name\nreplaces find_cpp_parent_class\n(cpp.rs)"]
end
subgraph TS ["TypeScript domain/features"]
T1["toSymbolRef\nreplaces inline {name,kind,file,line}\n(15 sites, 7 files)"]
T2["ParseError structured throw\n(parser.ts doLoadLanguage)"]
T3["BoundaryError throw\n(boundaries.ts)"]
T4["try/catch → status: warn\n(manifesto.ts)"]
T5["batchQuery\nreplaces isMultiBatch + dual-path\n(batch.ts)"]
T6["runAnalyses\nreplaces buildCFGData + buildDataflowEdges\n(detect-changes.ts)"]
T7["--modules / --threshold\nwires moduleBoundariesData\n(structure.ts)"]
end
T3 -->|"throws"| T4
T1 --> normalize["shared/normalize.ts"]
R1 --> helpers["helpers.rs"]
R2 --> helpers
Reviews (2): Last reviewed commit: "fix: address review feedback on dead hel..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } catch (e: unknown) { | ||
| debug(`boundary check failed: ${(e as Error).message}`); | ||
| ruleResults.push({ | ||
| name: 'boundaries', | ||
| level: 'graph', | ||
| status: 'pass', | ||
| thresholds: effectiveThresholds, | ||
| violationCount: 0, | ||
| }); | ||
| return; |
There was a problem hiding this comment.
pass
When evaluateBoundaries throws a BoundaryError because the boundary config is invalid (e.g. empty modules, unknown rule target), this catch block pushes status: 'pass' — the user's CI gate will green-light a run that never actually checked boundaries. A 'warn' status (or a re-thrown error when the cause is a config error rather than a DB error) would surface the misconfiguration without breaking unrelated checks.
| } catch (e: unknown) { | |
| debug(`boundary check failed: ${(e as Error).message}`); | |
| ruleResults.push({ | |
| name: 'boundaries', | |
| level: 'graph', | |
| status: 'pass', | |
| thresholds: effectiveThresholds, | |
| violationCount: 0, | |
| }); | |
| return; | |
| } catch (e: unknown) { | |
| debug(`boundary check failed: ${(e as Error).message}`); | |
| ruleResults.push({ | |
| name: 'boundaries', | |
| level: 'graph', | |
| status: 'warn', | |
| thresholds: effectiveThresholds, | |
| violationCount: 0, | |
| }); | |
| return; | |
| } |
There was a problem hiding this comment.
Fixed in d6e25a6 -- the catch block now reports status: 'warn' instead of 'pass', so invalid boundary configs surface in the manifesto output rather than silently passing.
Codegraph Impact Analysis40 functions changed → 110 callers affected across 48 files
|
- manifesto.ts: report 'warn' instead of 'pass' when boundary check throws - structure.ts: validate --threshold flag rejects non-numeric input - dependencies.ts: clarify intentional skip of toSymbolRef for callers
|
Addressed all three Greptile findings in d6e25a6:
|
Forge extracts helpers but never completes the adoption loop — dead symbol count inflates with every run. Grind closes the gap by finding dead helpers from forge, classifying them (adopt/re-export/promote/ false-positive/remove), wiring them into consumers, and gating on a non-positive dead-symbol delta. Pipeline is now: recon → gauntlet → sync → forge → grind → close
- Track currentTarget, processedTargets, failedTargets in state for mid-run resume after interruption - Persist grind classifications to grind-targets.ndjson (append-only) so re-runs skip already-analyzed targets - Write titan-state.json after every target, not just at phase end - Add interrupted-mid-target recovery logic in edge cases - Use codegraph audit/context/fn-impact/where/query/ast before edits - Add codegraph diff-impact --staged before commits - Add codegraph build after edits to keep graph current - Add --target flag for retrying individual failures
1 participant
Summary
Code adoptions (28 files, -30 net lines)
named_child_textacross 27 sites in 11 Rust extractors — replacesnode.child_by_field_name("X").map(|n| node_text(&n, source))boilerplatecpp.rsfrom hand-rolledfind_cpp_parent_classto existingfind_enclosing_type_namehelper (-14 lines)toSymbolRefutility inshared/normalize.ts, adopt at 15 inline{ name, kind, file, line }mapping sites across 7 filesParseErrorinparser.ts—doLoadLanguagenow throws structuredPARSE_FAILEDerrors withfileandcauseinstead of bare re-throwsBoundaryErrorinboundaries.ts— invalid config and DB failures now throw instead of silently returning empty results; callers (manifesto.ts,diff-impact.ts) handle gracefully--modules/--thresholdflags tocodegraph structurecommand, wiring existing but unusedmoduleBoundariesData+formatModuleBoundariesbatchQueryin CLI batch command, removing duplicated multi-batch routing logicdetect-changespending analysis path through unifiedrunAnalysesengine instead of separatebuildCFGData/buildDataflowEdgescallsNew
/titan-grindskill (Phase 4.5).claude/skills/titan-grind/SKILL.md— closes the adoption loop that forge leaves open. After forge extracts helpers, grind finds the dead ones, classifies them (adopt/re-export/promote/false-positive/remove), wires them into consumers, and gates on dead-symbol delta.claude/skills/titan-run/SKILL.md— pipeline is nowrecon → gauntlet → sync → forge → grind → close. Added Step 4.5 with grind loop, V16-V19 validations,--start-from grindsupport, and forge artifact pre-validationWhy
Titan runs (v3.3.1–v3.8.0) extracted helpers that were never consumed, inflating dead symbols from 8,259 → 11,647 across three runs. This PR does the manual adoption work and adds the
/titan-grindphase so future runs close the loop automatically.Test plan
boundaries.test.tsupdated for newBoundaryErrorthrow behavior (32/32 pass)tsc)codegraph structure --modulesproduces expected output on a real repocodegraph batchstill works for both single and multi-batch targets/titan-grind --dry-runon a post-forge branch to validate the skill