chore(mcp): remove confirmed dead code in the top-level MCP and misc surface - #6956
Conversation
…surface Eight symbols across three files, 27 lines, no behaviour change. stats.py: six orphaned mutators (inc_input_tokens, inc_output_tokens, inc_cache_creation_tokens, inc_cache_read_tokens, inc_turns, inc_duration_ms). Their sole caller was the chat_runner block deleted in 61cb304 (#1082); that PR also added a guard test asserting the wiring stays gone, so a quiet revival is blocked by CI rather than convention. Their counter keys in _init_counters are untouched, so the /api/system response shape is unchanged and test_snapshot_keys still holds. llm_helpers.py: OnPermissionCallback, a type alias whose only occurrence repo-wide was its own definition. The identical type is inlined at both live call sites, so no contract is lost. frontend.py: _REPO_URL and _DEFAULT_REPO_URL, dead as a pair since the latter's only consumer is the former, plus the comment above them. That comment claimed KIROCREW_WEBSITE_REPO was retained for tooling and docs to point at; there are zero references to it anywhere in the repo. Verified per symbol: whole-repo search across py/ts/tsx/js/md/json/yml/ toml/sh/ps1 plus tests, docs, system-specs, builtin_skills, .github and the baseline JSONs; bare-string search for string dispatch; getattr/ setattr/globals/importlib/__all__ sweep; reverse-direction reachability; public-surface check; and a 30-day introduction gate (all eight last touched 2026-07-16). flake8 confirms no import became unused. No test needed deleting or altering.
Design Review (Fable 5) — ✅ PASSDesign-level review of Verification complete: zero remaining references to any deleted symbol, no string-dispatch writers to the orphaned counter keys, and the Design-Verdict: PASS Deletion claims verified end-to-end; conservative scoping preserves the Suggestions
[DESIGN-REVIEWED] 279b23a |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All verifications complete. The dead cost trio ( First-Principles-Verdict: PASS A pure subtraction: 27 lines of verified-dead code deleted, every zero-reference claim independently reproduced by grep at HEAD. What this change shipsIntent: shrink maintenance surface by deleting confirmed-dead symbols — a FIX-shaped chore, purely subtractive.
Counter keys, Subtractions
[FIRST-PRINCIPLES-REVIEWED] 279b23a |
bolichen97
left a comment
There was a problem hiding this comment.
Full-diff maintainer review passed: change matches its stated scope, no regressions or trust-boundary weakening found, checks green and no outstanding change requests. Approving.
Eight symbols, 27 lines, three files. No behaviour change, no test altered.
This is one slice of a wider dead-code sweep, scoped to the top-level MCP modules, the bootstrap/model/publish modules, the trust surface, and the misc utility modules — 83 files, 47,596 lines.
Deleted
stats.py— six orphaned mutators (18 lines)inc_input_tokens,inc_output_tokens,inc_cache_creation_tokens,inc_cache_read_tokens,inc_turns,inc_duration_ms.Their sole caller was the
chat_runner.pyblock deleted in 61cb304 (#1082). That PR also addedtest_chat_runner_no_longer_builds_the_dead_stats_object, which asserts the wiring stays gone — so a quiet revival is blocked by CI, not just by convention. Zero references repo-wide across every file type; no computed-name dispatch (f"inc_{key}") exists; the threegetattr(stats, ...)sites in the repo target unrelatedStatsclasses in other modules.The counter keys in
_init_countersare deliberately not touched, sosnapshot()still returns the full dict and the/api/systemresponse shape (dashboard/handlers_system.py:193) is unchanged.test_stats.py's snapshot-key pin still holds.llm_helpers.py—OnPermissionCallback(3 lines)A type alias whose only occurrence repo-wide was its own definition. The identical type
Callable[[LLMEvent], Awaitable[bool]]is already inlined at both live call sites (:1432 and :1899), so no behaviour and no signature changes.frontend.py—_REPO_URL+_DEFAULT_REPO_URL(5 lines)Dead as a pair:
_DEFAULT_REPO_URL's only consumer was_REPO_URL, which nothing consumed. The.github/workflows/publish-docker.ymlgrep hit isSOURCE_REPO_URL, an unrelated Docker build ARG — a suffix collision.The comment above them is removed with them because it was false: it claimed
KIROCREW_WEBSITE_REPOwas "retained only so existing tooling/docs referencing the public mirror keep a stable name to point at", but that variable has zero references anywhere in the repo, website, docs, workflows or installers. A caller setting it today already gets a no-op.Verification
Per symbol: whole-repo search across
*.py *.ts *.tsx *.js *.md *.json *.yml *.yaml *.toml *.sh *.ps1plustests/,docs/,docs/system-specs/,builtin_skills/,.github/,config-baseline.json,error-code-baseline.json,install.sh,Makefile,pyproject.toml— never truncated throughhead; bare-string search for string dispatch;getattr/setattr/globals()/importlib/__all__sweep; reverse-direction reachability proof; public-surface check; 30-day introduction gate (all eight last touched 2026-07-16, 45 days).flake8clean on all three files —osremains used infrontend.py,Callable/Awaitableinllm_helpers.py.isortclean. 468 tests pass across the affected scope, 0 failures. (black --checkflagsfrontend.pyat lines 282/366, far from this diff; it reproduces identically on unmodifiedorigin/mainand is a local black/target-version mismatch, not this change.)A scanner defect worth recording
vultureis near-useless scoped to a file subset — it flags every symbol imported from outside the scope, so it reportedparse_widgets,GatewayLock,LessonStore,SleepInhibitorandprune_pycacheas unused. More importantly, this codebase usesfrom __future__ import annotations, which makes every annotation a string at runtime; quoted forward references likestore: "_ReconcilableStore"never become NAME tokens, so tokenize- and vulture-based scanning is structurally blind to annotation-only references. That produced a false positive (_ReconcilableStore, which is live) and was only caught by adding an annotation-aware AST pass that re-parses string annotations. Any future audit of this repo needs that pass or it will propose deleting live Protocols.Reviewer finding, addressed
A local GPT-5.6 review lane raised one Medium: deleting
OnPermissionCallbackremoves the only named form of the callback contract, breaking a hypotheticalfrom kiro_crew.llm_helpers import OnPermissionCallback.Not acting on it, on these grounds:
llm_helpers.pydeclares no__all__, the name appears in nodocs/,docs/system-specs/orSKILL.mdfile, it is not a declared export inpyproject.toml, and it has zero importers in-repo. Under Python's conventions every non-underscore module-level name is "externally importable", so that alone would make any internal helper undeletable. KiroCrew ships as an application rather than a published library, so there is no semver contract onkiro_crew.llm_helpersinternals. Happy to restore it as a deprecated alias if a maintainer disagrees.A local Opus-4.8 lane reviewed the same commit for behaviour change, env-var regression, vacuous tests and partial-deletion hazard.
Deferred — not in this PR
_create_embedding(embeddings.py:1529) — genuinely orphaned by efadac8, superseded by_submit_infer, but that commit is 2026-08-12, 18 days: inside the 30-day gate. Eligible 2026-09-12; deleting it must also fix the dangling:meth:cross-ref at :1485.DM_FILE_NAME(members.py:56) — real leftover ("dm.json"is never used to build a path; live layout istrust/member-bindings/<slug>.json), but introduced 3 days ago in a231501 (feat: add crew members page with per-member DM threads #6210) alongside its own replacement. Dead on arrival within one PR; belongs back with that PR rather than an unrelated chore branch.RemoteListing(publish_provider.py:152) —website/src/types/index.ts:1176-1204 mirrors its six fields and cites it 4× as the authoritative base contract. Also an unenforced contract: nothing constructs it, the wire shape is a plain dict, and the browse handler passes the provider dict straight through. Design debt, not dead code.sso_status_async,unregister_channel,accepts_priority,DERIVED_SLUG_RE,list_entries,_reset_signature_replay,_reset_auth_throttle.Live — scanner false positives, do not re-propose
_ReconcilableStore(embeddings.py:61) — atyping.Protocolannotating two functions in quoted form (:973, :1055), both with production callers incli_server.py,slack/gateway.pyanddashboard/handlers/memory.py, documented atdocs/system-specs/modules/memory-skills-hooks.md:413. It is the only written statement of a contract on a destructive path (clear_when_unknown=Trueclears stored vectors)._HANDLER_SURFACE(mcp_core.py:82) — live as a fuse. Holds 8 objects consumed via late-boundmcp_core.<name>lookup from sevenmcp_tools/*.pymodules and rebound by ~60patch("kiro_crew.mcp_core.X")sites. The tuple is the only in-file use of those imports, so deleting it makes them F401-visible and arms a cascade where a routine unused-import sweep breaks every handler withAttributeError. Named as a contract indocs/architecture/mcp.md:681.Needs a maintainer decision
stats.pycost trio.inc_cost_usd(:95),get_cost_usd(:105) and the backing fieldself._cost_usd(:56) are all three dead, andget_cost_usdnever had a caller in repo history. Because_cost_usdis not in the_cdict it never reachedsnapshot()or the API, so there is no exposure to lose. Left in place because a matched pair with both halves dead is design incompleteness rather than cruft — 9 lines if you want it gone as one unwired-feature removal.input_tokens,output_tokens,cache_creation_tokens,cache_read_tokens,total_turns,total_duration_mshave no writer and no reader, yet still ship in/api/system. Nothing inwebsite/srcreads them (the Usage tab sources the separate provider-usage API). Removing them changes the response shape and breakstest_snapshot_keys, so it is a separate decision.cli._setup_cli_logging(cli.py:1015) is the only production caller ofinstall_log_redactionand passes[], so_secret_patternis alwaysNone: only Bearer tokens are scrubbed, and only for_LONG_LIVED_COMMANDS(cli.py:1014). Meanwhilesecurity_posture.py:1577 allowlists the module out of the egress drift-guard describing it as "strips vault secrets and Bearer tokens from log output" — a governance surface asserting a control that is half-live. The module docstring admits it as "a follow-up PR". Worth its own ticket.skill_trust.grant_project_trusthas no expiry sweep: entries persistgranted_atonly, no TTL, no reaper. The 512-entry ceiling refuses the 513th grant (TrustStoreFull) instead of evicting the stalest, so a store full of abandoned grants blocks new legitimate consent until a human revokes manually.safety_override.renew(unscoped) has zero production callers (20 test refs); all real renewal goes throughrenew_scoped. The global YOLO grant therefore has no sliding extension in practice while the code reads as though it does.name_grant.pin_human_approvalhas no unpin: release is implicit via a 512-entry LRU and identity-mismatch rebind, with no operator-facing revoke.Files not exhaustively hand-confirmed
All 83 scope files were parsed by the AST/tokenize/annotation passes and all 71 declared top-level modules exist. What was not hand-audited for string-dispatch-only symbols is the deep-semantic layer of the ten largest files, which hold 44% of the scope in 12% of the files:
onboarding_import.py(5089),kiro_prerequisite.py(3034),llm_helpers.py(2351),embeddings.py(2183),mcp_core.py(2085),name_grant.py(1405),publish_sync.py(1369),mcp_dashboard.py(1212),mcp_shared.py(1201),tips.py(1170).name_grant.py,skill_trust.py,safety_override.py,trust_patterns.py,zip_vet.pyandpinned_fs.pyalso warrant a dedicated half-wired-guard pass.