Skip to content

chore(mcp): remove confirmed dead code in the top-level MCP and misc surface - #6956

Merged
iamwhatever merged 1 commit into
mainfrom
chore/dead-code-mcp-misc
Aug 30, 2026
Merged

chore(mcp): remove confirmed dead code in the top-level MCP and misc surface#6956
iamwhatever merged 1 commit into
mainfrom
chore/dead-code-mcp-misc

Conversation

@iamwhatever

Copy link
Copy Markdown
Collaborator

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.py block deleted in 61cb304 (#1082). That PR also added test_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 three getattr(stats, ...) sites in the repo target unrelated Stats classes in other modules.

The counter keys in _init_counters are deliberately not touched, so snapshot() still returns the full dict and the /api/system response shape (dashboard/handlers_system.py:193) is unchanged. test_stats.py's snapshot-key pin still holds.

llm_helpers.pyOnPermissionCallback (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.yml grep hit is SOURCE_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_REPO was "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 *.ps1 plus tests/, docs/, docs/system-specs/, builtin_skills/, .github/, config-baseline.json, error-code-baseline.json, install.sh, Makefile, pyproject.toml — never truncated through head; 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).

flake8 clean on all three files — os remains used in frontend.py, Callable/Awaitable in llm_helpers.py. isort clean. 468 tests pass across the affected scope, 0 failures. (black --check flags frontend.py at lines 282/366, far from this diff; it reproduces identically on unmodified origin/main and is a local black/target-version mismatch, not this change.)

A scanner defect worth recording

vulture is near-useless scoped to a file subset — it flags every symbol imported from outside the scope, so it reported parse_widgets, GatewayLock, LessonStore, SleepInhibitor and prune_pycache as unused. More importantly, this codebase uses from __future__ import annotations, which makes every annotation a string at runtime; quoted forward references like store: "_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 OnPermissionCallback removes the only named form of the callback contract, breaking a hypothetical from kiro_crew.llm_helpers import OnPermissionCallback.

Not acting on it, on these grounds: llm_helpers.py declares no __all__, the name appears in no docs/, docs/system-specs/ or SKILL.md file, it is not a declared export in pyproject.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 on kiro_crew.llm_helpers internals. 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 is trust/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.
  • Tests-only references (rule: refactor, not deletion): 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) — a typing.Protocol annotating two functions in quoted form (:973, :1055), both with production callers in cli_server.py, slack/gateway.py and dashboard/handlers/memory.py, documented at docs/system-specs/modules/memory-skills-hooks.md:413. It is the only written statement of a contract on a destructive path (clear_when_unknown=True clears stored vectors).
  • _HANDLER_SURFACE (mcp_core.py:82) — live as a fuse. Holds 8 objects consumed via late-bound mcp_core.<name> lookup from seven mcp_tools/*.py modules and rebound by ~60 patch("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 with AttributeError. Named as a contract in docs/architecture/mcp.md:681.

Needs a maintainer decision

  1. stats.py cost trio. inc_cost_usd (:95), get_cost_usd (:105) and the backing field self._cost_usd (:56) are all three dead, and get_cost_usd never had a caller in repo history. Because _cost_usd is not in the _c dict it never reached snapshot() 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.
  2. Six now-writerless counter keys. After this PR, input_tokens, output_tokens, cache_creation_tokens, cache_read_tokens, total_turns, total_duration_ms have no writer and no reader, yet still ship in /api/system. Nothing in website/src reads them (the Usage tab sources the separate provider-usage API). Removing them changes the response shape and breaks test_snapshot_keys, so it is a separate decision.
  3. Vault secrets are never redacted from logs — found while auditing the trust surface, unrelated to this diff and untouched by it. cli._setup_cli_logging (cli.py:1015) is the only production caller of install_log_redaction and passes [], so _secret_pattern is always None: only Bearer tokens are scrubbed, and only for _LONG_LIVED_COMMANDS (cli.py:1014). Meanwhile security_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.
  4. Three orphaned lifecycle halves on the trust surface (reported, not touched — a dead guard is a defect, not cruft):
    • skill_trust.grant_project_trust has no expiry sweep: entries persist granted_at only, 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 through renew_scoped. The global YOLO grant therefore has no sliding extension in practice while the code reads as though it does.
    • name_grant.pin_human_approval has 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.py and pinned_fs.py also warrant a dedicated half-wired-guard pass.

…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.
@iamwhatever
iamwhatever requested a review from a team as a code owner August 30, 2026 07:15
@iamwhatever
iamwhatever requested a review from bolichen97 August 30, 2026 07:15
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 279b23af93bc7ce5b2f6dcd0f9bcb99f9fb1fbb3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verification complete: zero remaining references to any deleted symbol, no string-dispatch writers to the orphaned counter keys, and the getattr(stats, ...) sites do target unrelated Stats classes as claimed. The deletions are exactly what the description says, conservatively scoped (snapshot keys and API shape deliberately untouched), with in-scope-but-risky items explicitly deferred to maintainer decisions.

Design-Verdict: PASS

Deletion claims verified end-to-end; conservative scoping preserves the /api/system contract, and every riskier removal is correctly deferred rather than smuggled in.

Suggestions

  • The stats.py cost trio (inc_cost_usd/get_cost_usd/_cost_usd) is now the file's only remaining fully-dead cluster; fold its removal into the maintainer decision on the six writerless counter keys so stats debt is retired in one pass rather than three.

[DESIGN-REVIEWED] 279b23a

@github-actions

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 279b23af93bc7ce5b2f6dcd0f9bcb99f9fb1fbb3 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 279b23a

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 279b23af93bc7ce5b2f6dcd0f9bcb99f9fb1fbb3: <one-sentence reason>

@github-actions

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 279b23af93bc7ce5b2f6dcd0f9bcb99f9fb1fbb3 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 279b23a

Verdict parsed from the review's SHA-scoped output markers for commit 279b23af93bc7ce5b2f6dcd0f9bcb99f9fb1fbb3.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 279b23af93bc7ce5b2f6dcd0f9bcb99f9fb1fbb3: <one-sentence reason>

@github-actions

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 279b23af93bc7ce5b2f6dcd0f9bcb99f9fb1fbb3 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verifications complete. The dead cost trio (inc_cost_usd/get_cost_usd/_cost_usd) indeed has zero external callers — every other grep hit is an unrelated total_cost_usd in the auto-improvement app or chat-runner locals — so the author's "needs a maintainer decision" deferral leaves confirmed-dead siblings in the same file. Everything else checks out: zero references to any deleted symbol, counter keys and snapshot() untouched.

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 ships

Intent: shrink maintenance surface by deleting confirmed-dead symbols — a FIX-shaped chore, purely subtractive.

  1. Six unused token/turn/duration mutators removed from Stats — justified (grep inc_input_tokens|…|inc_duration_ms: 0 callers, one test-docstring prose mention).
  2. OnPermissionCallback type alias removed — justified (grep: 0 references repo-wide; both live call sites already inline the type).
  3. _REPO_URL/_DEFAULT_REPO_URL and the KIROCREW_WEBSITE_REPO env-var read removed — justified (grep KIROCREW_WEBSITE_REPO: 0 references; setting it was already a no-op since _REPO_URL had 0 consumers).
  4. False comment claiming the env var was "retained for tooling/docs" removed — justified (the claim had 0 supporting references).

Counter keys, snapshot(), and the /api/system shape are untouched as described; no undeclared item rides along.

Subtractions

  • Delete the inc_cost_usd / get_cost_usd / _cost_usd trio in src/kiro_crew/stats.py:56,83-89 in the same sweep — grep confirms 0 callers (all other cost_usd hits are unrelated total_cost_usd symbols), _cost_usd never reaches snapshot() so nothing observable changes; "design incompleteness rather than cruft" is a distinction without a consumer, and deferring 9 dead lines the diff sits next to just schedules a second PR.

[FIRST-PRINCIPLES-REVIEWED] 279b23a

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 30, 2026
@iamwhatever
iamwhatever enabled auto-merge (squash) August 30, 2026 17:16

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iamwhatever
iamwhatever merged commit 0552cf6 into main Aug 30, 2026
67 checks passed
@iamwhatever
iamwhatever deleted the chore/dead-code-mcp-misc branch August 30, 2026 20:49
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants