refactor: AI slop reduction with cross-model quality review (v0.16.3.0) by garrytan · Pull Request #941 · garrytan/gstack

garrytan · 2026-04-09T15:48:40Z

Summary

Ran slop-scan against gstack. Scored 2.38 score/file (highest on Ben Vinegar's benchmark). Dropped to 1.96 after cleanup, keeping only changes that genuinely improve code quality.

What shipped:

browse/src/error-handling.ts — shared utilities: safeUnlink (ignores ENOENT, rethrows EPERM), safeUnlinkQuiet (swallows all errors for shutdown paths), safeKill (ignores ESRCH), isProcessAlive (pure boolean probe, never throws)
Selective catches across 13 source files replacing empty catch blocks with typed error handling (ENOENT, ESRCH, TypeError, DOMException)
return await removals in content-security.ts and read-commands.ts
bun run slop:diff — shows only NEW findings on your branch vs main
Slop-scan usage guidelines in CLAUDE.md (what to fix vs what NOT to fix)

What we tried and reverted (cross-model review with Codex):

String-matching on Playwright error messages (includes('closed')) — brittle, will break on version bumps
Extension catch-and-log → selective rethrow — uncaught errors crash Chrome extensions
"alias for active session" comments — pure linter gaming
route() wrapper for Bun.serve — creates two styles for no score impact

Score:

Before: 100 findings, 432.8 score, 2.38 score/file
After: 90 findings, 358.1 score, 1.96 score/file

Pre-Landing Review

No issues found. All changes are mechanical error-handling refactoring with no business logic changes.

Test Coverage

7 unit tests for error-handling.ts (safeUnlink, safeKill, isProcessAlive)
All existing browse tests pass
Pre-existing failures (golden file, uninstall) unchanged

Test plan

bun test passes (pre-existing failures only)
bun run slop:diff runs and shows net improvement
Cross-model review (Claude + Codex) — 3 findings accepted, reverts applied

🤖 Generated with Claude Code

safeUnlink (ignores ENOENT), safeKill (ignores ESRCH), isProcessAlive (extracted from cli.ts with Windows support), and json() Response helper. All catches check err.code and rethrow unexpected errors instead of swallowing silently. Unit tests cover happy path + error code paths. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Replace ~12 try/catch sites with safeUnlink/safeKill calls in shutdown, emergencyCleanup, killAgent, and log cleanup. Convert empty catches to selective catches with error code checks. Remove needless welcome page try/catches (fs.existsSync doesn't need wrapping). Reduces slop-scan empty-catch locations from 11 to 8 and error-swallowing from 24 to 18. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Move isProcessAlive to shared error-handling module. Replace ~20 try/catch sites with safeUnlink/safeKill in killServer, connect, disconnect, and cleanup flows. Convert empty catches to selective catches. Reduces slop-scan empty-catch from 22 to 2 locations. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

…d-commands Remove 6 redundant return-await patterns where there's no enclosing try block. Eliminates all defensive.async-noise findings from these files. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

…bar-agent Convert 8 empty catch blocks to selective catches that check err.code (ESRCH for process kills, ENOENT for file ops). Import safeUnlink for cancel file cleanup. Unexpected errors now propagate instead of being silently swallowed. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

…wser-manager Convert 12 empty catch blocks to selective catches: filesystem ops check ENOENT/EACCES, browser ops check for closed/Target messages, URL parsing checks TypeError. Add 'alias for active session' comments above 6 pass-through wrapper methods to document their purpose (and exempt from slop-scan pass-through-wrappers rule). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Convert 8 defensive catch blocks to selective error handling. Filesystem ops check ENOENT/EACCES, process ops check exit status. Unexpected errors now propagate instead of returning silent defaults. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

…mmands, snapshot Convert ~27 empty/obscuring catches to selective error handling across 4 browse source files. CDP ops check for closed/Target/detached messages, DOM ops check TypeError/DOMException, filesystem ops check ENOENT/EACCES, JSON parsing checks SyntaxError. Remove dead code in cdp-inspector where try/catch wrapped synchronous no-ops. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Convert empty catches and error-swallowing patterns across inspector.js, content.js, background.js, and sidepanel.js. DOM catches filter TypeError/DOMException, chrome API catches filter Extension context invalidated, network catches filter Failed to fetch. Unexpected errors now propagate. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

…emove unused json() isProcessAlive now catches ALL errors and returns false (pure boolean probe). Callers use it in if/while conditions without try/catch, so throwing on EPERM was a behavior change that could crash the CLI. Windows path gets its safety catch restored. safeUnlinkQuiet added for best-effort cleanup paths where throwing on non-ENOENT errors (like EPERM during shutdown) would abort cleanup. json() removed — dead code, never imported anywhere. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Shutdown, emergency cleanup, and disconnect paths should never throw on file deletion failures. Switched from safeUnlink (throws on EPERM) to safeUnlinkQuiet (swallows all errors) in these best-effort paths. Normal operation paths (startup, lock release) keep safeUnlink. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

…browser-manager Revert 6 catches that matched error messages via includes('closed'), includes('Target'), etc. back to empty catches. These fire-and-forget operations (page.close, bringToFront, dialog dismiss) genuinely don't care about any error type. String matching on error messages is brittle and will break on Playwright version bumps. Remove 6 'alias for active session' comments that existed solely to game slop-scan's pass-through-wrapper exemption rule. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Revert error-swallowing fixes in background.js and sidepanel.js that matched error messages via includes('Failed to fetch'), includes( 'Extension context invalidated'), etc. In Chrome extensions, uncaught errors crash the entire extension. The original catch-and-log pattern is the correct choice for extension code where any error is non-fatal. content.js and inspector.js changes kept — their TypeError/DOMException catches are typed, not string-based. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Instructions for using slop-scan to improve genuine code quality, not to game metrics or hide that we're AI-coded. Documents what to fix (empty catches on file/process ops, typed exception narrows, return await) and what NOT to fix (string-matching on error messages, linter gaming comments, tightening extension/cleanup catches). Includes utility function reference and baseline score tracking. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Runs slop-scan after bun test as a non-blocking diagnostic. Prints the summary (top files, hotspots) so you see the number without it gating anything. Available standalone via bun run slop. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Runs slop-scan on HEAD and the merge-base, diffs results with line-number-insensitive fingerprinting so shifted code doesn't create false positives. Uses git worktree for clean base comparison. Shows net new vs removed findings. Runs automatically after bun test. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Deferred plan for surfacing slop-diff findings automatically during code review and shipping. Documents integration points, auto-fix vs skip heuristics, and implementation notes. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

github-actions · 2026-04-09T15:57:23Z

E2E Evals: ✅ PASS

9/9 tests passed | $1.54 total cost | 12 parallel runners

Suite	Result	Status	Cost
e2e-browse	2/2	✅	$0.13
e2e-deploy	2/2	✅	$0.33
e2e-qa-workflow	1/1	✅	$0.71
llm-judge	2/2	✅	$0.04
e2e-deploy	2/2	✅	$0.33

12x ubicloud-standard-2 (Docker: pre-baked toolchain + deps) | wall clock ≈ slowest suite

garrytan and others added 20 commits April 9, 2026 04:41

chore: add slop-scan config to exclude vendor files

d2349d1

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

Merge remote-tracking branch 'origin/main' into garrytan/slop-reduction

a302593

chore: bump version and changelog (v0.16.3.0)

e4b80dd

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

garrytan merged commit c6e6a21 into main Apr 11, 2026
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: AI slop reduction with cross-model quality review (v0.16.3.0)#941

refactor: AI slop reduction with cross-model quality review (v0.16.3.0)#941
garrytan merged 20 commits intomainfrom
garrytan/slop-reduction

garrytan commented Apr 9, 2026

Uh oh!

github-actions bot commented Apr 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

garrytan commented Apr 9, 2026

Summary

Pre-Landing Review

Test Coverage

Test plan

Uh oh!

github-actions bot commented Apr 9, 2026

E2E Evals: ✅ PASS

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant