From 1103ca77a597685591e5ae1b2b47a640acd47b33 Mon Sep 17 00:00:00 2001 From: iamtoruk Date: Mon, 10 Aug 2026 06:07:27 -0700 Subject: [PATCH] fix(insights): count read-shaped shell commands as reads, not as verification (#941) --- src/bash-utils.ts | 51 +++++++++++++++++++++++++++++++++++++ src/classifier.ts | 9 ++++++- src/optimize.ts | 12 +++++++++ tests/bash-commands.test.ts | 32 ++++++++++++++++++++++- tests/classifier.test.ts | 22 ++++++++++++++++ tests/optimize.test.ts | 24 +++++++++++++++++ 6 files changed, 148 insertions(+), 2 deletions(-) diff --git a/src/bash-utils.ts b/src/bash-utils.ts index 06a4f6df..b6388a95 100644 --- a/src/bash-utils.ts +++ b/src/bash-utils.ts @@ -62,3 +62,54 @@ export function extractBashCommands(rawCommand: string): string[] { return commands } + +// Read-shaped shell commands: they inspect state and cannot modify the +// worktree, so (1) the read-edit-ratio detector counts them as reads and +// (2) retry detection does NOT treat them as a verification step between two +// edits of the same file (#941 — both detectors previously disagreed about +// the same Bash call, scoring rg-first workflows as reckless AND reworked). +const READ_ONLY_BASH = new Set([ + 'rg', 'grep', 'egrep', 'fgrep', 'ag', + 'cat', 'head', 'tail', 'less', 'more', + 'ls', 'find', 'fd', 'tree', + 'wc', 'stat', 'file', 'du', 'df', + 'which', 'type', 'pwd', 'printenv', 'env', + 'readlink', 'realpath', 'basename', 'dirname', + 'jq', 'diff', +]) + +// git subcommands that only inspect history/state. Deliberately excludes +// anything that can create or mutate under any flag (branch, tag, stash, +// remote), so a mutation is never misread as a read. +const GIT_READ_SUBCOMMANDS = new Set([ + 'log', 'diff', 'status', 'show', 'blame', 'grep', + 'shortlog', 'describe', 'rev-parse', 'ls-files', +]) + +/// True when EVERY segment of the raw command line (split on &&, ;, |) is a +/// read-only inspection command. A single mutating segment makes the whole +/// call non-read (`cat x && sed -i ...` edits). Unknown commands are +/// non-read: the conservative default both call sites want. +export function isReadShapedBashCommand(rawCommand: string): boolean { + if (!rawCommand || !rawCommand.trim()) return false + const stripped = stripQuotedStrings(stripAnsi(rawCommand)) + const segments = stripped.split(/\s*(?:&&|;|\|)\s*/) + let sawCommand = false + for (const segment of segments) { + const trimmed = segment.trim() + if (!trimmed) continue + const tokens = trimmed.split(/\s+/) + let i = 0 + while (i < tokens.length && (/^\w+=/.test(tokens[i]!) || COMMAND_PREFIXES.has(basename(tokens[i]!)))) i++ + const base = i < tokens.length ? basename(tokens[i]!) : '' + if (!base) continue + sawCommand = true + if (base === 'git') { + const sub = tokens[i + 1] + if (!sub || !GIT_READ_SUBCOMMANDS.has(sub)) return false + continue + } + if (!READ_ONLY_BASH.has(base)) return false + } + return sawCommand +} diff --git a/src/classifier.ts b/src/classifier.ts index 9255f841..44582db8 100644 --- a/src/classifier.ts +++ b/src/classifier.ts @@ -1,4 +1,5 @@ import type { ClassifiedTurn, ParsedTurn, TaskCategory, ToolCall } from './types.js' +import { isReadShapedBashCommand } from './bash-utils.js' const TEST_PATTERNS = /\b(test|pytest|vitest|jest|mocha|spec|coverage|npm\s+test|npx\s+vitest|npx\s+jest)\b/i const GIT_PATTERNS = /\bgit\s+(push|pull|commit|merge|rebase|checkout|branch|stash|log|diff|status|add|reset|cherry-pick|tag)\b/i @@ -170,7 +171,13 @@ function countRetries(turn: ParsedTurn): number { steps.forEach((step, i) => { for (const call of step) { if (BASH_TOOLS.has(call.tool)) { - lastVerifyStep = i + // A read-shaped shell command (rg/cat/git log) is a lookup, not a + // verification of the previous edit: edit -> grep -> edit is research, + // and counting it as a retry penalized bash-first workflows (#941). + // A command we cannot classify keeps the old behavior. + if (!call.command || !isReadShapedBashCommand(call.command)) { + lastVerifyStep = i + } } if (EDIT_TOOLS.has(call.tool)) { const fileKey = call.file ?? '__no_file__' diff --git a/src/optimize.ts b/src/optimize.ts index 121507ce..63d49330 100644 --- a/src/optimize.ts +++ b/src/optimize.ts @@ -1,4 +1,5 @@ import chalk from 'chalk' +import { isReadShapedBashCommand } from './bash-utils.js' import { readdir, stat } from 'fs/promises' import { existsSync, statSync } from 'fs' import { basename, join } from 'path' @@ -383,6 +384,10 @@ function compactOptimizeInput(name: string, input: unknown): Record): WasteFinding | export const READ_TOOL_NAMES = new Set(['Read', 'Grep', 'Glob', 'FileReadTool', 'GrepTool', 'GlobTool']) export const EDIT_TOOL_NAMES = new Set(['Edit', 'Write', 'FileEditTool', 'FileWriteTool', 'NotebookEdit']) +export const BASH_TOOL_NAMES = new Set(['Bash', 'BashTool', 'PowerShellTool']) export function detectLowReadEditRatio(calls: ToolCall[]): WasteFinding | null { let reads = 0 @@ -2199,6 +2205,12 @@ export function detectLowReadEditRatio(calls: ToolCall[]): WasteFinding | null { if (READ_TOOL_NAMES.has(call.name)) { reads++ if (call.recent) recentReads++ + } else if (BASH_TOOL_NAMES.has(call.name) && typeof call.input['command'] === 'string' && isReadShapedBashCommand(call.input['command'])) { + // A session that looks things up with rg/cat/git log IS reading (#941): + // ignoring shell reads scored disciplined rg-first workflows as + // reckless editors (90%+ of real reads were invisible to this ratio). + reads++ + if (call.recent) recentReads++ } else if (EDIT_TOOL_NAMES.has(call.name)) { edits++ if (call.recent) recentEdits++ diff --git a/tests/bash-commands.test.ts b/tests/bash-commands.test.ts index 3daf439a..949d50ff 100644 --- a/tests/bash-commands.test.ts +++ b/tests/bash-commands.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { extractBashCommands } from '../src/bash-utils.js' +import { extractBashCommands, isReadShapedBashCommand } from '../src/bash-utils.js' import { BASH_TOOLS } from '../src/classifier.js' describe('extractBashCommands', () => { @@ -117,3 +117,33 @@ describe('BASH_TOOLS', () => { it('recognizes BashTool', () => { expect(BASH_TOOLS.has('BashTool')).toBe(true) }) it('rejects unknown tools', () => { expect(BASH_TOOLS.has('Read')).toBe(false) }) }) + +describe('isReadShapedBashCommand (#941)', () => { + it('accepts single read commands and read-only git subcommands', () => { + expect(isReadShapedBashCommand('rg -n "x" src/')).toBe(true) + expect(isReadShapedBashCommand('cat file.ts')).toBe(true) + expect(isReadShapedBashCommand('git log --oneline -5')).toBe(true) + expect(isReadShapedBashCommand('git diff HEAD~1')).toBe(true) + expect(isReadShapedBashCommand('VAR=1 sudo head -c 100 f')).toBe(true) + }) + + it('accepts pipelines where every segment reads', () => { + expect(isReadShapedBashCommand('grep -r "x" src | head -20')).toBe(true) + expect(isReadShapedBashCommand('git log --oneline && git status')).toBe(true) + }) + + it('rejects any mutating or unknown segment', () => { + expect(isReadShapedBashCommand('npm test')).toBe(false) + expect(isReadShapedBashCommand('sed -i s/a/b/ x.ts')).toBe(false) + expect(isReadShapedBashCommand('cat x && rm -rf dist')).toBe(false) + expect(isReadShapedBashCommand('git commit -m "x"')).toBe(false) + expect(isReadShapedBashCommand('git branch new-branch')).toBe(false) + expect(isReadShapedBashCommand('git')).toBe(false) + expect(isReadShapedBashCommand('')).toBe(false) + expect(isReadShapedBashCommand(' ')).toBe(false) + }) + + it('is not fooled by read-command names inside quoted strings', () => { + expect(isReadShapedBashCommand('echo "cat file"')).toBe(false) + }) +}) diff --git a/tests/classifier.test.ts b/tests/classifier.test.ts index 6b4730c2..a42c0570 100644 --- a/tests/classifier.test.ts +++ b/tests/classifier.test.ts @@ -153,6 +153,28 @@ describe('classifyTurn — feature vs debugging precedence (#196)', () => { }) describe('classifyTurn — retry detection via toolSequence', () => { + it('does not count a read-shaped shell lookup between edits as a retry (#941)', () => { + // edit -> rg -> edit is research, not a failed verification loop. + const call = makeCall({ tools: ['Edit', 'Bash'] }) + call.toolSequence = [[{ tool: 'Edit', file: 'a.ts' }], [{ tool: 'Bash', command: 'rg -n "helper" src/' }], [{ tool: 'Edit', file: 'a.ts' }]] + const turn = makeTurn([call], 'fix the build') + expect(classifyTurn(turn).retries).toBe(0) + }) + + it('still counts a verification-shaped shell command between edits as a retry (#941)', () => { + const call = makeCall({ tools: ['Edit', 'Bash'] }) + call.toolSequence = [[{ tool: 'Edit', file: 'a.ts' }], [{ tool: 'Bash', command: 'npm test' }], [{ tool: 'Edit', file: 'a.ts' }]] + const turn = makeTurn([call], 'fix the build') + expect(classifyTurn(turn).retries).toBe(1) + }) + + it('keeps counting command-less bash steps as verification (unknown stays conservative)', () => { + const call = makeCall({ tools: ['Edit', 'Bash'] }) + call.toolSequence = [[{ tool: 'Edit', file: 'a.ts' }], [{ tool: 'Bash' }], [{ tool: 'Edit', file: 'a.ts' }]] + const turn = makeTurn([call], 'fix the build') + expect(classifyTurn(turn).retries).toBe(1) + }) + it('detects retries from multi-call turns (Claude-style)', () => { const turn = makeTurn([ makeCall({ tools: ['Edit'] }), diff --git a/tests/optimize.test.ts b/tests/optimize.test.ts index c3bcdf0b..bc72dd88 100644 --- a/tests/optimize.test.ts +++ b/tests/optimize.test.ts @@ -234,6 +234,30 @@ describe('detectDuplicateReads', () => { }) describe('detectLowReadEditRatio', () => { + it('counts read-shaped bash commands as reads (#941)', () => { + // 10 edits with 40 rg/cat/git-log lookups through the shell: a healthy + // 4:1 workflow that previously read as 0 reads and fired at high impact. + const calls = [ + ...Array.from({ length: 20 }, () => call('Bash', { command: 'rg -n "pattern" src/' })), + ...Array.from({ length: 10 }, () => call('Bash', { command: 'cat src/parser.ts | head -50' })), + ...Array.from({ length: 10 }, () => call('Bash', { command: 'git log --oneline -5' })), + ...Array.from({ length: 10 }, () => call('Edit', {})), + ] + expect(detectLowReadEditRatio(calls)).toBeNull() + }) + + it('does not count mutating or unclassifiable bash as reads (#941)', () => { + const calls = [ + ...Array.from({ length: 20 }, () => call('Bash', { command: 'npm test' })), + ...Array.from({ length: 10 }, () => call('Bash', { command: 'cat notes.md && sed -i s/a/b/ src/x.ts' })), + ...Array.from({ length: 10 }, () => call('Bash', {})), + ...Array.from({ length: 10 }, () => call('Edit', {})), + ] + const finding = detectLowReadEditRatio(calls) + expect(finding).not.toBeNull() + expect(finding!.explanation).toContain('0 reads') + }) + it('returns null below minimum edit count', () => { const calls = [ call('Edit', {}),