refactor(optimize): correctness, constants, and real tests

Phase 1 hardening pass.

Bug fixes:
- Move cwd/version collection inside date-range filter. 7d and 30d
  now produce different findings for filesystem detectors.
- detectGhostSkills threshold aligned with peer detectors.
- detectUnusedMcp gets 24-hour grace period via config file mtime so
  newly added servers are not flagged as unused.
- detectCacheBloat replaces hardcoded 50K baseline with user-derived
  p25 of cache writes. Flags only when median exceeds 1.4x baseline.
- detectBashBloat scans user shell profiles instead of the auditor's
  process.env.
- @-import pattern requires ./ ../ or / to avoid matching email
  addresses or npm scopes.
- Command usage pattern requires leading whitespace/start-of-line
  before /cmd so path references like /tmp are not counted as usage.
- AVG_TOKENS_PER_READ lowered from 1500 to 600 and
  CLAUDEMD_TOKENS_PER_LINE lowered from 25 to 13 for realistic
  prose/config sizing.

Code quality:
- Every magic number extracted to named module-scope constants.
- Dead code removed (IMPACT_ORDER, unused stat import).
- Shared loadMcpConfigs helper deduplicates config walking.
- Shared shortHomePath, isReadTool, inRange helpers.
- All detectors and computeHealth exported for real tests.
- Ghost detectors run in parallel via Promise.all.
- Cost rate defaults to 0 when unknown so UI can suppress instead of
  showing fabricated numbers.

Tests:
- Replaced 17 fake tests that re-implemented detector logic with
  26 tests importing and exercising the real exports.
- Cover threshold boundaries, impact scaling, edge cases.
- 121 tests pass.

UX header: "Setup" renamed to "Health", issue count shown inline.
CLAUDE.md: adds rule against "steal/copy/rip-off" wording in
public-facing text.
This commit is contained in:
AgentSeal 2026-04-16 06:30:15 -07:00
parent b88f2cd730
commit be45045fd8
3 changed files with 600 additions and 537 deletions

View file

@ -75,3 +75,10 @@ gh pr comment <number> --body "Merged, thanks!"
- NEVER include personal names or usernames in commits
- Small, focused commits. One feature per commit
- Test locally before every commit
### Public-facing language (commits, PRs, release notes, README)
- Commits and release notes are public. Write like you'd publish them.
- NEVER use words like "steal", "stealing", "copy", "rip off", "inspired by" in commit messages
- Describe what the code does, not where ideas came from
- If you must credit prior art, do it in code comments or docs, not commit messages
- No snark, no filler, no self-deprecation. Treat each commit as a product statement

File diff suppressed because it is too large Load diff

View file

@ -1,318 +1,258 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
import { existsSync, readFileSync } from 'fs'
import { homedir } from 'os'
import { join } from 'path'
import { describe, it, expect } from 'vitest'
vi.mock('fs', async () => {
const actual = await vi.importActual<typeof import('fs')>('fs')
return { ...actual, existsSync: vi.fn(), readFileSync: vi.fn() }
})
import {
detectJunkReads,
detectDuplicateReads,
detectLowReadEditRatio,
detectCacheBloat,
detectBloatedClaudeMd,
detectMissingClaudeignore,
computeHealth,
type ToolCall,
type ApiCallMeta,
type WasteFinding,
} from '../src/optimize.js'
import type { ProjectSummary } from '../src/types.js'
const mockExistsSync = vi.mocked(existsSync)
const mockReadFileSync = vi.mocked(readFileSync)
type ToolCall = {
name: string
input: Record<string, unknown>
sessionId: string
project: string
function call(name: string, input: Record<string, unknown>, sessionId = 's1', project = 'p1'): ToolCall {
return { name, input, sessionId, project }
}
// Re-implement detector logic for isolated testing
// This avoids importing the module which has side-effect imports
function detectJunkReadsLogic(calls: ToolCall[]) {
const JUNK_PATTERN = /\/(node_modules|\.git|dist|build|__pycache__|\.next|\.nuxt|\.output|coverage|\.cache|\.tsbuildinfo|\.venv|venv|\.svn|\.hg)\//
const readCalls = calls.filter(c => c.name === 'Read' || c.name === 'FileReadTool')
const dirCounts = new Map<string, number>()
let total = 0
for (const call of readCalls) {
const fp = call.input.file_path as string | undefined
if (!fp || !JUNK_PATTERN.test(fp)) continue
total++
const dirs = ['node_modules', '.git', 'dist', 'build', '__pycache__', '.next', '.venv', 'venv']
for (const d of dirs) {
if (fp.includes(`/${d}/`)) { dirCounts.set(d, (dirCounts.get(d) ?? 0) + 1); break }
}
}
return { total, dirCounts }
function emptyProjects(): ProjectSummary[] {
return []
}
function detectDuplicateReadsLogic(calls: ToolCall[]) {
const JUNK_PATTERN = /\/(node_modules|\.git|dist|build|__pycache__|\.next)\//
const readCalls = calls.filter(c => c.name === 'Read' || c.name === 'FileReadTool')
const sessionFiles = new Map<string, Map<string, number>>()
for (const call of readCalls) {
const fp = call.input.file_path as string | undefined
if (!fp || JUNK_PATTERN.test(fp)) continue
const key = `${call.project}:${call.sessionId}`
if (!sessionFiles.has(key)) sessionFiles.set(key, new Map())
const fm = sessionFiles.get(key)!
fm.set(fp, (fm.get(fp) ?? 0) + 1)
}
let totalDuplicates = 0
for (const fm of sessionFiles.values()) {
for (const [, count] of fm) {
if (count > 1) totalDuplicates += count - 1
}
}
return totalDuplicates
}
describe('optimize: junk reads detection', () => {
it('detects node_modules reads', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/node_modules/foo/index.js' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/node_modules/bar/package.json' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/node_modules/baz/lib.js' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/src/main.ts' }, sessionId: 's1', project: 'p1' },
describe('detectJunkReads', () => {
it('returns null below minimum threshold', () => {
const calls = [
call('Read', { file_path: '/x/node_modules/a.js' }),
call('Read', { file_path: '/x/node_modules/b.js' }),
]
const result = detectJunkReadsLogic(calls)
expect(result.total).toBe(3)
expect(result.dirCounts.get('node_modules')).toBe(3)
expect(detectJunkReads(calls)).toBeNull()
})
it('detects .git reads', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/.git/config' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/.git/HEAD' }, sessionId: 's1', project: 'p1' },
it('flags when threshold is met', () => {
const calls = [
call('Read', { file_path: '/x/node_modules/a.js' }),
call('Read', { file_path: '/x/node_modules/b.js' }),
call('Read', { file_path: '/x/.git/config' }),
]
const result = detectJunkReadsLogic(calls)
expect(result.total).toBe(2)
expect(result.dirCounts.get('.git')).toBe(2)
const finding = detectJunkReads(calls)
expect(finding).not.toBeNull()
expect(finding!.impact).toBe('low')
})
it('detects mixed junk directories', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/node_modules/a.js' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/dist/bundle.js' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/.venv/lib/python/site.py' }, sessionId: 's1', project: 'p1' },
]
const result = detectJunkReadsLogic(calls)
expect(result.total).toBe(3)
expect(result.dirCounts.get('node_modules')).toBe(1)
expect(result.dirCounts.get('dist')).toBe(1)
expect(result.dirCounts.get('.venv')).toBe(1)
it('scales impact with read count', () => {
const make = (n: number) => Array.from({ length: n }, (_, i) =>
call('Read', { file_path: `/x/node_modules/file-${i}.js` })
)
expect(detectJunkReads(make(25))!.impact).toBe('high')
expect(detectJunkReads(make(10))!.impact).toBe('medium')
})
it('ignores non-junk paths', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/src/index.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/README.md' }, sessionId: 's1', project: 'p1' },
const calls = [
call('Read', { file_path: '/x/src/a.ts' }),
call('Read', { file_path: '/x/src/b.ts' }),
call('Read', { file_path: '/x/README.md' }),
]
const result = detectJunkReadsLogic(calls)
expect(result.total).toBe(0)
expect(detectJunkReads(calls)).toBeNull()
})
it('ignores non-Read tools', () => {
const calls: ToolCall[] = [
{ name: 'Edit', input: { file_path: '/project/node_modules/foo.js' }, sessionId: 's1', project: 'p1' },
{ name: 'Bash', input: { command: 'ls node_modules' }, sessionId: 's1', project: 'p1' },
it('ignores non-read tools', () => {
const calls = [
call('Edit', { file_path: '/x/node_modules/a.js' }),
call('Bash', { command: 'ls node_modules' }),
call('Grep', { pattern: 'test', path: '/x/node_modules' }),
]
const result = detectJunkReadsLogic(calls)
expect(result.total).toBe(0)
expect(detectJunkReads(calls)).toBeNull()
})
it('handles missing file_path', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: {}, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: null as unknown as string }, sessionId: 's1', project: 'p1' },
it('handles missing file_path gracefully', () => {
const calls = [
call('Read', {}),
call('Read', { file_path: null as unknown as string }),
]
const result = detectJunkReadsLogic(calls)
expect(result.total).toBe(0)
expect(detectJunkReads(calls)).toBeNull()
})
it('builds .claudeignore content from detected + common extras', () => {
const calls = Array.from({ length: 5 }, () => call('Read', { file_path: '/x/node_modules/a.js' }))
const finding = detectJunkReads(calls)!
expect(finding.fix.type).toBe('file-content')
if (finding.fix.type === 'file-content') {
expect(finding.fix.content).toContain('node_modules')
}
})
})
describe('optimize: duplicate reads detection', () => {
it('detects files read multiple times in same session', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/src/main.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/src/main.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/src/main.ts' }, sessionId: 's1', project: 'p1' },
describe('detectDuplicateReads', () => {
it('counts same file read multiple times in same session', () => {
const calls = [
...Array.from({ length: 4 }, () => call('Read', { file_path: '/src/a.ts' }, 's1')),
...Array.from({ length: 4 }, () => call('Read', { file_path: '/src/b.ts' }, 's1')),
]
expect(detectDuplicateReadsLogic(calls)).toBe(2)
const finding = detectDuplicateReads(calls)
expect(finding).not.toBeNull()
})
it('does not count reads across different sessions', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/src/main.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/src/main.ts' }, sessionId: 's2', project: 'p1' },
it('does not count across sessions', () => {
const calls = [
call('Read', { file_path: '/src/a.ts' }, 's1'),
call('Read', { file_path: '/src/a.ts' }, 's2'),
call('Read', { file_path: '/src/a.ts' }, 's3'),
]
expect(detectDuplicateReadsLogic(calls)).toBe(0)
expect(detectDuplicateReads(calls)).toBeNull()
})
it('excludes junk directory reads from duplicate count', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/node_modules/foo.js' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/node_modules/foo.js' }, sessionId: 's1', project: 'p1' },
]
expect(detectDuplicateReadsLogic(calls)).toBe(0)
it('excludes junk directory reads', () => {
const calls = Array.from({ length: 10 }, () =>
call('Read', { file_path: '/x/node_modules/foo.js' }, 's1')
)
expect(detectDuplicateReads(calls)).toBeNull()
})
it('counts duplicates per file independently', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/a.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/a.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/b.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/b.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/b.ts' }, sessionId: 's1', project: 'p1' },
it('returns null for single reads', () => {
const calls = [
call('Read', { file_path: '/src/a.ts' }, 's1'),
call('Read', { file_path: '/src/b.ts' }, 's1'),
]
expect(detectDuplicateReadsLogic(calls)).toBe(3)
})
it('returns 0 for single reads', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: { file_path: '/project/a.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/b.ts' }, sessionId: 's1', project: 'p1' },
{ name: 'Read', input: { file_path: '/project/c.ts' }, sessionId: 's1', project: 'p1' },
]
expect(detectDuplicateReadsLogic(calls)).toBe(0)
})
it('handles empty calls', () => {
expect(detectDuplicateReadsLogic([])).toBe(0)
expect(detectDuplicateReads(calls)).toBeNull()
})
})
function detectReadEditRatioLogic(calls: ToolCall[]) {
const READ_NAMES = new Set(['Read', 'Grep', 'Glob', 'FileReadTool', 'GrepTool', 'GlobTool'])
const EDIT_NAMES = new Set(['Edit', 'Write', 'FileEditTool', 'FileWriteTool', 'NotebookEdit'])
let reads = 0, edits = 0
for (const c of calls) {
if (READ_NAMES.has(c.name)) reads++
else if (EDIT_NAMES.has(c.name)) edits++
}
return { reads, edits, ratio: edits > 0 ? reads / edits : Infinity }
}
describe('optimize: read:edit ratio detection', () => {
it('detects low ratio (edit-heavy)', () => {
const calls: ToolCall[] = [
...Array(5).fill(null).map(() => ({ name: 'Read', input: {}, sessionId: 's1', project: 'p1' })),
...Array(10).fill(null).map(() => ({ name: 'Edit', input: {}, sessionId: 's1', project: 'p1' })),
describe('detectLowReadEditRatio', () => {
it('returns null below minimum edit count', () => {
const calls = [
call('Edit', {}),
call('Edit', {}),
call('Read', {}),
]
const { ratio } = detectReadEditRatioLogic(calls)
expect(ratio).toBe(0.5)
expect(detectLowReadEditRatio(calls)).toBeNull()
})
it('healthy ratio passes (4:1+)', () => {
const calls: ToolCall[] = [
...Array(40).fill(null).map(() => ({ name: 'Read', input: {}, sessionId: 's1', project: 'p1' })),
...Array(10).fill(null).map(() => ({ name: 'Edit', input: {}, sessionId: 's1', project: 'p1' })),
it('returns null when ratio is healthy', () => {
const calls = [
...Array.from({ length: 40 }, () => call('Read', {})),
...Array.from({ length: 10 }, () => call('Edit', {})),
]
const { ratio } = detectReadEditRatioLogic(calls)
expect(ratio).toBe(4)
expect(detectLowReadEditRatio(calls)).toBeNull()
})
it('counts Grep and Glob as reads', () => {
const calls: ToolCall[] = [
{ name: 'Read', input: {}, sessionId: 's1', project: 'p1' },
{ name: 'Grep', input: {}, sessionId: 's1', project: 'p1' },
{ name: 'Glob', input: {}, sessionId: 's1', project: 'p1' },
{ name: 'Edit', input: {}, sessionId: 's1', project: 'p1' },
it('flags when edits outpace reads', () => {
const calls = [
...Array.from({ length: 5 }, () => call('Read', {})),
...Array.from({ length: 10 }, () => call('Edit', {})),
]
const { reads, edits } = detectReadEditRatioLogic(calls)
expect(reads).toBe(3)
expect(edits).toBe(1)
const finding = detectLowReadEditRatio(calls)
expect(finding).not.toBeNull()
expect(finding!.impact).toBe('high')
})
it('counts Grep and Glob as reads for ratio', () => {
const calls = [
...Array.from({ length: 40 }, () => call('Grep', {})),
...Array.from({ length: 10 }, () => call('Edit', {})),
]
expect(detectLowReadEditRatio(calls)).toBeNull()
})
it('counts Write as edit', () => {
const calls: ToolCall[] = [
{ name: 'Write', input: {}, sessionId: 's1', project: 'p1' },
{ name: 'Edit', input: {}, sessionId: 's1', project: 'p1' },
const calls = [
...Array.from({ length: 15 }, () => call('Read', {})),
...Array.from({ length: 10 }, () => call('Write', {})),
]
const { edits } = detectReadEditRatioLogic(calls)
expect(edits).toBe(2)
})
it('ignores non-read non-edit tools', () => {
const calls: ToolCall[] = [
{ name: 'Bash', input: {}, sessionId: 's1', project: 'p1' },
{ name: 'Agent', input: {}, sessionId: 's1', project: 'p1' },
{ name: 'mcp__foo__bar', input: {}, sessionId: 's1', project: 'p1' },
]
const { reads, edits } = detectReadEditRatioLogic(calls)
expect(reads).toBe(0)
expect(edits).toBe(0)
const finding = detectLowReadEditRatio(calls)
expect(finding).not.toBeNull()
})
})
function computeHealthLogic(impacts: Array<'high' | 'medium' | 'low'>): { score: number; grade: string } {
if (impacts.length === 0) return { score: 100, grade: 'A' }
const impactWeight: Record<string, number> = { high: 15, medium: 7, low: 3 }
let penalty = 0
for (const i of impacts) penalty += impactWeight[i] ?? 0
const score = Math.max(0, 100 - Math.min(80, penalty))
const grade = score >= 90 ? 'A' : score >= 75 ? 'B' : score >= 55 ? 'C' : score >= 30 ? 'D' : 'F'
return { score, grade }
}
describe('detectCacheBloat', () => {
it('returns null below minimum api calls', () => {
const apiCalls: ApiCallMeta[] = [
{ cacheCreationTokens: 80000, version: '2.1.100' },
{ cacheCreationTokens: 80000, version: '2.1.100' },
]
expect(detectCacheBloat(apiCalls, emptyProjects())).toBeNull()
})
describe('optimize: health score and grade', () => {
it('returns A with no findings', () => {
const { score, grade } = computeHealthLogic([])
it('returns null when median is close to baseline', () => {
const apiCalls: ApiCallMeta[] = Array.from({ length: 20 }, () => ({
cacheCreationTokens: 50000,
version: '2.1.98',
}))
expect(detectCacheBloat(apiCalls, emptyProjects())).toBeNull()
})
it('flags when median exceeds 1.4x baseline', () => {
const apiCalls: ApiCallMeta[] = Array.from({ length: 20 }, () => ({
cacheCreationTokens: 80000,
version: '2.1.100',
}))
const finding = detectCacheBloat(apiCalls, emptyProjects())
expect(finding).not.toBeNull()
})
})
describe('detectBloatedClaudeMd', () => {
it('returns null when no projects have CLAUDE.md', () => {
const result = detectBloatedClaudeMd(new Set(['/nonexistent/path']))
expect(result).toBeNull()
})
it('returns null for empty project set', () => {
const result = detectBloatedClaudeMd(new Set())
expect(result).toBeNull()
})
})
describe('detectMissingClaudeignore', () => {
it('returns null for empty set', () => {
expect(detectMissingClaudeignore(new Set())).toBeNull()
})
it('returns null for non-existent cwds', () => {
expect(detectMissingClaudeignore(new Set(['/does/not/exist']))).toBeNull()
})
})
describe('computeHealth', () => {
it('returns A with 100 for no findings', () => {
const { score, grade } = computeHealth([])
expect(score).toBe(100)
expect(grade).toBe('A')
})
it('one low finding keeps grade at A', () => {
const { score, grade } = computeHealthLogic(['low'])
function mockFinding(impact: 'high' | 'medium' | 'low'): WasteFinding {
return {
title: 't', explanation: 'e', impact, tokensSaved: 1000,
fix: { type: 'paste', label: 'l', text: 't' },
}
}
it('one low finding stays at A', () => {
const { score, grade } = computeHealth([mockFinding('low')])
expect(score).toBe(97)
expect(grade).toBe('A')
})
it('two high findings drop to C', () => {
const { score, grade } = computeHealthLogic(['high', 'high'])
const { score, grade } = computeHealth([mockFinding('high'), mockFinding('high')])
expect(score).toBe(70)
expect(grade).toBe('C')
})
it('caps penalty at 80 to prevent going below 20', () => {
const impacts = Array(20).fill('high' as const)
const { score } = computeHealthLogic(impacts)
it('caps penalty at 80 to prevent score below 20', () => {
const findings = Array.from({ length: 20 }, () => mockFinding('high'))
const { score } = computeHealth(findings)
expect(score).toBe(20)
})
it('mix produces D grade', () => {
const { score, grade } = computeHealthLogic(['high', 'high', 'medium', 'medium', 'low'])
expect(score).toBe(100 - 15 - 15 - 7 - 7 - 3)
expect(grade).toBe('D')
})
})
function expandImportsLogic(content: string, resolveMap: Record<string, string>, depth = 0): number {
if (depth > 5) return 0
let total = content.split('\n').length
const matches = content.matchAll(/^@([^\s]+)/gm)
for (const m of matches) {
const key = m[1] || ''
if (resolveMap[key]) {
total += expandImportsLogic(resolveMap[key], resolveMap, depth + 1)
}
}
return total
}
describe('optimize: @-import expansion', () => {
it('counts only the base file when no imports', () => {
const total = expandImportsLogic('line 1\nline 2\nline 3', {})
expect(total).toBe(3)
})
it('expands single @-import', () => {
const main = 'line 1\n@./imported.md\nline 3'
const imported = 'i1\ni2\ni3\ni4\ni5'
expect(expandImportsLogic(main, { './imported.md': imported })).toBe(3 + 5)
})
it('expands nested @-imports recursively', () => {
const main = 'a\n@b.md'
const b = 'b1\nb2\n@c.md'
const c = 'c1\nc2\nc3'
expect(expandImportsLogic(main, { 'b.md': b, 'c.md': c })).toBe(2 + 3 + 3)
})
it('caps recursion depth', () => {
const circular = '@x.md'
expect(expandImportsLogic(circular, { 'x.md': circular })).toBeLessThan(10)
it('progresses grades predictably', () => {
expect(computeHealth([mockFinding('low')]).grade).toBe('A')
expect(computeHealth([mockFinding('medium')]).grade).toBe('A')
expect(computeHealth([mockFinding('medium'), mockFinding('medium')]).grade).toBe('B')
expect(computeHealth([mockFinding('high'), mockFinding('high'), mockFinding('high')]).grade).toBe('C')
expect(computeHealth([mockFinding('high'), mockFinding('high'), mockFinding('high'), mockFinding('high'), mockFinding('high')]).grade).toBe('F')
})
})