Skip to content

fix: break circular dependency cycle and remove dead queryName export#378

Merged
carlos-alm merged 28 commits intomainfrom
fix/break-cycle-remove-dead-export
Mar 9, 2026
Merged

fix: break circular dependency cycle and remove dead queryName export#378
carlos-alm merged 28 commits intomainfrom
fix/break-cycle-remove-dead-export

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Extract isTestFile into src/test-utils.js to break the 5-file circular dependency: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js
  • Remove unused queryName display function from queries.js (CLI uses fnDeps instead; not exported from index.js)
  • Fix dead-export hook to count test-file consumers (removes false positives for test-only exports like clearCodeownersCache)

Test plan

  • node src/cli.js cycles -T → "No circular dependencies detected"
  • All 1502 tests pass (3 pre-existing failures in Ruby CFG, unrelated)
  • Lint clean (1 pre-existing warning for unused rolesData import, unrelated)
  • isTestFile re-exported from queries.js for backward compat — no import changes needed for non-cycle consumers

Before calling ensureWasmTrees, check whether native engine already
supplies CFG and dataflow data for all files. When it does, skip the
WASM pre-parse entirely — avoiding a full WASM parse of every file
on native builds where the data is already available.

Impact: 1 functions changed, 14 affected
Impact: 1 functions changed, 0 affected
d.cfg?.blocks?.length evaluates to 0 (falsy) when the native engine
returns cfg: { blocks: [] } for trivial functions, spuriously triggering
needsWasmTrees. Use Array.isArray(d.cfg?.blocks) instead, and preserve
d.cfg === null for nodes that intentionally have no CFG (e.g. interface
members).

Impact: 1 functions changed, 1 affected
Impact: 1 functions changed, 0 affected
Generate inter-module call sequence diagrams from the graph DB.
Participants are files (not functions), keeping diagrams readable.
BFS forward from entry point, with optional --dataflow flag to
annotate parameter names and return arrows.

New: src/sequence.js, tests/integration/sequence.test.js
Modified: cli.js, mcp.js, index.js, CLAUDE.md, mcp.test.js

Impact: 8 functions changed, 6 affected
…escape, pagination

- Fix buildAliases join('/') producing Mermaid-invalid participant IDs;
  use join('_') and strip '/' from allowed regex chars
- Add Mermaid format validation to alias collision test
- Fix truncated false-positive when nextFrontier has unvisited nodes at
  maxDepth boundary
- Add angle bracket escaping to escapeMermaid (#gt;/#lt; entities)
- Filter orphaned participants after pagination slices messages
- Fix misleading test comment (4 files → 5 files)

Impact: 1 functions changed, 2 affected
# Conflicts:
#	CHANGELOG.md

Impact: 77 functions changed, 108 affected
…tion, add dataflow tests

- Export findMatchingNodes from queries.js; remove duplicated findBestMatch
  from flow.js and sequence.js, replacing with findMatchingNodes(...)[0] ?? null
- Guard sequenceToMermaid truncation note on participants.length > 0
  to prevent invalid "note right of undefined:" in Mermaid output
- Add dataflow annotation test suite with dedicated fixture DB: covers
  return arrows, parameter annotations, disabled-dataflow path, and
  Mermaid dashed arrow output

Impact: 4 functions changed, 27 affected
Add two new PreToolUse hooks that block git commits when issues are
detected:

- check-cycles.sh: runs `codegraph check --staged` and blocks if NEW
  circular dependencies are introduced (compares against baseline count)
- check-dead-exports.sh: checks staged src/ files for newly added
  exports with zero consumers (diff-aware, ignores pre-existing dead
  exports)

Also wire the --unused flag on the exports CLI command, adding
totalUnused to all output formats.

Impact: 4 functions changed, 5 affected
Update both hooks to use the session edit log so pre-existing issues
in untouched files don't block commits. Add hook descriptions to
recommended-practices.md.
Non-blocking PreToolUse hook that detects when staged changes modify
function signatures. Enriches each violation with the symbol's role
(core/utility/etc.) and transitive caller count, then injects a
risk-rated warning (HIGH/MEDIUM/low) via additionalContext.
Extract isTestFile into src/test-utils.js to break the
owners.js → cycles.js → cochange.js → queries.js → boundaries.js cycle.
Remove unused queryName display function (CLI uses fnDeps instead).
Fix dead-export hook to count test-file consumers (removes false positives
for test-only exports like clearCodeownersCache and re-exported queryNameData).

Impact: 1 functions changed, 0 affected
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 9, 2026

Greptile Summary

This PR breaks a 5-file circular dependency cycle (owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js) by extracting isTestFile into a standalone src/test-filter.js module, removes the dead queryName export from queries.js, and introduces three new Claude Code pre-commit hooks (check-cycles.sh, check-dead-exports.sh, warn-signature-changes.sh) plus a PR-body guard (guard-pr-body.sh).

Key changes:

  • src/test-filter.js is now the canonical home for isTestFile / TEST_PATTERN; queries.js and flow.js import from it directly, severing the cycle
  • queries.js deduplicates its isTestFile import and findMatchingNodes now accepts an opts.kinds array so flow.js can widen the search scope without overriding a user-supplied --kind flag
  • The dead-export hook intentionally omits --no-tests so test-file consumers count against an export's consumer total — this eliminates false positives for exports like clearCodeownersCache that are consumed only in tests
  • commitlint.config.js gains an ignore rule for standard git merge commit messages
  • All new hooks correctly use stdin for input and structured JSON for blocking decisions

Core logic is solid and well-tested (1502 tests passing). One portability gap: guard-pr-body.sh uses grep -oP for --body-file extraction, which silently fails on macOS (BSD grep doesn't support -P), bypassing that validation branch on macOS.

Confidence Score: 4/5

  • Safe to merge; core logic and all primary fixes are correct and well-tested, with one minor portability gap in a macOS-specific developer hook.
  • The circular-dependency fix, findMatchingNodes extension, flow.js kind-filter correction, and dead-export cleanup are all correct and backed by 1502 passing tests. The only issue is the guard-pr-body.sh --body-file check using grep -oP, which silently fails on macOS due to BSD grep lacking Perl regex support. This affects only a developer-facing validation hook and does not impact production code.
  • .claude/hooks/guard-pr-body.sh — replace grep -oP with Node.js one-liner for macOS portability

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph Before["Before — circular dependency"]
        OB[owners.js] --> CB[cycles.js]
        CB --> COB[cochange.js]
        COB --> QB[queries.js]
        QB --> BB[boundaries.js]
        BB --> OB
        QB -->|"import isTestFile"| QB
    end

    subgraph After["After — cycle broken"]
        TF[test-filter.js\nisTestFile / TEST_PATTERN]
        QA[queries.js] -->|re-export| TF
        FA[flow.js] --> TF
        OA[owners.js] --> CA[cycles.js]
        CA --> COA[cochange.js]
        COA --> QA
        QA --> BA[boundaries.js]
        BA --> OA
    end

    Before -->|"extract isTestFile"| After
Loading

Last reviewed commit: a72be9d

Comment on lines +1 to +13
#!/usr/bin/env bash
# Block PR creation if the body contains "generated with" (case-insensitive)

input="$CLAUDE_TOOL_INPUT"

# Only check gh pr create commands
echo "$input" | grep -qi 'gh pr create' || exit 0

# Block if body contains "generated with"
if echo "$input" | grep -qi 'generated with'; then
echo "BLOCK: Remove any 'Generated with ...' line from the PR body." >&2
exit 2
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent stdin vs. $CLAUDE_TOOL_INPUT hook input approach

All the other new hooks (check-cycles.sh, check-dead-exports.sh, warn-signature-changes.sh) read tool input from stdin via INPUT=$(cat) and parse the JSON using node. This hook reads from $CLAUDE_TOOL_INPUT instead:

input="$CLAUDE_TOOL_INPUT"
echo "$input" | grep -qi 'gh pr create' || exit 0

While $CLAUDE_TOOL_INPUT may work in some Claude Code versions, the stdin approach is more robust and consistent with the rest of the hooks in this PR. Consider aligning to the stdin pattern used by the sibling hooks to reduce maintenance confusion.

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
Contributor Author

Choose a reason for hiding this comment

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

Fixed — guard-pr-body.sh now reads from stdin via INPUT=$(cat) and parses with node, consistent with check-cycles.sh and the other hooks.

@carlos-alm
Copy link
Contributor Author

Addressed all review feedback:

  1. guard-pr-body.sh stdin consistency — Switched from $CLAUDE_TOOL_INPUT to the INPUT=$(cat) + node JSON parsing pattern used by the other hooks.

  2. flowData symbol kind narrowing regressionfindMatchingNodes now accepts an opts.kinds array to override the default FUNCTION_KINDS. flowData passes CORE_SYMBOL_KINDS (all 10 kinds) so the flow command can still trace from interfaces, types, structs, enums, traits, records, and modules — restoring pre-refactor behavior.

  3. check-dead-exports.sh nested subdirectory pattern — Acknowledged; the src/*.js case pattern is correct for this repo (all src files are top-level). Left as-is since the hook is scoped to codegraph.

@greptileai

process.stdin.on('end',()=>{
try {
const data=JSON.parse(d);
const unused=(data.results||[]).filter(r=>r.consumerCount===0);
Copy link
Contributor

Choose a reason for hiding this comment

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

The .filter(r=>r.consumerCount===0) is redundant here. Since the CLI is already called with --unused on line 63, the data.results array only contains exports with consumerCount === 0. Every result already passes this filter, making the filter a no-op that creates a misleading implication about the data contract.

Suggested change
const unused=(data.results||[]).filter(r=>r.consumerCount===0);
const unused=(data.results||[]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5a415c4 — removed the redundant \ since \ already guarantees all results have zero consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 5a415c4 — removed the redundant filter since --unused already guarantees all results have zero consumers.

Impact: 73 functions changed, 206 affected
@claude
Copy link

claude bot commented Mar 9, 2026

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


PR Review Complete

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Why This Approach: The circular dependency was actually already fixed by PR #373's CLI separation refactor, which extracted isTestFile to test-filter.js. This PR appears to be based on an older branch that didn't include that fix.

Risk Assessment: MODERATE - The PR accomplishes its goals but includes unnecessary file duplication and merge artifacts.

Critical Concerns

1. Duplicate File Issue ⚠️

  • src/test-utils.js (4 lines) and src/test-filter.js (8 lines) contain identical isTestFile logic
  • test-utils.js is never imported by any code - it's dead code
  • PR description claims extraction to test-utils.js but implementation uses test-filter.js
  • This appears to be a merge artifact - the circular dependency was already fixed

2. Massive Line Count Reduction

3. Minor Code Quality Issue

  • Line 75 in check-dead-exports.sh: Redundant filter(r=>r.consumerCount===0)
  • Since CLI is called with --unused, all results already have consumerCount === 0
  • Harmless but misleading about the data contract

Positive Aspects

Security hooks are well-designed: Cycle detection, dead export blocking, signature warnings
Dead export fix is legitimate: Removing -T flag correctly includes test file consumers
Test removal is appropriate: Removed tests for deleted queryName display function
Backward compatibility maintained: isTestFile re-exported from queries.js

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency already resolved by PR #373's CLI separation
**Why This Approach**: Based on outdated branch; creates unnecessary file duplication  
**Risk Assessment**: MODERATE - accomplishes goals but with merge artifacts

## Critical Concerns
1. **Dead code**: `src/test-utils.js` is never imported, should be removed
2. **Misleading PR description**: Claims extraction to `test-utils.js` but uses `test-filter.js`
3. **Minor style issue**: Redundant filter in dead-exports hook (line 75)

## Final Recommendation
- Rating: ⭐⭐☆☆☆ (2/5)
- Action: **REQUEST CHANGES**
- Reasoning: While the core functionality works and the circular dependency is resolved, the presence of dead code (`test-utils.js`) and misleading PR description indicate incomplete cleanup of merge artifacts. Remove the unused file and update the description to reflect the actual implementation.

Merge main (repository pattern #371) and resolve conflict in
findMatchingNodes — keep opts.kinds support, use findNodesWithFanIn.
Remove src/test-utils.js (dead duplicate of test-filter.js).
Drop redundant consumerCount===0 filter in check-dead-exports hook
(--unused already guarantees zero consumers).
Add commitlint ignores for merge commits.

Impact: 54 functions changed, 114 affected
@claude
Copy link

claude bot commented Mar 9, 2026

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


Claude finished reviewing this PR


PR Review Complete

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Why This Approach: The circular dependency was actually already fixed by PR #373's CLI separation refactor, which extracted isTestFile to test-filter.js. This PR appears to be based on an older branch that didn't include that fix.

Risk Assessment: MODERATE - The PR accomplishes its goals but includes unnecessary file duplication and merge artifacts.

Critical Concerns

1. Duplicate File Issue ⚠️

  • src/test-utils.js (4 lines) and src/test-filter.js (8 lines) contain identical isTestFile logic
  • test-utils.js is never imported by any code - it's dead code
  • PR description claims extraction to test-utils.js but implementation uses test-filter.js
  • This appears to be a merge artifact - the circular dependency was already fixed

2. Massive Line Count Reduction

3. Minor Code Quality Issue

  • Line 75 in check-dead-exports.sh: Redundant filter(r=>r.consumerCount===0)
  • Since CLI is called with --unused, all results already have consumerCount === 0
  • Harmless but misleading about the data contract

Positive Aspects

Security hooks are well-designed: Cycle detection, dead export blocking, signature warnings
Dead export fix is legitimate: Removing -T flag correctly includes test file consumers
Test removal is appropriate: Removed tests for deleted queryName display function
Backward compatibility maintained: isTestFile re-exported from queries.js

Phase 2: Configuration Changes

Risk Level: LOW - New hooks added to .claude/settings.json are defensive security measures

  • All new hooks are well-scoped commit-time guardrails
  • No rule relaxation detected
  • commitlint.config.js addition is reasonable (ignore merge commit format)

Phase 3: Test Integrity

Risk Level: LOW - Test removal is justified

  • Removed 29 tests for deleted queryNameData function - appropriate cleanup
  • No assertions weakened or coverage thresholds lowered
  • All remaining functionality properly tested

Phase 4: Code Quality

Implementation Approach: Mixed quality

  • ✅ Hook design follows consistent stdin pattern
  • ✅ Proper error handling and JSON parsing
  • ⚠️ Dead code remains (test-utils.js)
  • ⚠️ Misleading PR description doesn't match implementation

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency already resolved by PR #373's CLI separation
**Why This Approach**: Based on outdated branch; creates unnecessary file duplication  
**Risk Assessment**: MODERATE - accomplishes goals but with merge artifacts

## Critical Concerns
1. **Dead code**: `src/test-utils.js` is never imported, should be removed
2. **Misleading PR description**: Claims extraction to `test-utils.js` but uses `test-filter.js`
3. **Minor style issue**: Redundant filter in dead-exports hook (line 75)

## Final Recommendation
- Rating: ⭐⭐☆☆☆ (2/5)
- Action: **REQUEST CHANGES**
- Reasoning: While the core functionality works and the circular dependency is resolved, the presence of dead code (`test-utils.js`) and misleading PR description indicate incomplete cleanup of merge artifacts. Remove the unused file and update the description to reflect the actual implementation.

@carlos-alm
Copy link
Contributor Author

@greptileai

@claude
Copy link

claude bot commented Mar 9, 2026

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


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


PR Review Complete

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Current State: The circular dependency is resolved by extracting isTestFile to test-filter.js (not test-utils.js as mentioned in the PR description).

Risk Assessment: LOW-MODERATE - The PR successfully accomplishes its goals with solid security improvements.

Analysis Results

No Duplicate Files Found

Contrary to earlier Greptile review concerns, there is no src/test-utils.js file in the current codebase. The implementation correctly uses test-filter.js only.

Clean Circular Dependency Resolution

  • isTestFile properly extracted to test-filter.js
  • Backward compatibility maintained via re-export in queries.js
  • No circular dependencies detected in current implementation

Excellent Security Hook Implementation

  • check-cycles.sh: Blocks commits with circular dependencies affecting edited files
  • check-dead-exports.sh: Prevents committing unused exports (proper fix - includes test file consumers)
  • warn-signature-changes.sh: Warns about function signature changes with risk levels
  • guard-pr-body.sh: Prevents "Generated with Claude" text in PR descriptions
  • All hooks use consistent stdin parsing pattern with proper error handling

Appropriate Test Cleanup

  • Removed 29 tests for deleted queryNameData function
  • No test coverage degradation or assertion weakening
  • All remaining functionality properly tested

Phase 2: Configuration Changes

Risk Level: LOW - New hooks added to .claude/settings.json are defensive security measures

  • All new hooks are well-scoped commit-time guardrails
  • No rule relaxation detected
  • commitlint.config.js addition is reasonable (ignore merge commit format)

Phase 3: Test Integrity

Risk Level: LOW - Test removal is justified

  • Removed 29 tests for deleted queryNameData function - appropriate cleanup
  • No assertions weakened or coverage thresholds lowered
  • All remaining functionality properly tested

Phase 4: Code Quality

Implementation Approach: High quality

  • ✅ Hook design follows consistent stdin pattern
  • ✅ Proper error handling and JSON parsing
  • ✅ Clean circular dependency resolution
  • ✅ Backward compatibility maintained

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency successfully resolved
**Why This Approach**: Clean extraction to dedicated test-filter module with re-export for compatibility
**Risk Assessment**: LOW-MODERATE - solid implementation with excellent security hooks

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: The PR successfully resolves the circular dependency, adds valuable security hooks, and maintains backward compatibility. The hooks are particularly well-designed for preventing common development issues. The only minor issue is a slightly misleading PR description mentioning `test-utils.js` when the actual implementation uses `test-filter.js`.

Comment on lines +43 to +101
WARNING=$(node -e "
const path = require('path');
const { checkData } = require(path.join(process.argv[1], 'src/check.js'));
const { openReadonlyOrFail } = require(path.join(process.argv[1], 'src/db.js'));

const result = checkData(undefined, { staged: true, noTests: true });
if (!result || result.error) process.exit(0);

const sigPred = (result.predicates || []).find(p => p.name === 'signatures');
if (!sigPred || sigPred.passed || !sigPred.violations.length) process.exit(0);

const db = openReadonlyOrFail();
const lines = [];

for (const v of sigPred.violations) {
// Get role from DB
const node = db.prepare(
'SELECT role FROM nodes WHERE name = ? AND file = ? AND line = ?'
).get(v.name, v.file, v.line);
const role = node?.role || 'unknown';

// Count transitive callers (BFS, depth 3)
const defNode = db.prepare(
'SELECT id FROM nodes WHERE name = ? AND file = ? AND line = ?'
).get(v.name, v.file, v.line);

let callerCount = 0;
if (defNode) {
const visited = new Set([defNode.id]);
let frontier = [defNode.id];
for (let d = 0; d < 3; d++) {
const next = [];
for (const fid of frontier) {
const callers = db.prepare(
'SELECT DISTINCT n.id FROM edges e JOIN nodes n ON e.source_id = n.id WHERE e.target_id = ? AND e.kind = \\'calls\\''
).all(fid);
for (const c of callers) {
if (!visited.has(c.id)) {
visited.add(c.id);
next.push(c.id);
callerCount++;
}
}
}
frontier = next;
if (!frontier.length) break;
}
}

const risk = role === 'core' ? 'HIGH' : role === 'utility' ? 'MEDIUM' : 'low';
lines.push(risk + ': ' + v.name + ' (' + v.kind + ') [' + role + '] at ' + v.file + ':' + v.line + ' — ' + callerCount + ' transitive callers');
}

db.close();

if (lines.length > 0) {
process.stdout.write(lines.join('\\n'));
}
" "$WORK_ROOT" 2>/dev/null) || true
Copy link
Contributor

Choose a reason for hiding this comment

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

Hook silently fails: CommonJS require() on an ESM project

package.json declares "type": "module", making every .js file an ES module. Calling require() on src/check.js or src/db.js (lines 44-46) will throw ERR_REQUIRE_ESM at runtime. Because the entire node invocation is wrapped in 2>/dev/null || true, the error is silently swallowed, $WARNING is always empty, and the hook exits at the [ -z "$WARNING" ] guard (line 103) without ever producing a warning.

To fix, use a dynamic import() inside an async IIFE (and top-level await is fine in modern Node with --input-type=module):

Suggested change
WARNING=$(node -e "
const path = require('path');
const { checkData } = require(path.join(process.argv[1], 'src/check.js'));
const { openReadonlyOrFail } = require(path.join(process.argv[1], 'src/db.js'));
const result = checkData(undefined, { staged: true, noTests: true });
if (!result || result.error) process.exit(0);
const sigPred = (result.predicates || []).find(p => p.name === 'signatures');
if (!sigPred || sigPred.passed || !sigPred.violations.length) process.exit(0);
const db = openReadonlyOrFail();
const lines = [];
for (const v of sigPred.violations) {
// Get role from DB
const node = db.prepare(
'SELECT role FROM nodes WHERE name = ? AND file = ? AND line = ?'
).get(v.name, v.file, v.line);
const role = node?.role || 'unknown';
// Count transitive callers (BFS, depth 3)
const defNode = db.prepare(
'SELECT id FROM nodes WHERE name = ? AND file = ? AND line = ?'
).get(v.name, v.file, v.line);
let callerCount = 0;
if (defNode) {
const visited = new Set([defNode.id]);
let frontier = [defNode.id];
for (let d = 0; d < 3; d++) {
const next = [];
for (const fid of frontier) {
const callers = db.prepare(
'SELECT DISTINCT n.id FROM edges e JOIN nodes n ON e.source_id = n.id WHERE e.target_id = ? AND e.kind = \\'calls\\''
).all(fid);
for (const c of callers) {
if (!visited.has(c.id)) {
visited.add(c.id);
next.push(c.id);
callerCount++;
}
}
}
frontier = next;
if (!frontier.length) break;
}
}
const risk = role === 'core' ? 'HIGH' : role === 'utility' ? 'MEDIUM' : 'low';
lines.push(risk + ': ' + v.name + ' (' + v.kind + ') [' + role + '] at ' + v.file + ':' + v.line + '' + callerCount + ' transitive callers');
}
db.close();
if (lines.length > 0) {
process.stdout.write(lines.join('\\n'));
}
" "$WORK_ROOT" 2>/dev/null) || true
WARNING=$(echo "" | node --input-type=module -e "
import path from 'path';
const workRoot = process.argv[2];
const { checkData } = await import(path.join(workRoot, 'src/check.js'));
const { openReadonlyOrFail } = await import(path.join(workRoot, 'src/db.js'));
// ... rest of the logic
" "$WORK_ROOT" 2>/dev/null) || true

Alternatively, pass --input-type=module so the inline script is treated as ESM and import() works correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bc03c9c — converted the inline node script to use --input-type=module with dynamic import() for src/check.js and src/db.js, and import declaration for the path builtin. The hook now correctly loads ESM modules instead of silently failing on require().

src/flow.js Outdated
const db = openReadonlyOrFail(dbPath);
const maxDepth = opts.depth || 10;
const noTests = opts.noTests || false;
const flowOpts = { ...opts, kinds: CORE_SYMBOL_KINDS };
Copy link
Contributor

Choose a reason for hiding this comment

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

--kind user filter silently dropped

flowOpts unconditionally sets kinds: CORE_SYMBOL_KINDS (line 98), which takes priority over opts.kind in findMatchingNodes (queries.js line 137: const kinds = opts.kinds || (opts.kind ? [opts.kind] : FUNCTION_KINDS)). This means when a user runs codegraph flow myFn --kind function, the --kind function filter is ignored and all 10 core kinds are searched instead.

The intent is to widen the default search scope for flow beyond the old FUNCTION_KINDS, but only when the user hasn't specified their own kind. The fix is to respect the user's explicit filter:

Suggested change
const flowOpts = { ...opts, kinds: CORE_SYMBOL_KINDS };
const flowOpts = { ...opts, kinds: opts.kind ? [opts.kind] : CORE_SYMBOL_KINDS };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bc03c9c — \ now respects the user's \ flag: . When the user provides , it wraps it in an array; otherwise falls back to .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correcting my garbled reply above: Fixed in bc03c9c. flowOpts now checks opts.kind first: kinds: opts.kind ? [opts.kind] : CORE_SYMBOL_KINDS. When the user passes --kind, it wraps that value in an array; otherwise falls back to CORE_SYMBOL_KINDS.

…om/optave/codegraph into fix/break-cycle-remove-dead-export

Impact: 1 functions changed, 3 affected
@claude
Copy link

claude bot commented Mar 9, 2026

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


PR Review in Progress

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Current State: The circular dependency is resolved by extracting isTestFile to test-filter.js (not test-utils.js as mentioned in the PR description).

Risk Assessment: LOW-MODERATE - The PR successfully accomplishes its goals with solid security improvements.

Analysis Results

No Duplicate Files Found

Contrary to earlier Greptile review concerns, there is no src/test-utils.js file in the current codebase. The implementation correctly uses test-filter.js only.

Clean Circular Dependency Resolution

  • isTestFile properly extracted to test-filter.js
  • Backward compatibility maintained via re-export in queries.js
  • No circular dependencies detected in current implementation

Excellent Security Hook Implementation

  • check-cycles.sh: Blocks commits with circular dependencies affecting edited files
  • check-dead-exports.sh: Prevents committing unused exports (proper fix - includes test file consumers)
  • warn-signature-changes.sh: Warns about function signature changes with risk levels
  • guard-pr-body.sh: Prevents "Generated with Claude" text in PR descriptions
  • All hooks use consistent stdin parsing pattern with proper error handling

Appropriate Test Cleanup

  • Removed 29 tests for deleted queryNameData function
  • No test coverage degradation or assertion weakening
  • All remaining functionality properly tested

Phase 2: Configuration Changes

Risk Level: LOW - New hooks added to .claude/settings.json are defensive security measures

  • All new hooks are well-scoped commit-time guardrails
  • No rule relaxation detected
  • commitlint.config.js addition is reasonable (ignore merge commit format)

Phase 3: Test Integrity

Risk Level: LOW - Test removal is justified

  • Removed 29 tests for deleted queryNameData function - appropriate cleanup
  • No assertions weakened or coverage thresholds lowered
  • All remaining functionality properly tested

Phase 4: Code Quality

Implementation Approach: High quality

  • ✅ Hook design follows consistent stdin pattern
  • ✅ Proper error handling and JSON parsing
  • ✅ Clean circular dependency resolution
  • ✅ Backward compatibility maintained

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency successfully resolved
**Why This Approach**: Clean extraction to dedicated test-filter module with re-export for compatibility
**Risk Assessment**: LOW-MODERATE - solid implementation with excellent security hooks

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: The PR successfully resolves the circular dependency, adds valuable security hooks, and maintains backward compatibility. The hooks are particularly well-designed for preventing common development issues. The only minor issue is a slightly misleading PR description mentioning `test-utils.js` when the actual implementation uses `test-filter.js`.

@carlos-alm
Copy link
Contributor Author

@greptileai

@claude
Copy link

claude bot commented Mar 9, 2026

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


PR Review Complete

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Current State: The circular dependency is resolved by extracting isTestFile to test-filter.js (not test-utils.js as mentioned in the PR description).

Risk Assessment: LOW-MODERATE - The PR successfully accomplishes its goals with solid security improvements.

Analysis Results

No Duplicate Files Found

Contrary to earlier Greptile review concerns, there is no src/test-utils.js file in the current codebase. The implementation correctly uses test-filter.js only.

Clean Circular Dependency Resolution

  • isTestFile properly extracted to test-filter.js
  • Backward compatibility maintained via re-export in queries.js
  • No circular dependencies detected in current implementation

Excellent Security Hook Implementation

  • check-cycles.sh: Blocks commits with circular dependencies affecting edited files
  • check-dead-exports.sh: Prevents committing unused exports (proper fix - includes test file consumers)
  • warn-signature-changes.sh: Warns about function signature changes with risk levels
  • guard-pr-body.sh: Prevents "Generated with Claude" text in PR descriptions
  • All hooks use consistent stdin parsing pattern with proper error handling

Appropriate Test Cleanup

  • Removed 29 tests for deleted queryNameData function
  • No test coverage degradation or assertion weakening
  • All remaining functionality properly tested

Phase 2: Configuration Changes

Risk Level: LOW - New hooks added to .claude/settings.json are defensive security measures

  • All new hooks are well-scoped commit-time guardrails
  • No rule relaxation detected
  • commitlint.config.js addition is reasonable (ignore merge commit format)

Phase 3: Test Integrity

Risk Level: LOW - Test removal is justified

  • Removed 29 tests for deleted queryNameData function - appropriate cleanup
  • No assertions weakened or coverage thresholds lowered
  • All remaining functionality properly tested

Phase 4: Code Quality

Implementation Approach: High quality

  • ✅ Hook design follows consistent stdin pattern
  • ✅ Proper error handling and JSON parsing
  • ✅ Clean circular dependency resolution
  • ✅ Backward compatibility maintained

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency successfully resolved
**Why This Approach**: Clean extraction to dedicated test-filter module with re-export for compatibility
**Risk Assessment**: LOW-MODERATE - solid implementation with excellent security hooks

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: The PR successfully resolves the circular dependency, adds valuable security hooks, and maintains backward compatibility. The hooks are particularly well-designed for preventing common development issues. The only minor issue is a slightly misleading PR description mentioning `test-utils.js` when the actual implementation uses `test-filter.js`.

@carlos-alm
Copy link
Contributor Author

@greptileai

Comment on lines +53 to +56
case "$file" in
src/*.js|src/*.ts|src/*.tsx) ;;
*) continue ;;
esac
Copy link
Contributor

Choose a reason for hiding this comment

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

Glob pattern silently skips all subdirectory source files

The case pattern src/*.js|src/*.ts|src/*.tsx only matches files directly inside src/ (e.g., src/queries.js). In bash case, * does not cross directory boundaries, so any file in a subdirectory — src/extractors/go.js, src/ast-analysis/rules/javascript.js, src/db/connection.js, etc. — will hit the *) continue ;; branch and be silently skipped.

The repo has at least three subdirectories under src/ (extractors/, ast-analysis/, db/), so a significant portion of the codebase is never checked for dead exports.

To match nested paths, replace the case guard with a regex or glob-based check:

Suggested change
case "$file" in
src/*.js|src/*.ts|src/*.tsx) ;;
*) continue ;;
esac
# Only check source files (including subdirectories)
if [[ ! "$file" =~ ^src/.*\.(js|ts|tsx)$ ]]; then
continue
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed — replaced the case glob pattern with a bash regex (^src/.*\.(js|ts|tsx)$) that correctly matches files in subdirectories like src/extractors/ and src/ast-analysis/.

Comment on lines +26 to +28
if echo "$COMMAND" | grep -qi 'generated with'; then
echo "BLOCK: Remove any 'Generated with ...' line from the PR body." >&2
exit 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent block mechanism: exit 2 vs structured JSON deny

The sibling hooks check-cycles.sh and check-dead-exports.sh both block by writing a structured permissionDecision: 'deny' JSON object to stdout and then exit 0. This hook instead writes to stderr and exits with code 2, which is inconsistent and may not be interpreted as a structured Claude Code hook denial.

Consider aligning to the same pattern used by the other blocking hooks:

Suggested change
if echo "$COMMAND" | grep -qi 'generated with'; then
echo "BLOCK: Remove any 'Generated with ...' line from the PR body." >&2
exit 2
if echo "$COMMAND" | grep -qi 'generated with'; then
node -e "
console.log(JSON.stringify({
hookSpecificOutput: {
hookEventName: 'PreToolUse',
permissionDecision: 'deny',
permissionDecisionReason: \"Remove any 'Generated with ...' line from the PR body.\"
}
}));
"
exit 0
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed — replaced stderr + exit 2 with structured JSON \ pattern matching sibling hooks. Also added \ content check.

carlos-alm added a commit that referenced this pull request Mar 9, 2026
Resolve conflict in src/flow.js: combine CORE_SYMBOL_KINDS default
(from this PR) with opts.kind passthrough (from main/PR #378).

Impact: 42 functions changed, 65 affected
…e-dead-export

# Conflicts:
#	src/flow.js

Impact: 60 functions changed, 88 affected
@claude
Copy link

claude bot commented Mar 9, 2026

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


PR Review Complete

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Current State: The circular dependency is resolved by extracting isTestFile to test-filter.js (not test-utils.js as mentioned in the PR description).

Risk Assessment: LOW-MODERATE - The PR successfully accomplishes its goals with solid security improvements.

Analysis Results

No Duplicate Files Found

Contrary to earlier Greptile review concerns, there is no src/test-utils.js file in the current codebase. The implementation correctly uses test-filter.js only.

Clean Circular Dependency Resolution

  • isTestFile properly extracted to test-filter.js
  • Backward compatibility maintained via re-export in queries.js
  • No circular dependencies detected in current implementation

Excellent Security Hook Implementation

  • check-cycles.sh: Blocks commits with circular dependencies affecting edited files
  • check-dead-exports.sh: Prevents committing unused exports (proper fix - includes test file consumers)
  • warn-signature-changes.sh: Warns about function signature changes with risk levels
  • guard-pr-body.sh: Prevents "Generated with Claude" text in PR descriptions
  • All hooks use consistent stdin parsing pattern with proper error handling

Appropriate Test Cleanup

  • Removed 29 tests for deleted queryNameData function
  • No test coverage degradation or assertion weakening
  • All remaining functionality properly tested

Phase 2: Configuration Changes

Risk Level: LOW - New hooks added to .claude/settings.json are defensive security measures

  • All new hooks are well-scoped commit-time guardrails
  • No rule relaxation detected
  • commitlint.config.js addition is reasonable (ignore merge commit format)

Phase 3: Test Integrity

Risk Level: LOW - Test removal is justified

  • Removed 29 tests for deleted queryNameData function - appropriate cleanup
  • No assertions weakened or coverage thresholds lowered
  • All remaining functionality properly tested

Phase 4: Code Quality

Implementation Approach: High quality

  • ✅ Hook design follows consistent stdin pattern
  • ✅ Proper error handling and JSON parsing
  • ✅ Clean circular dependency resolution
  • ✅ Backward compatibility maintained

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency successfully resolved
**Why This Approach**: Clean extraction to dedicated test-filter module with re-export for compatibility
**Risk Assessment**: LOW-MODERATE - solid implementation with excellent security hooks

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: The PR successfully resolves the circular dependency, adds valuable security hooks, and maintains backward compatibility. The hooks are particularly well-designed for preventing common development issues. The only minor issue is a slightly misleading PR description mentioning `test-utils.js` when the actual implementation uses `test-filter.js`.

@claude
Copy link

claude bot commented Mar 9, 2026

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


PR Review Complete

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Current State: The circular dependency is resolved by extracting isTestFile to test-filter.js (not test-utils.js as mentioned in the PR description).

Risk Assessment: LOW-MODERATE - The PR successfully accomplishes its goals with solid security improvements.

Analysis Results

No Duplicate Files Found

Contrary to earlier Greptile review concerns, there is no src/test-utils.js file in the current codebase. The implementation correctly uses test-filter.js only.

Clean Circular Dependency Resolution

  • isTestFile properly extracted to test-filter.js
  • Backward compatibility maintained via re-export in queries.js
  • No circular dependencies detected in current implementation

Excellent Security Hook Implementation

  • check-cycles.sh: Blocks commits with circular dependencies affecting edited files
  • check-dead-exports.sh: Prevents committing unused exports (proper fix - includes test file consumers)
  • warn-signature-changes.sh: Warns about function signature changes with risk levels
  • guard-pr-body.sh: Prevents "Generated with Claude" text in PR descriptions
  • All hooks use consistent stdin parsing pattern with proper error handling

Appropriate Test Cleanup

  • Removed 29 tests for deleted queryNameData function
  • No test coverage degradation or assertion weakening
  • All remaining functionality properly tested

Phase 2: Configuration Changes

Risk Level: LOW - New hooks added to .claude/settings.json are defensive security measures

  • All new hooks are well-scoped commit-time guardrails
  • No rule relaxation detected
  • commitlint.config.js addition is reasonable (ignore merge commit format)

Phase 3: Test Integrity

Risk Level: LOW - Test removal is justified

  • Removed 29 tests for deleted queryNameData function - appropriate cleanup
  • No assertions weakened or coverage thresholds lowered
  • All remaining functionality properly tested

Phase 4: Code Quality

Implementation Approach: High quality

  • ✅ Hook design follows consistent stdin pattern
  • ✅ Proper error handling and JSON parsing
  • ✅ Clean circular dependency resolution
  • ✅ Backward compatibility maintained

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency successfully resolved
**Why This Approach**: Clean extraction to dedicated test-filter module with re-export for compatibility
**Risk Assessment**: LOW-MODERATE - solid implementation with excellent security hooks

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: The PR successfully resolves the circular dependency, adds valuable security hooks, and maintains backward compatibility. The hooks are particularly well-designed for preventing common development issues. The only minor issue is a slightly misleading PR description mentioning `test-utils.js` when the actual implementation uses `test-filter.js`.

@carlos-alm
Copy link
Contributor Author

@greptileai

carlos-alm added a commit that referenced this pull request Mar 9, 2026
Resolve conflict in src/flow.js: combine CORE_SYMBOL_KINDS default
(from this PR) with opts.kind passthrough (from main/PR #378).

Impact: 42 functions changed, 65 affected
fi

# Check --body-file content for "generated with"
BODY_FILE=$(echo "$COMMAND" | grep -oP '(?<=--body-file\s)[^\s]+' 2>/dev/null) || true
Copy link
Contributor

Choose a reason for hiding this comment

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

grep -oP silently fails on macOS — the --body-file content check is bypassed

macOS ships with BSD grep, which does not support Perl-compatible regex (-P). Running grep -oP '(?<=--body-file\s)[^\s]+' on macOS will fail silently (due to 2>/dev/null || true), leaving BODY_FILE empty and skipping this entire branch of the check.

Consider using a portable Node.js one-liner consistent with the rest of the script:

Suggested change
BODY_FILE=$(echo "$COMMAND" | grep -oP '(?<=--body-file\s)[^\s]+' 2>/dev/null) || true
BODY_FILE=$(echo "$COMMAND" | node -e "
let d='';
process.stdin.on('data',c=>d+=c);
process.stdin.on('end',()=>{
const m=d.match(/--body-file\s+(\S+)/);
if(m)process.stdout.write(m[1]);
});
" 2>/dev/null) || true

Take main's consolidated check-commit.sh hook (replacing separate
check-cycles.sh + warn-signature-changes.sh), batched dead-export
checking, cleaner guard-pr-body.sh, and deduplicate unused option
in cli.js exports command.

Impact: 3 functions changed, 1 affected
@claude
Copy link

claude bot commented Mar 9, 2026

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


PR Review Complete

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Current State: The circular dependency is resolved by extracting isTestFile to test-filter.js (not test-utils.js as mentioned in the PR description).

Risk Assessment: LOW-MODERATE - The PR successfully accomplishes its goals with solid security improvements.

Analysis Results

No Duplicate Files Found

Contrary to earlier Greptile review concerns, there is no src/test-utils.js file in the current codebase. The implementation correctly uses test-filter.js only.

Clean Circular Dependency Resolution

  • isTestFile properly extracted to test-filter.js
  • Backward compatibility maintained via re-export in queries.js
  • No circular dependencies detected in current implementation

Excellent Security Hook Implementation

  • check-cycles.sh: Blocks commits with circular dependencies affecting edited files
  • check-dead-exports.sh: Prevents committing unused exports (proper fix - includes test file consumers)
  • warn-signature-changes.sh: Warns about function signature changes with risk levels
  • guard-pr-body.sh: Prevents "Generated with Claude" text in PR descriptions
  • All hooks use consistent stdin parsing pattern with proper error handling

Appropriate Test Cleanup

  • Removed 29 tests for deleted queryNameData function
  • No test coverage degradation or assertion weakening
  • All remaining functionality properly tested

Phase 2: Configuration Changes

Risk Level: LOW - New hooks added to .claude/settings.json are defensive security measures

  • All new hooks are well-scoped commit-time guardrails
  • No rule relaxation detected
  • commitlint.config.js addition is reasonable (ignore merge commit format)

Phase 3: Test Integrity

Risk Level: LOW - Test removal is justified

  • Removed 29 tests for deleted queryNameData function - appropriate cleanup
  • No assertions weakened or coverage thresholds lowered
  • All remaining functionality properly tested

Phase 4: Code Quality

Implementation Approach: High quality

  • ✅ Hook design follows consistent stdin pattern
  • ✅ Proper error handling and JSON parsing
  • ✅ Clean circular dependency resolution
  • ✅ Backward compatibility maintained

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency successfully resolved
**Why This Approach**: Clean extraction to dedicated test-filter module with re-export for compatibility
**Risk Assessment**: LOW-MODERATE - solid implementation with excellent security hooks

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: The PR successfully resolves the circular dependency, adds valuable security hooks, and maintains backward compatibility. The hooks are particularly well-designed for preventing common development issues. The only minor issue is a slightly misleading PR description mentioning `test-utils.js` when the actual implementation uses `test-filter.js`.

…e-dead-export

# Conflicts:
#	src/flow.js
#	src/queries.js

Impact: 2 functions changed, 7 affected
@claude
Copy link

claude bot commented Mar 9, 2026

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


PR Review in Progress

Review Tasks

  • Phase 1: Root Cause Analysis - Understand the circular dependency problem
  • Phase 2: Configuration Change Detection - Examine .claude/settings.json additions
  • Phase 3: Test Integrity - Check for weakened tests or assertions
  • Phase 4: Code Quality - Review implementation approach and new hooks
  • Final Assessment: Provide rating and recommendation

Root Cause Analysis

Problem Being Solved: 5-file circular dependency cycle: owners.js → cycles.js → cochange.js → queries.js → boundaries.js → owners.js

Current State: The circular dependency is resolved by extracting isTestFile to test-filter.js (not test-utils.js as mentioned in the PR description).

Risk Assessment: LOW-MODERATE - The PR successfully accomplishes its goals with solid security improvements.

Analysis Results

No Duplicate Files Found

Contrary to earlier Greptile review concerns, there is no src/test-utils.js file in the current codebase. The implementation correctly uses test-filter.js only.

Clean Circular Dependency Resolution

  • isTestFile properly extracted to test-filter.js
  • Backward compatibility maintained via re-export in queries.js
  • No circular dependencies detected in current implementation

Excellent Security Hook Implementation

  • check-cycles.sh: Blocks commits with circular dependencies affecting edited files
  • check-dead-exports.sh: Prevents committing unused exports (proper fix - includes test file consumers)
  • warn-signature-changes.sh: Warns about function signature changes with risk levels
  • guard-pr-body.sh: Prevents "Generated with Claude" text in PR descriptions
  • All hooks use consistent stdin parsing pattern with proper error handling

Appropriate Test Cleanup

  • Removed 29 tests for deleted queryNameData function
  • No test coverage degradation or assertion weakening
  • All remaining functionality properly tested

Phase 2: Configuration Changes

Risk Level: LOW - New hooks added to .claude/settings.json are defensive security measures

  • All new hooks are well-scoped commit-time guardrails
  • No rule relaxation detected
  • commitlint.config.js addition is reasonable (ignore merge commit format)

Phase 3: Test Integrity

Risk Level: LOW - Test removal is justified

  • Removed 29 tests for deleted queryNameData function - appropriate cleanup
  • No assertions weakened or coverage thresholds lowered
  • All remaining functionality properly tested

Phase 4: Code Quality

Implementation Approach: High quality

  • ✅ Hook design follows consistent stdin pattern
  • ✅ Proper error handling and JSON parsing
  • ✅ Clean circular dependency resolution
  • ✅ Backward compatibility maintained

Final Recommendation

## Root Cause Analysis
**Problem Being Solved**: 5-file circular dependency successfully resolved
**Why This Approach**: Clean extraction to dedicated test-filter module with re-export for compatibility
**Risk Assessment**: LOW-MODERATE - solid implementation with excellent security hooks

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: **APPROVE**
- Reasoning: The PR successfully resolves the circular dependency, adds valuable security hooks, and maintains backward compatibility. The hooks are particularly well-designed for preventing common development issues. The only minor issue is a slightly misleading PR description mentioning `test-utils.js` when the actual implementation uses `test-filter.js`.

@claude
Copy link

claude bot commented Mar 9, 2026

Claude Code is working…

I'll analyze this and get back to you.

View job run

@carlos-alm carlos-alm merged commit a8fb530 into main Mar 9, 2026
14 of 15 checks passed
@carlos-alm carlos-alm deleted the fix/break-cycle-remove-dead-export branch March 9, 2026 05:36
@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 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