feat: add pre-commit hooks for cycles, dead exports, signature warnings, and PR body guard#376
feat: add pre-commit hooks for cycles, dead exports, signature warnings, and PR body guard#376carlos-alm wants to merge 27 commits intomainfrom
Conversation
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
…omplete changelog
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
Impact: 1 functions changed, 1 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
Greptile SummaryThis PR introduces four Claude Code pre-commit hooks ( Issues found:
Confidence Score: 3/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[git commit triggered] --> B[guard-git.sh\nBlocks dangerous commands]
A --> C[check-cycles.sh\nPreToolUse / Bash]
A --> D[check-dead-exports.sh\nPreToolUse / Bash]
A --> E[warn-signature-changes.sh\nPreToolUse / Bash]
C --> C1{codegraph DB\nexists?}
C1 -- No --> CX[exit 0 / allow]
C1 -- Yes --> C2[codegraph check\n--staged --json]
C2 --> C3{cycles involve\nsession-edited files?}
C3 -- No --> CX
C3 -- Yes --> C4[deny: list cycles]
D --> D1{session-edits.log\nexists?}
D1 -- No --> DX[exit 0 / allow]
D1 -- Yes --> D2[for each staged src/ file\nedited this session]
D2 --> D3[codegraph exports\nfile --unused --json]
D3 --> D4{consumerCount\n=== 0?}
D4 -- No --> DX
D4 -- Yes --> D5[deny: list dead exports]
E --> E1{codegraph DB\nexists?}
E1 -- No --> EX[exit 0 / allow]
E1 -- Yes --> E2[checkData staged\nsignatures predicate]
E2 --> E3{violations?}
E3 -- No --> EX
E3 -- Yes --> E4[enrich: role +\ntransitive callers]
E4 --> E5[allow + additionalContext\nHIGH / MEDIUM / low risk]
B2[gh pr create triggered] --> F[guard-pr-body.sh\nPreToolUse / Bash]
F --> F1[extract .command\nfield via jq]
F1 --> F2{command contains\n'generated with'?}
F2 -- No --> FX[exit 0 / allow]
F2 -- Yes --> F3[exit 2 / BLOCK]
Last reviewed commit: 7d17382 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
.claude/hooks/guard-pr-body.sh
Outdated
| if echo "$input" | grep -qi 'generated with'; then | ||
| echo "BLOCK: Remove any 'Generated with ...' line from the PR body." >&2 | ||
| exit 2 |
There was a problem hiding this comment.
False-positive risk: hook checks entire CLAUDE_TOOL_INPUT JSON, not just --body
CLAUDE_TOOL_INPUT is the full JSON object passed to the Bash tool, which includes both the command field and the description field (Claude's internal task label). This means grep -qi 'generated with' will match on Claude's task description (e.g., "Create PR for changes generated with the automation script"), blocking the PR even when the body is clean.
To fix this, extract only the value of the --body argument from the command string:
# Extract just the --body value from the command
body=$(echo "$input" | jq -r '.command // ""' | grep -oP '(?<=--body )['\''"]?\K[^'\''"\n]+')
if echo "$body" | grep -qi 'generated with'; then
echo "BLOCK: Remove any 'Generated with ...' line from the PR body." >&2
exit 2
fiOr, at minimum, parse only the command key before grepping:
input=$(echo "$CLAUDE_TOOL_INPUT" | jq -r '.command // ""')
if echo "$input" | grep -qi 'generated with'; then
# ... block ...
fiThere was a problem hiding this comment.
Fixed — the hook now extracts only the \ field via \ before grepping, so the \ field no longer triggers false positives.
| export function sequenceData(name, dbPath, opts = {}) { | ||
| const db = openReadonlyOrFail(dbPath); | ||
| const maxDepth = opts.depth || 10; | ||
| const noTests = opts.noTests || false; | ||
| const withDataflow = opts.dataflow || false; | ||
|
|
||
| // Phase 1: Direct LIKE match | ||
| let matchNode = findMatchingNodes(db, name, opts)[0] ?? null; | ||
|
|
||
| // Phase 2: Prefix-stripped matching | ||
| if (!matchNode) { | ||
| for (const prefix of FRAMEWORK_ENTRY_PREFIXES) { | ||
| matchNode = findMatchingNodes(db, `${prefix}${name}`, opts)[0] ?? null; | ||
| if (matchNode) break; | ||
| } | ||
| } | ||
|
|
||
| if (!matchNode) { | ||
| db.close(); | ||
| return { | ||
| entry: null, | ||
| participants: [], | ||
| messages: [], | ||
| depth: maxDepth, | ||
| totalMessages: 0, | ||
| truncated: false, | ||
| }; | ||
| } | ||
|
|
||
| const entry = { | ||
| name: matchNode.name, | ||
| file: matchNode.file, | ||
| kind: matchNode.kind, | ||
| line: matchNode.line, | ||
| }; | ||
|
|
||
| // BFS forward — track edges, not just nodes | ||
| const visited = new Set([matchNode.id]); | ||
| let frontier = [matchNode.id]; | ||
| const messages = []; | ||
| const fileSet = new Set([matchNode.file]); | ||
| const idToNode = new Map(); | ||
| idToNode.set(matchNode.id, matchNode); | ||
| let truncated = false; | ||
|
|
||
| const getCallees = db.prepare( | ||
| `SELECT DISTINCT n.id, n.name, n.kind, n.file, n.line | ||
| FROM edges e JOIN nodes n ON e.target_id = n.id | ||
| WHERE e.source_id = ? AND e.kind = 'calls'`, | ||
| ); | ||
|
|
||
| for (let d = 1; d <= maxDepth; d++) { | ||
| const nextFrontier = []; | ||
|
|
||
| for (const fid of frontier) { | ||
| const callees = getCallees.all(fid); | ||
|
|
||
| const caller = idToNode.get(fid); | ||
|
|
||
| for (const c of callees) { | ||
| if (noTests && isTestFile(c.file)) continue; | ||
|
|
||
| // Always record the message (even for visited nodes — different caller path) | ||
| fileSet.add(c.file); | ||
| messages.push({ | ||
| from: caller.file, | ||
| to: c.file, | ||
| label: c.name, | ||
| type: 'call', | ||
| depth: d, | ||
| }); | ||
|
|
||
| if (visited.has(c.id)) continue; | ||
|
|
||
| visited.add(c.id); | ||
| nextFrontier.push(c.id); | ||
| idToNode.set(c.id, c); | ||
| } | ||
| } | ||
|
|
||
| frontier = nextFrontier; | ||
| if (frontier.length === 0) break; | ||
|
|
||
| if (d === maxDepth && frontier.length > 0) { | ||
| // Only mark truncated if at least one frontier node has further callees | ||
| const hasMoreCalls = frontier.some((fid) => getCallees.all(fid).length > 0); | ||
| if (hasMoreCalls) truncated = true; | ||
| } | ||
| } | ||
|
|
||
| // Dataflow annotations: add return arrows | ||
| if (withDataflow && messages.length > 0) { | ||
| const hasTable = db | ||
| .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='dataflow'") | ||
| .get(); | ||
|
|
||
| if (hasTable) { | ||
| // Build name|file lookup for O(1) target node access | ||
| const nodeByNameFile = new Map(); | ||
| for (const n of idToNode.values()) { | ||
| nodeByNameFile.set(`${n.name}|${n.file}`, n); | ||
| } | ||
|
|
||
| const getReturns = db.prepare( | ||
| `SELECT d.expression FROM dataflow d | ||
| WHERE d.source_id = ? AND d.kind = 'returns'`, | ||
| ); | ||
| const getFlowsTo = db.prepare( | ||
| `SELECT d.expression FROM dataflow d | ||
| WHERE d.target_id = ? AND d.kind = 'flows_to' | ||
| ORDER BY d.param_index`, | ||
| ); | ||
|
|
||
| // For each called function, check if it has return edges | ||
| const seenReturns = new Set(); | ||
| for (const msg of [...messages]) { | ||
| if (msg.type !== 'call') continue; | ||
| const targetNode = nodeByNameFile.get(`${msg.label}|${msg.to}`); | ||
| if (!targetNode) continue; | ||
|
|
||
| const returnKey = `${msg.to}->${msg.from}:${msg.label}`; | ||
| if (seenReturns.has(returnKey)) continue; | ||
|
|
||
| const returns = getReturns.all(targetNode.id); | ||
|
|
||
| if (returns.length > 0) { | ||
| seenReturns.add(returnKey); | ||
| const expr = returns[0].expression || 'result'; | ||
| messages.push({ | ||
| from: msg.to, | ||
| to: msg.from, | ||
| label: expr, | ||
| type: 'return', | ||
| depth: msg.depth, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Annotate call messages with parameter names | ||
| for (const msg of messages) { | ||
| if (msg.type !== 'call') continue; | ||
| const targetNode = nodeByNameFile.get(`${msg.label}|${msg.to}`); | ||
| if (!targetNode) continue; | ||
|
|
||
| const params = getFlowsTo.all(targetNode.id); | ||
|
|
||
| if (params.length > 0) { | ||
| const paramNames = params | ||
| .map((p) => p.expression) | ||
| .filter(Boolean) | ||
| .slice(0, 3); | ||
| if (paramNames.length > 0) { | ||
| msg.label = `${msg.label}(${paramNames.join(', ')})`; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Sort messages by depth, then call before return | ||
| messages.sort((a, b) => { | ||
| if (a.depth !== b.depth) return a.depth - b.depth; | ||
| if (a.type === 'call' && b.type === 'return') return -1; | ||
| if (a.type === 'return' && b.type === 'call') return 1; | ||
| return 0; | ||
| }); | ||
|
|
||
| // Build participant list from files | ||
| const aliases = buildAliases([...fileSet]); | ||
| const participants = [...fileSet].map((file) => ({ | ||
| id: aliases.get(file), | ||
| label: file.split('/').pop(), | ||
| file, | ||
| })); | ||
|
|
||
| // Sort participants: entry file first, then alphabetically | ||
| participants.sort((a, b) => { | ||
| if (a.file === entry.file) return -1; | ||
| if (b.file === entry.file) return 1; | ||
| return a.file.localeCompare(b.file); | ||
| }); | ||
|
|
||
| // Replace file paths with alias IDs in messages | ||
| for (const msg of messages) { | ||
| msg.from = aliases.get(msg.from); | ||
| msg.to = aliases.get(msg.to); | ||
| } | ||
|
|
||
| db.close(); | ||
|
|
||
| const base = { | ||
| entry, | ||
| participants, | ||
| messages, | ||
| depth: maxDepth, | ||
| totalMessages: messages.length, | ||
| truncated, | ||
| }; | ||
| const result = paginateResult(base, 'messages', { limit: opts.limit, offset: opts.offset }); | ||
| if (opts.limit !== undefined || opts.offset !== undefined) { | ||
| const activeFiles = new Set(result.messages.flatMap((m) => [m.from, m.to])); | ||
| result.participants = result.participants.filter((p) => activeFiles.has(p.id)); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Database connection not closed on error paths
db.close() is called only at line 104 (early return) and line 274 (happy path). If any SQLite query throws (bad schema, corrupt DB, unexpected BFS error, etc.), the connection is never closed, leaking the file handle for the process lifetime.
Wrap the core logic in try/finally:
export function sequenceData(name, dbPath, opts = {}) {
let db;
try {
db = openReadonlyOrFail(dbPath);
const maxDepth = opts.depth || 10;
// ... rest of logic ...
return result;
} finally {
db?.close();
}
}There was a problem hiding this comment.
Fixed — wrapped sequenceData in try { ... } finally { db.close(); } so the connection is always closed, including on error paths. Removed the redundant db.close() calls from the early return and happy path.
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.
carlos-alm
changed the title
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.
carlos-alm
changed the title
Resolve merge conflicts in CHANGELOG.md and src/queries.js. Fix guard-pr-body.sh: extract only the `command` field from CLAUDE_TOOL_INPUT JSON before grepping, avoiding false positives on the description field. Fix sequence.js: wrap sequenceData in try/finally so db.close() runs on all code paths (error or success). Impact: 39 functions changed, 60 affected
- Remove duplicate `totalUnused` declaration in queries.js (merge artifact) - Remove duplicate `unused` property in cli.js exports command - Fix indentation of try/finally block in sequence.js Impact: 2 functions changed, 6 affected
Impact: 1 functions changed, 1 affected
|
Claude finished @carlos-alm's task —— View job PR Code Review ⭐⭐☆☆☆ (2/5) — REQUEST CHANGESReview Checklist
Root Cause AnalysisProblem Being Solved: Adding automated quality gates to prevent common issues: circular dependency commits, dead code accumulation, breaking signature changes, and AI-generated PR content. Why This Approach: Pre-commit hooks provide real-time feedback and prevention, which is justified. Risk Assessment: High — this PR violates the "One PR = one concern" rule and has critical implementation bugs. Critical Concerns1. DATABASE CONNECTION LEAK 🚨File: Current code: export function sequenceData(name, dbPath, opts = {}) {
const db = openReadonlyOrFail(dbPath); // Line 87
try {
// ... 200 lines of SQLite queries that could throw
return result;
} finally {
db.close(); // Line 289
}
}Problem: If any query between lines 88-287 throws, the connection is never closed. The fix is already correctly implemented with try/finally, but Greptile's concern about missing error handling was valid in an earlier version. Status: ✅ FIXED - Author has already implemented proper try/finally pattern. 2. PR BODY GUARD HOOK FIXEDFile: Current code: cmd=$(echo "$input" | jq -r '.command // ""')
echo "$cmd" | grep -qi 'gh pr create' || exit 0
if echo "$cmd" | grep -qi 'generated with'; thenStatus: ✅ FIXED - Author has correctly isolated the command field before grepping. 3. ARCHITECTURE VIOLATION
|
|
@greptileai review |
Merge from main introduced a second .option('--unused', ...) on the
exports command, causing commander to throw at startup.
| .option('--unused', 'Show only exports with zero consumers (dead exports)') | ||
| .option('--ndjson', 'Newline-delimited JSON output') | ||
| .option('--unused', 'Show only exports with zero consumers') | ||
| .action((file, opts) => { |
There was a problem hiding this comment.
Duplicate --unused option declaration crashes the CLI
--unused is declared twice on the exports command — once on line 279 (new) and again on line 281 (the original, which was never removed). Commander.js throws a hard error when a duplicate option flag is registered: "Cannot add option with the same flag as an existing option." This means every invocation of codegraph exports will crash at startup — including the check-dead-exports.sh hook that calls node src/cli.js exports "$file" --unused --json -T.
Remove the old declaration on line 281:
| .option('--unused', 'Show only exports with zero consumers (dead exports)') | |
| .option('--ndjson', 'Newline-delimited JSON output') | |
| .option('--unused', 'Show only exports with zero consumers') | |
| .action((file, opts) => { | |
| .option('--unused', 'Show only exports with zero consumers (dead exports)') | |
| .option('--ndjson', 'Newline-delimited JSON output') | |
| .action((file, opts) => { |
There was a problem hiding this comment.
Fixed — the duplicate --unused option was removed in a previous commit. The exports command now declares it exactly once (line 279), and the CLI loads without errors.
| 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) { |
There was a problem hiding this comment.
db.prepare() called inside the BFS inner loop
The db.prepare() call for the callers query is inside three nested loops: for (const v of sigPred.violations) → for (let d = 0; d < 3; d++) → for (const fid of frontier). A new prepared statement object is allocated on every BFS iteration for every violation. For a function with many callers this can be O(violations × depth × frontier) prepare calls.
Hoist the prepared statement to just outside the outer violations loop (or outside all loops if it is the same query for every symbol):
const getCallers = 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\''
);
for (const v of sigPred.violations) {
// ...
for (const fid of frontier) {
const callers = getCallers.all(fid);
// ...
}
}There was a problem hiding this comment.
Fixed — hoisted both db.prepare() calls (stmtNode and stmtCallers) outside the violation loop. Each statement is now prepared once and reused across all iterations, reducing allocations from O(violations × depth × frontier) to O(1).
|
Claude finished @carlos-alm's task —— View job PR Code Review ⭐⭐☆☆☆ (2/5) — REQUEST CHANGESReview Checklist
Root Cause AnalysisProblem Being Solved: Adding automated quality gates to prevent common issues: circular dependency commits, dead code accumulation, breaking signature changes, and AI-generated PR content. Why This Approach: Pre-commit hooks provide real-time feedback and prevention, which is justified. Risk Assessment: High — this PR violates the "One PR = one concern" rule and has critical implementation bugs. Critical Concerns1. DATABASE CONNECTION LEAK 🚨File: Current code: export function sequenceData(name, dbPath, opts = {}) {
const db = openReadonlyOrFail(dbPath); // Line 87
try {
// ... 200 lines of SQLite queries that could throw
return result;
} finally {
db.close(); // Line 289
}
}Problem: If any query between lines 88-287 throws, the connection is never closed. The fix is already correctly implemented with try/finally, but Greptile's concern about missing error handling was valid in an earlier version. Status: ✅ FIXED - Author has already implemented proper try/finally pattern. 2. PR BODY GUARD HOOK FIXEDFile: Current code: cmd=$(echo "$input" | jq -r '.command // ""')
echo "$cmd" | grep -qi 'gh pr create' || exit 0
if echo "$cmd" | grep -qi 'generated with'; thenStatus: ✅ FIXED - Author has correctly isolated the command field before grepping. 3. ARCHITECTURE VIOLATION
|
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.
Resolve merge conflicts in CHANGELOG.md and src/queries.js. Fix guard-pr-body.sh false-positive by extracting only the command field from CLAUDE_TOOL_INPUT. Wrap sequenceData in try/finally so db.close() runs on all code paths. Remove duplicate --unused option and property from cli.js exports command. Impact: 39 functions changed, 60 affected
carlos-alm
force-pushed
the
chore/guard-pr-body
branch
from
59cfe40 to
4c924da
Compare
|
Claude finished @carlos-alm's task —— View job PR Code Review AnalysisReview Checklist
Root Cause AnalysisProblem Being Solved: Adding automated quality gates to prevent common issues: circular dependency commits, dead code accumulation, breaking signature changes, and AI-generated PR content. Why This Approach: Pre-commit hooks provide real-time feedback and prevention, which is justified for preventing quality issues at commit time. Risk Assessment: CRITICAL — This PR violates the explicit "One PR = one concern" policy from Critical Concerns1. ARCHITECTURE VIOLATION 🚨Issue: This PR bundles 4+ distinct concerns that should be separate PRs per
Evidence: 61 files changed across completely unrelated systems (hooks, CLI, native Rust engine, docs, tests) Policy Violation:
2. GOOD: Critical Bugs Fixed ✅Greptile Issues Resolved:
3. PERFORMANCE CONCERNFile: # Inside 3 nested loops:
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);Impact: O(violations × depth × frontier) prepare statements instead of single prepare + multiple execute calls. 4. HOOK TIMEOUT CONCERNSFile: "timeout": 15 // check-cycles.sh
"timeout": 30 // check-dead-exports.sh
"timeout": 15 // warn-signature-changes.shTotal: 60+ seconds of additional commit overhead. Could significantly slow development workflow. Configuration Changes Analysis
Test Integrity Assessment
Code Quality FindingsPositive Aspects ✅
Minor Issues
Final Recommendation
Required Actions
Would Approve When SplitOnce properly separated, both the pre-commit hooks and sequence features are high-quality implementations that would merit ⭐⭐⭐⭐☆ individually. |
…e hook Merge main into chore/guard-pr-body, resolving the cli.js conflict by adopting main's QUERY_OPTS() pattern while keeping the improved --unused description from this branch. Address greptile review: hoist db.prepare() calls outside the BFS inner loops in warn-signature-changes.sh to avoid redundant prepared statement allocations (O(violations × depth × frontier) → O(1)). Impact: 37 functions changed, 133 affected
Resolve conflicts in src/cli.js (keep QUERY_OPTS pattern from main), .claude/hooks/warn-signature-changes.sh (keep hoisted db.prepare), and src/queries.js (keep CLI functions in queries-cli.js).
|
Claude finished @carlos-alm's task —— View job PR Code Review AnalysisReview Checklist
Root Cause AnalysisProblem Being Solved: Adding automated quality gates to prevent common issues: circular dependency commits, dead code accumulation, breaking signature changes, and AI-generated PR content. Why This Approach: Pre-commit hooks provide real-time feedback and prevention, which is justified. Risk Assessment: High — this PR violates the "One PR = one concern" rule and has critical implementation bugs. Critical Concerns1. DATABASE CONNECTION LEAK 🚨File: Current code: export function sequenceData(name, dbPath, opts = {}) {
const db = openReadonlyOrFail(dbPath); // Line 87
try {
// ... 200 lines of SQLite queries that could throw
return result;
} finally {
db.close(); // Line 289
}
}Problem: If any query between lines 88-287 throws, the connection is never closed. The fix is already correctly implemented with try/finally, but Greptile's concern about missing error handling was valid in an earlier version. Status: ✅ FIXED - Author has already implemented proper try/finally pattern. 2. PR BODY GUARD HOOK FIXEDFile: Current code: cmd=$(echo "$input" | jq -r '.command // ""')
echo "$cmd" | grep -qi 'gh pr create' || exit 0
if echo "$cmd" | grep -qi 'generated with'; thenStatus: ✅ FIXED - Author has correctly isolated the command field before grepping. 3. ARCHITECTURE VIOLATION
|
|
@greptileai review |
Impact: 1 functions changed, 23 affected
- findMatchingNodes now accepts opts.kinds to override default FUNCTION_KINDS - flowData passes CORE_SYMBOL_KINDS (all 10 kinds) so flow command can trace from interfaces, types, structs, etc. — restoring pre-refactor behavior - guard-pr-body.sh switched from $CLAUDE_TOOL_INPUT to stdin pattern, consistent with check-cycles.sh and other hooks Impact: 2 functions changed, 40 affected
|
Claude finished @carlos-alm's task —— View job PR Code Review AnalysisReview Checklist
Root Cause AnalysisProblem Being Solved: Adding automated quality gates to prevent common issues: circular dependency commits, dead code accumulation, breaking signature changes, and AI-generated PR content. Why This Approach: Pre-commit hooks provide real-time feedback and prevention, which is justified for preventing quality issues at commit time. Risk Assessment: CRITICAL — This PR violates the explicit "One PR = one concern" policy from Critical Concerns1. ARCHITECTURE VIOLATION 🚨Issue: This PR bundles 4+ distinct concerns that should be separate PRs per
Evidence: 61 files changed across completely unrelated systems (hooks, CLI, native Rust engine, docs, tests) Policy Violation:
2. GOOD: Critical Bugs Fixed ✅Greptile Issues Resolved:
3. PERFORMANCE CONCERNFile: # Inside 3 nested loops:
const stmtCallers = 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\''
);Impact: O(violations × depth × frontier) prepare statements instead of single prepare + multiple execute calls. 4. HOOK TIMEOUT CONCERNSFile: "timeout": 15 // check-cycles.sh
"timeout": 30 // check-dead-exports.sh
"timeout": 15 // warn-signature-changes.shTotal: 60+ seconds of additional commit overhead. Could significantly slow development workflow. Configuration Changes Analysis
Test Integrity Assessment
Code Quality FindingsPositive Aspects ✅
Minor Issues
Final Recommendation
Required Actions
Would Approve When SplitOnce properly separated, both the pre-commit hooks and sequence features are high-quality implementations that would merit ⭐⭐⭐⭐☆ individually. |
| cmd=$(echo "$INPUT" | node -e " | ||
| let d=''; | ||
| process.stdin.on('data',c=>d+=c); | ||
| process.stdin.on('end',()=>{ |
There was a problem hiding this comment.
The guard scans the entire gh pr create command string for generated with, not just the --body value. This means a PR whose title contains that phrase (e.g. --title "Report generated with nightly CI") would be incorrectly blocked, even though the body is clean.
Extract just the --body argument before testing:
| process.stdin.on('end',()=>{ | |
| if echo "$cmd" | grep -oP "(?<=--body )['\"]?\K[^'\"\\n]+" | grep -qi 'generated with'; then |
Or more robustly, parse the body via Node:
body=$(echo "$cmd" | node -e "
let d=''; process.stdin.on('data',c=>d+=c);
process.stdin.on('end',()=>{
const m=d.match(/--body\s+['\"]([^'\"]*)['\"]/) || d.match(/--body\s+(\S+)/);
if(m) process.stdout.write(m[1]);
});
" 2>/dev/null) || true
if echo "\$body" | grep -qi 'generated with'; thenThis avoids false positives from other flag values.
…essage When the SQL query fails (e.g. invalid --kind or missing table), the catch block now returns an error field. The CLI formatter checks this field first, showing the real error instead of "No symbols found. Run codegraph build first." Impact: 2 functions changed, 4 affected
|
Claude finished @carlos-alm's task —— View job PR Code Review AnalysisReview Checklist
Root Cause AnalysisProblem Being Solved: Adding automated quality gates to prevent common issues: circular dependency commits, dead code accumulation, breaking signature changes, and AI-generated PR content. Why This Approach: Pre-commit hooks provide real-time feedback and prevention, which is justified for preventing quality issues at commit time. Risk Assessment: CRITICAL — This PR violates the explicit "One PR = one concern" policy from Critical Concerns1. ARCHITECTURE VIOLATION 🚨Issue: This PR bundles 4+ distinct concerns that should be separate PRs per
Evidence: 13 files changed across completely unrelated systems (hooks, CLI, native engine, docs, tests) Policy Violation:
2. GOOD: Critical Bugs Fixed ✅Greptile Issues Resolved:
3. PERFORMANCE CONCERNFile: # Inside 3 nested loops:
const stmtCallers = 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\''
);Impact: O(violations × depth × frontier) prepare statements instead of single prepare + multiple execute calls. 4. HOOK TIMEOUT CONCERNSFile: "timeout": 15 // check-cycles.sh
"timeout": 30 // check-dead-exports.sh
"timeout": 15 // warn-signature-changes.shTotal: 60+ seconds of additional commit overhead. Could significantly slow development workflow. Configuration Changes Analysis
Test Integrity Assessment
Code Quality FindingsPositive Aspects ✅
Minor Issues
Final Recommendation
Required Actions
Would Approve When SplitOnce properly separated, both the pre-commit hooks and sequence features are high-quality implementations that would merit ⭐⭐⭐⭐☆ individually. |
|
Closing in favor of focused PRs per review feedback:
CHANGELOG native engine entries dropped (belong with PR #344). |
1 participant
Summary
check-cycles.shhook: blocks commits if circular dependencies involve files edited in the current sessioncheck-dead-exports.shhook: blocks commits if files you edited have exports with zero consumers (dead code)warn-signature-changes.shhook: non-blocking warning when function signatures are modified, with risk level based on symbol role (core/utility/leaf) and transitive caller countguard-pr-body.shhook: blocks PR creation if the body contains "generated with"--unusedflag on theexportsCLI command, addingtotalUnusedto all output formats (JSON, CLI).claude/settings.jsonrecommended-practices.mdTest plan
npm testpasses (exports + check integration tests)npm run lintclean (pre-existing warning only)codegraph exports <file> --unused --json -Tcorrectly filters to zero-consumer exportsgit commitand correctly scope to session-edited filesadditionalContext(tested manually)