chore(apps): remove confirmed dead code in the smaller builtin apps - #6978
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of All claims verified against the repo: zero residual references to the deleted symbols, Design-Verdict: PASS Narrow, evidence-backed dead-code removal; the one behavior-adjacent hunk is disclosed, payload-identical, and closes a real aliasing footgun; risky fixes correctly deferred. [DESIGN-REVIEWED] ef4ceb4 |
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 Review detailsAll changes verify clean: No findings. [OPUS-REVIEWED] ef4ceb4 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) — 🟡 CONCERNSPremise-level review of All verification is done. The five declared deletions check out completely (zero residual consumers each), but the diff also ships undeclared work in First-Principles-Verdict: CONCERNS Five exemplary deletions as declared — plus four undeclared crew_companion/papyrus edits from apps the author's own table marks ineligible, one contradicting "No behaviour change". What this change shipsIntent: delete confirmed-dead code from the three ≥30-day-old builtin apps, with no behaviour change — a FIX (subtractive cleanup).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] ef4ceb4 |
Seven confirmed-dead regions across the builtin apps in this group, plus the
prose those removals falsify. Every response payload is unchanged; the one
non-cosmetic change is named below.
auto_research/subquestion_queue.py
- `analyzed_count`: read-only accessor, born unconsumed. The module's sole
production consumer is handlers.py, whose 11 `_sq.` call sites use 8 of 10
public functions; the cycle gate at handlers.py:2005 chose `pending_count`
instead. No inline `len(queue["analyzed"])` duplicate exists, so this is
unconsumed rather than drift, and handlers.py has been through 17 commits
since the rename baseline without adopting it.
- `is_known`: pre-check superseded by `enqueue`'s own documented internal
dedup ("De-duplicates (case/whitespace-insensitive) against everything
already in the queue AND within the batch"). Callers cannot need a pre-check
because `enqueue` self-dedups and returns only what it admitted; the
emergent-question path at handlers.py:1958-1984 dedups via `normalize` plus
its own `existing_norm` set.
workflows/server.py
- `_Handler._read_body`: orphaned by 5492a2c ("fix(security): backend
gateway hardening", #72), which removed the `body = self._read_body()` call
from `do_POST` and replaced it with an inline read because `_authorized()`
needs the raw bytes to verify the HMAC. The definition was left behind.
Compared dimension by dimension the inline path is not weaker: identical
Content-Length parse, identical `length <= 0` handling, identical JSON
dict-shape check and `JSONDecodeError` swallow, plus the HMAC verification
the parser never did. Neither version bounds Content-Length, so no size
bound is lost. `design_tweak` and `pptx_maker` have their own independent
copies; both are untouched and still called.
file_explorer/server.py
- `_entry_meta(with_size=)`: vestigial keyword-only parameter. Its three
production call sites (server.py:452, 553, 993) and its two test call sites
all use the default `True`, so `if with_size and kind == "file"` is always
`if kind == "file"` and the `False` path is unreachable.
- The `out: list[dict] = []` initializer in `_list_dir`: a dead store,
unconditionally overwritten by `out = walk(p, depth)`. The nested `walk()`
builds its own `items` and never touches the outer name. flake8's F841
misses it because `out` is read after the reassignment.
crew_companion — the presets had three definitions and two false comments
- `BREAK_PRESETS` (reminders.py): zero Python references of any kind. Its
comment named "the panel and the dashboard page" as the consumers that keep
the surfaces from drifting, but both are frontend and both read a TS
constant. The frontend one predates it by exactly six days (dcfaafd
2026-08-01 vs d54272d 2026-08-07), so it was born unwired rather than
superseded, and its neighbours show the intended split: the backend enforces
the RANGE via `clamp_break_mins` and has no use for the preset LIST.
- `website/src/apps/crew-companion/reminders.ts` carried a SECOND copy of the
constant under two stale comments: one dangling block attached to nothing
(the same false "the two surfaces cannot drift" prose), and one claiming the
floor and ceiling live there too -- they do not, line 10 imports them from
./constants. Collapsed to a re-export so PanelViews.tsx keeps importing from
'./reminders' while one definition remains. constants.ts imports nothing
local, so there is no cycle, and reminders.ts has no internal use of the
name, so the re-export creating no local binding is harmless.
- `constants.ts`: dropped the reciprocal claim that reminders.py holds the
backend mirror.
crew_companion — stop aliasing the colour store's internal dict
- `pack_detail` built its `colorMap` payload from `self._colour_maps.get(...)`
in both branches, putting the store's own mutable state into a response.
`colour_map()` exists precisely to prevent that -- it returns a copy and
normalises the id -- and had zero callers. Both reads now go through it.
`_safe_id` is idempotent on an already-normalised id and on DEFAULT_PACK
("kiro-ghost"), and `dict(get(k, {}))` matches `get(k) or {}` in the present,
empty and absent cases, so the payload is byte-identical.
Nothing observable changes: `pack_detail`'s only callers are routes.py:280,
which just serialises via `web.json_response`, and pack_transfer.py:236,
whose `build_bundle` ignores `detail["colorMap"]` and rebuilds from
`animations`/`categories`/`sprite`. Repo-wide there is no mutation of
`detail["colorMap"]`. So the alias was a real footgun but not reachable
today, and this closes it rather than fixing a live defect.
The two remaining `_colour_maps` reads are `bool(...)` truthiness checks
with no aliasing risk and are left alone.
papyrus/backend/store.py -- independent documentation fix, not removal-driven
- `ARTIFACT_SUFFIXES`' comment claimed filtering happens "here as well as in
the UI" so the two cannot drift. Neither half holds: `list_files` never
applies the set (its only reader is `is_artifact`, which has no production
caller), and the lists have already drifted -- the UI carries `.pdf` and
spells `.synctex.gz` as one suffix where this set has `.synctex` and `.gz`
separately, which would also hide any plain `.gz`. Comment corrected to
state where filtering actually happens. The set and `is_artifact` both stay:
they are inside the 30-day gate, so nothing is removed here.
Test changes are edits, not blanket deletions, and each premise was checked
first. `assert analyzed_count(q) == 1` becomes `assert len(q["analyzed"]) == 1`,
the shape the next line already uses; `test_mark_analyzed_then_blocks_reenqueue`
still exercises the full enqueue -> dequeue -> mark -> re-enqueue-blocked path.
`test_is_known` goes with its symbol -- its dedup coverage is redundant with
`test_dedup_against_existing` and the re-enqueue assertions in the same class,
so no real behaviour loses coverage.
Deliberately NOT in this diff: the `aws_control` upload timeout (declared
3600s, effectively 600s because both push paths omit `timeout=`) and the
bypassed `design_tweak` proxy Content-Type allowlist. Both are behavioural
changes on cloud-spend and security paths and need their own reviewed diffs;
folding them into a dead-code sweep would bury real risk.
5dc7630 to
ef4ceb4
Compare
UX Review (Fable 5) — ✅ PASSUX-level review of This is a dead-code cleanup PR: removed unused Python helpers ( UX-Verdict: PASS Pure dead-code removal; no user-facing string, control, state, or preset value changes — the rendered experience is byte-identical. [UX-REVIEWED] ef4ceb4 |
CI note on
|
bolichen97
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: chore (9 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: dead-code removal in smaller builtin apps (unused analyzed_count/is_known, dead BREAK_PRESETS, unused with_size param, dead _read_body, plus copy-on-return colour_map and re-export consolidation) — all removed symbols verified unreferenced on the merged tree, no runtime behaviour change.
chenmingwei23
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: chore (9 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean (Semgrep + CodeQL success, no PR-scoped alerts), security checklist all-NO, AI reviewers green. Category: chore, confirmed dead-code removal across smaller builtin apps with no runtime behaviour change.
What
Removes seven confirmed-dead regions from the smaller builtin apps, and corrects the prose those removals falsify. Every response payload is unchanged; the one non-cosmetic change is named below.
This is the D group of the builtin-apps dead-code audit:
crew_companion,meetings,pptx_maker,papyrus,md_notebook,design_tweak,auto_research,aws_control,personal_shopper,file_explorer,design_critique,workflows,projects— 98 files, 41,110 lines, 6,057 in-scope definitions.Why no screenshot: the two frontend files change a doc comment and collapse a
duplicated
BREAK_PRESETSconstant into a re-export of the identical value, so norendered pixel moves —
Frontend5CcSections.cov80.test.tsx(20/20) covers thepreset pills that consume it.
No linked issue to close: #7085 and #7086 are follow-ups this PR deliberately
does NOT fix — they are behavioural changes on security and cloud-spend paths that
need their own reviewed diffs. Nothing here should close on merge.
Why the deletions cluster in three apps: the 30-day new-code gate
Ten of these apps are newer than 30 days, so their unreferenced symbols are not eligible for a cleanup branch — a symbol born dead last week belongs back with its author, not here.
auto_research,file_explorer,workflowsdesign_critique,crew_companionpapyrus,meetings,md_notebook,projectspptx_makerpersonal_shopperdesign_tweakaws_control2026-07-16 is the
KiroClaw→KiroCrewrename (849a03fff, #2), the effective repo epoch. Thecrew_companionconstant below is the one exception, deleted on the maintainer's explicit call.Deleted
auto_research/subquestion_queue.pyanalyzed_count— read-only accessor, born unconsumed. Whole-repo search: 3 sites, the definition plus 2 intest/test_subquestion_queue.py. The module's only production consumer isauto_research/handlers.py(import subquestion_queue as _sq), whose 11_sq.call sites use 8 of the 10 public functions; the cycle gate athandlers.py:2005chosepending_countinstead. No inlinelen(queue["analyzed"])duplicate exists, so this is unconsumed rather than drift, andhandlers.pyhas been through 17 commits since the rename baseline without adopting it.is_known— pre-check superseded byenqueue's own documented internal dedup ("De-duplicates (case/whitespace-insensitive) against everything already in the queue AND within the batch"). Callers cannot need a pre-check becauseenqueueself-dedups and returns only what it admitted; the emergent-question path athandlers.py:1958-1984dedups vianormalizeplus its ownexisting_normset. Of 17 repo-wide name hits only 4 are this symbol — the rest are a 2-arg_is_knownlocal totest_messaging_import_purity.pyand six unrelated..._is_known...test names._normand_known_keysremain live (both called byenqueue).workflows/server.py_Handler._read_body— orphaned by5492a2cb9("fix(security): backend gateway hardening — 17 CSE findings (Batch 1)", #72), which removed thebody = self._read_body()call fromdo_POSTand replaced it with an inline read because_authorized()needs the raw bytes to verify the HMAC. The definition was left behind. No caller, no_Handlersubclass, no string dispatch.Compared dimension by dimension, the inline path is not weaker: identical Content-Length parse (
int(... or 0)), identicallength <= 0→b""handling, identical JSON dict-shape check andJSONDecodeErrorswallow, plus the HMAC verification the parser never did. Neither version bounds Content-Length, so no size bound is lost — the unboundedrfile.read(length)is pre-existing in both and untouched here.design_tweak/backend/server.pyandpptx_maker/backend/routes.pyhave their own independent_read_body; both are untouched and still called.file_explorer/server.py_entry_meta(*, with_size=)— vestigial keyword-only parameter. Its three production call sites (server.py:452, 553, 993) and its two test call sites all use the defaultTrue, soif with_size and kind == "file"is alwaysif kind == "file"and theFalsepath is unreachable. Simplified accordingly.The
out: list[dict] = []initializer in_list_dir— dead store, unconditionally overwritten byout = walk(p, depth)with no read in between; the nestedwalk()builds its ownitemsand never touches the outer name.flake8's F841 misses it becauseoutis read after the reassignment.crew_companion— the presets had three definitions and two false commentsBREAK_PRESETS(reminders.py) — zero Python references of any kind. Its comment named "the panel and the dashboard page" as the consumers that keep the surfaces from drifting, but both are frontend and both read a TS constant. The frontend one predates it by exactly six days (dcfaafd2a2026-08-01 vsd54272dce2026-08-07), so it was born unwired rather than superseded, and its neighbours show the intended split: the backend enforces the RANGE viaclamp_break_minsand has no use for the preset LIST.website/src/apps/crew-companion/reminders.tscarried a SECOND copy of the constant under two stale comments: one dangling block attached to nothing (carrying the same false "the two surfaces cannot drift" prose as the Python side), and one claiming the floor and ceiling live there too — they do not, line 10 imports them from./constants. Collapsed to a re-export soPanelViews.tsxkeeps importing from'./reminders'while one definition remains.constants.tsimports nothing local, so there is no cycle, andreminders.tshas no internal use of the name, so the re-export creating no local binding is harmless.constants.ts— dropped the reciprocal claim thatreminders.pyholds the backend mirror.Changed:
crew_companionstops aliasing the colour store's internal dictpack_detailbuilt itscolorMappayload fromself._colour_maps.get(...)in both branches, putting the store's own mutable state into a response.colour_map()exists precisely to prevent that — it returns a copy and normalises the id — and had zero callers. Both reads now go through it._safe_idis idempotent on an already-normalised id and onDEFAULT_PACK("kiro-ghost"), anddict(get(k, {}))matchesget(k) or {}in the present, empty and absent cases, so the payload is byte-identical.Nothing observable changes.
pack_detail's only callers areroutes.py:280, which just serialises viaweb.json_response, andpack_transfer.py:236, whosebuild_bundleignoresdetail["colorMap"]and rebuilds fromanimations/categories/sprite. Repo-wide there is no mutation ofdetail["colorMap"]. So the alias was a real footgun but not reachable today, and this closes it rather than fixing a live defect.The two remaining
_colour_mapsreads arebool(...)truthiness checks with no aliasing risk and are left alone.Changed:
papyrusartifact-filter comment — independent documentation fixNot driven by any removal in this diff; called out separately for that reason.
ARTIFACT_SUFFIXES' comment claimed filtering happens "here as well as in the UI" so the two cannot drift. Neither half holds:list_filesnever applies the set (its only reader isis_artifact, which has no production caller), and the lists have already drifted — the UI carries.pdfand spells.synctex.gzas one suffix where this set has.synctexand.gzseparately, which would also hide any plain.gz. The comment now states where filtering actually happens.The set and
is_artifactboth stay: they are inside the 30-day gate, so nothing is removed here.Test changes are edits, not blanket deletions
Each premise was checked before touching it.
assert analyzed_count(q) == 1→assert len(q["analyzed"]) == 1, the shape the next line already uses (q["analyzed"][0]["status"]).test_mark_analyzed_then_blocks_reenqueuestill exercises the fullenqueue → dequeue_top_k → mark_analyzed → re-enqueue-blockedpath.test_is_knowngoes with its symbol. Its dedup coverage is redundant with the live path —test_dedup_against_existingand the re-enqueue assertions in the same class both exercise dedup throughenqueue— so no real behaviour loses coverage.How the candidates were found
Four scanner passes, all required:
--min-confidence 60→ 77 findings, confirmed to over-report on a scoped subset:is_owned_research_slotis imported bydashboard/session_directive_apply.py:51,411, outside the scanned directories.AnnAssign/arg.annotation/returns/cast()and counts the names inside as references.Pass 4 was load-bearing, not defensive: vulture flagged
AsyncIterator(md_notebook/server.py:34) at 90% confidence as an unused import, but it is used at line 211 inside the string annotation"AsyncIterator[None]". Deleting it would have broken the module.from __future__ import annotationsmakes every annotation a string at runtime, so passes 1-3 are structurally blind to that whole class of reference.Two of the deletions here were invisible to all four passes and were found only by reading the smaller apps directly:
_read_bodybecause name matching is repo-global and its live twins indesign_tweak/pptx_makermasked the orphaned copy, andwith_sizebecause a vestigial parameter is not a definition at all.Verification
origin/mainin a separate worktree —AF_UNIX path too long,/local/homeowned by uid 65534, no Node ≥ 20, a jq version mismatch. The sampledspec_builderfailure (assert 'unsupported_platform' == 'write_failed') was checked specifically and is pre-existing.crew_companion/tests/,test_subquestion_queue.py,test_auto_research.py,test_papyrus_store.py,test_workflows_app.py.tsc -bclean,Frontend5CcSections.cov80.test.tsx20/20,eslint0 errors oncrew-companion/andpapyrus/(one pre-existingreact-hooks/exhaustive-depswarning inCrewCompanionPage.tsx, untouched),i18n:checkexit 0 withI18N_BASE_REF.mypy src/kiro_crew/clean, 1177 files.isortandflake8clean over all ofsrc/kiro_crewandtest.check_brand_name,check_harness_parity,check_changelog_history,check_focus_cue— all pass. Pluscheck_black_formatting,check_subprocess_encoding,check_lockdown_before_publish,check_loop_bound_locks,check_testpaths_coverage,verify_vendor_manifest.analyzed_count,is_known,with_size,BREAK_PRESETS(Python), or the workflows_read_bodyrepo-wide.crew_companion/reminders.py), and only its module docstring at lines 22-32 — no conflict.gpt-5.6-sol: No findings after verifying every new comment line against the code and testing the byte-identity claim across all input classes.claude-opus-4.8: no Critical/High/Medium; its four Low findings on message precision are all folded in (the_entry_metacall-site count now separates production from test, the colour_map hunk is described as non-cosmetic rather than behavioural with its reachability spelled out, and the commit-count claim is now stated against the rename baseline).Not touched — 21 deferred symbols
Recorded in full with evidence, not silently dropped.
30-day gate, otherwise deletable (1).
UnsupportedPlatform(papyrus/backend/tectonic.py:234) — genuinely dead, superseded by the documentedcurrent_asset() -> Nonedegradation; all 25 other name hits arecode_review_sage's separate class. Eligible 2026-09-01.Test-only references, refactor not deletion (8).
reset_provision_state,is_artifact(papyrus);reset_uv_cache(pptx_maker);shared_dictionary,write_agent_output,read_transcript(meetings);is_local_remote(md_notebook);PreferenceStore.close(personal_shopper).read_transcriptis the best-evidenced of these — a one-line wrapper over the paged form production actually uses (routes/meeting_lifecycle.py:183), whose five test premises all survive a mechanical substitution already used by sibling tests in the same class. It is blocked by nothing but the gate at 20 days.Declared public surface, report only (6).
_register_resources,_BITMAP_URI_TERMINATORS,missing_optional_deps(pptx_maker);register_calendar_provider,register_task_provider,MAX_CONCURRENT_MEETINGS(meetings).Guard / validation / permission — deadness would be a defect, never a deletion (3).
STATE_DIR_LEAF,_WINDOWS_CMDLINE_MAX(aws_control);is_user_owned(pptx_maker).Two trap notes for whoever picks these up:
test/test_meetings_routes.py:2014carries"write_agent_output"as a string insideTestNoStoreCallRunsOnTheEventLoop._BLOCKING_STORE_FNS, so a name-based scanner reads it as live and deleting the function without editing that frozenset leaves it stale. Andis_local_remote's security concern is disproved, not deferred — it is a classifier, never a gate; all six network-reach controls are live and independently tested.Apps not scanned: none. All 13 in-scope apps went through all four passes.
command_bar/,channels/,auto_triage_pipeline/andagent_worlds/carry zero Python lines.Two defects deliberately left out — filed as issues
Both are behavioural changes on cloud-spend and security paths and need their own reviewed diffs; folding them into a dead-code sweep would bury real risk. Both reviewer lanes agreed with the split.
design_tweakproxy Content-Type allowlist is bypassedaws_controlnightly push is bounded at 600s, not the declared 3600sdesign_tweakproxy Content-Type allowlist is bypassed (fix(design-tweak): proxy Content-Type allowlist is bypassed — _safe_upstream_ctype has zero callers #7085)._safe_upstream_ctype(server.py:999) selects a literal from a 30-entry allowlist whose comment names the CVE class it closes (py/http-response-splitting), and has zero callers._DevProxyHandler._relay_httpforwards the upstreamContent-Typeverbatim through_header_valueonly. Response splitting stays closed (CR/LF stripped at the sink); what is lost is forcedcharset=utf-8on text types and the collapse of exotic media types toapplication/octet-stream— on a body rendered at127.0.0.1, the dashboard's own cookie host. Two statements in the tree are false about this, and one is a passing test (test_design_tweak_backend.py:1925) asserting "The proxy path keeps its own selector." Born dead ine6dfd22ee(feat(apps): add Design Tweak builtin (visual select-to-edit) #1122) alongside the sink guard that replaced it.aws_controlnightly push runs on a 600s bound, not the declared 3600s (fix(aws-control): nightly push is bounded at 600s, not the declared 3600s — _PUSH_TIMEOUT_SECS is never applied #7086)._PUSH_TIMEOUT_SECS = 3600has one site, its definition. Both push paths callstorage.put_filewith notimeout=, taking its 600s default, and noasyncio.wait_forexists anywhere on the backup path. From2d74cf533(feat: AWS Control - account portal + S3-backed cloud drive #5517). Fix by wiring it, not deleting it.