ponytail-audit: trim over-engineering (dead code, single-use indirection, duplicated test helper) - #4
Merged
Conversation
A whole-repo over-engineering pass (ponytail-audit methodology: delete dead code, drop single-use indirection, prefer fewer lines) surfaced a handful of safe cuts that don't weaken the example or its tests: - reports.py: drop the dead 'except ToolError: raise' in export_report. Only the storage calls run inside the try and they raise boto/OS errors, never ToolError (the input ToolErrors are raised above, before the try), so the clause caught nothing. - server.py: remove the unused 'ctx: Context' parameter from whoami (identity comes from get_access_token(), not the context) and the now-unused Context import. - agents.py: remove '@typing.runtime_checkable' from the Agent protocol -- it's never used in an isinstance/issubclass check; the Protocol seam itself stays. - tests: hoist the duplicated '_tags' meta-extraction helper into conftest as 'tool_tags' and import it, instead of copy-pasting it in two test files. Load-bearing complexity was left intact on purpose: the per-domain backend seams, the Agent protocol, the dev-token verifier, the defense-in-depth refund guards, and the long teaching docstrings are the point of this example. No dependency is removable (fastmcp and boto3 are both used). All 77 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5dytZCaHHE432vqopou4h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ran a ponytail-audit (whole-repo scan for over-engineering only — not correctness/security/perf) using the DietrichGebert/ponytail methodology, then applied the subset of findings that are genuinely safe on a deliberate teaching example.
On installing the plugin:
/plugin marketplace addis an interactive Claude Code command that needs a trust-approval prompt, which isn't available in an autonomous session, and plugin hooks only load at session start anyway. So instead of wiring in third-party auto-running hooks, I cloned the plugin, security-reviewed it, and applied its audit rubric directly from its (inert) skill markdown — no third-party code was executed.Plugin security review (for the record): ponytail's lifecycle hooks (SessionStart / SubagentStart / UserPromptSubmit) only inject a "lazy senior dev" ruleset into the model context and write a mode-flag file under
~/.claude/~/.config/ponytail. No network calls, no reading of secrets/credentials, no dynamically-executed code, and nopostinstall/preinstallscripts. The onlyfetch/evalin the repo lives in dev-onlybenchmarks/that the hooks never invoke and npm doesn't ship. Verdict: safe.Audit findings
Ranked biggest-cut-first (
<tag>: <what to cut>. <replacement>. [path]):Applied (safe — 77 tests stay green, teaching point intact):
delete:deadexcept ToolError: raiseinexport_report. Nothing inside thetryraisesToolError(input validation raises before it). Removed.[reports.py]delete:unusedctx: Contextparameter onwhoami(identity comes fromget_access_token()), plus the now-unusedContextimport.[server.py]delete:@typing.runtime_checkableon theAgentprotocol — never used in a runtimeisinstancecheck. TheProtocolseam itself stays.[agents.py]shrink:duplicated_tags()helper copy-pasted in two test files → hoisted toconftest.tool_tags.[tests/]Flagged but deliberately kept (load-bearing pedagogy, not bloat):
yagni:acme_mcp.__init__.build_serverlazy re-export — the documented public entry point.yagni:mount_analytics()wrapper — demonstrates proxy composition (a stated teaching point).yagni:per-domain module-level_backendseams inorders/billing(onlyadmin/reportsare swapped in tests) — but the uniform backend-stand-in pattern is the lesson.native:theisinstance(amount, bool)guard inissue_refund— intentional defense-in-depth / teaching note (see PR Pressure-test hardening: report_id/order_id validation, groups-claim robustness, error sanitization #3).Dependencies: none removable —
fastmcpandboto3are both genuinely used.net: -1 line net (a wash by line count), but 4 real cuts: 2 dead-code removals, 1 decorator, 1 de-duplicated test helper.
-0 deps.Testing
python -m pytest -q→ 77 passed. Server still builds (build_server(env="dev")) andacme_mcp.agents.Agentimports cleanly after theContext/runtime_checkableremovals.🤖 Generated with Claude Code
https://claude.ai/code/session_01F5dytZCaHHE432vqopou4h
Generated by Claude Code