Skip to content

chore(apps): remove confirmed dead code in the smaller builtin apps - #6978

Merged
chenmingwei23 merged 1 commit into
mainfrom
chore/dead-code-app-small
Aug 30, 2026
Merged

chore(apps): remove confirmed dead code in the smaller builtin apps#6978
chenmingwei23 merged 1 commit into
mainfrom
chore/dead-code-app-small

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

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_PRESETS constant into a re-export of the identical value, so no
rendered pixel moves — Frontend5CcSections.cov80.test.tsx (20/20) covers the
preset 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.

app oldest file age eligible
auto_research, file_explorer, workflows 2026-07-16 45d yes
design_critique, crew_companion 2026-08-01 29d no
papyrus, meetings, md_notebook, projects 2026-08-02 28d no
pptx_maker 2026-08-03 27d no
personal_shopper 2026-08-10 20d no
design_tweak 2026-08-16 14d no
aws_control 2026-08-26 4d no

2026-07-16 is the KiroClawKiroCrew rename (849a03fff, #2), the effective repo epoch. The crew_companion constant below is the one exception, deleted on the maintainer's explicit call.

Deleted

auto_research/subquestion_queue.py

analyzed_count — read-only accessor, born unconsumed. Whole-repo search: 3 sites, the definition plus 2 in test/test_subquestion_queue.py. The module's only production consumer is auto_research/handlers.py (import subquestion_queue as _sq), whose 11 _sq. call sites use 8 of the 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. Of 17 repo-wide name hits only 4 are this symbol — the rest are a 2-arg _is_known local to test_messaging_import_purity.py and six unrelated ..._is_known... test names.

_norm and _known_keys remain live (both called by enqueue).

workflows/server.py

_Handler._read_body — orphaned by 5492a2cb9 ("fix(security): backend gateway hardening — 17 CSE findings (Batch 1)", #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. No caller, no _Handler subclass, no string dispatch.

Compared dimension by dimension, the inline path is not weaker: identical Content-Length parse (int(... or 0)), identical length <= 0b"" 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 — the unbounded rfile.read(length) is pre-existing in both and untouched here.

design_tweak/backend/server.py and pptx_maker/backend/routes.py have 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 default True, so if with_size and kind == "file" is always if kind == "file" and the False path is unreachable. Simplified accordingly.

The out: list[dict] = [] initializer in _list_dir — dead store, unconditionally overwritten by out = walk(p, depth) with no read in between; 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 (dcfaafd2a 2026-08-01 vs d54272dce 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 (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 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.

Changed: crew_companion stops 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.

Changed: papyrus artifact-filter comment — independent documentation fix

Not 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_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. The comment now states 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

Each premise was checked before touching it.

  • assert analyzed_count(q) == 1assert len(q["analyzed"]) == 1, the shape the next line already uses (q["analyzed"][0]["status"]). test_mark_analyzed_then_blocks_reenqueue still exercises the full enqueue → dequeue_top_k → mark_analyzed → re-enqueue-blocked path.
  • test_is_known goes with its symbol. Its dedup coverage is redundant with the live path — test_dedup_against_existing and the re-enqueue assertions in the same class both exercise dedup through enqueue — so no real behaviour loses coverage.
  • Two imports dropped.

How the candidates were found

Four scanner passes, all required:

  1. vulture --min-confidence 60 → 77 findings, confirmed to over-report on a scoped subset: is_owned_research_slot is imported by dashboard/session_directive_apply.py:51,411, outside the scanned directories.
  2. naive AST — defined names minus Load-context names.
  3. tokenize — real NAME tokens only, excluding same-named strings in docstrings and comments.
  4. annotation-aware AST — re-parses string constants in 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 annotations makes 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_body because name matching is repo-global and its live twins in design_tweak/pptx_maker masked the orphaned copy, and with_size because a vestigial parameter is not a definition at all.

Verification

  • Full backend suite: 75,251 passed. The 114 failures are environmental and reproduce identically on pristine origin/main in a separate worktree — AF_UNIX path too long, /local/home owned by uid 65534, no Node ≥ 20, a jq version mismatch. The sampled spec_builder failure (assert 'unsupported_platform' == 'write_failed') was checked specifically and is pre-existing.
  • Scoped after the final rebase: 654 passed across crew_companion/tests/, test_subquestion_queue.py, test_auto_research.py, test_papyrus_store.py, test_workflows_app.py.
  • Frontend: tsc -b clean, Frontend5CcSections.cov80.test.tsx 20/20, eslint 0 errors on crew-companion/ and papyrus/ (one pre-existing react-hooks/exhaustive-deps warning in CrewCompanionPage.tsx, untouched), i18n:check exit 0 with I18N_BASE_REF.
  • mypy src/kiro_crew/ clean, 1177 files. isort and flake8 clean over all of src/kiro_crew and test.
  • Diff-scoped gates run with their base refs so they enforce rather than report: check_brand_name, check_harness_parity, check_changelog_history, check_focus_cue — all pass. Plus check_black_formatting, check_subprocess_encoding, check_lockdown_before_publish, check_loop_bound_locks, check_testpaths_coverage, verify_vendor_manifest.
  • Zero residual references to analyzed_count, is_known, with_size, BREAK_PRESETS (Python), or the workflows _read_body repo-wide.
  • File-overlap gate: 282 open PRs scanned. Only PR#3013 touches a file of mine (crew_companion/reminders.py), and only its module docstring at lines 22-32 — no conflict.
  • Two local reviewer lanes before push. 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_meta call-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 documented current_asset() -> None degradation; all 25 other name hits are code_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_transcript is 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:2014 carries "write_agent_output" as a string inside TestNoStoreCallRunsOnTheEventLoop._BLOCKING_STORE_FNS, so a name-based scanner reads it as live and deleting the function without editing that frozenset leaves it stale. And is_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/ and agent_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.

  1. design_tweak proxy 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_http forwards the upstream Content-Type verbatim through _header_value only. Response splitting stays closed (CR/LF stripped at the sink); what is lost is forced charset=utf-8 on text types and the collapse of exotic media types to application/octet-stream — on a body rendered at 127.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 in e6dfd22ee (feat(apps): add Design Tweak builtin (visual select-to-edit) #1122) alongside the sink guard that replaced it.
  2. aws_control nightly 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 = 3600 has one site, its definition. Both push paths call storage.put_file with no timeout=, taking its 600s default, and no asyncio.wait_for exists anywhere on the backup path. From 2d74cf533 (feat: AWS Control - account portal + S3-backed cloud drive #5517). Fix by wiring it, not deleting it.

@iamwhatever
iamwhatever requested a review from a team as a code owner August 30, 2026 09:34
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 30, 2026
@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

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

All claims verified against the repo: zero residual references to the deleted symbols, _read_body fully gone from the workflows app, and the two remaining _colour_maps direct reads are indeed the truthiness checks the description says were deliberately left. The colour_map change routes through an existing copy-returning accessor with identical payload, and the deferred behavioral defects (design_tweak proxy, aws_control timeout) are correctly split out rather than smuggled in. No design-level issues found.

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

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ef4ceb4

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

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

All changes verify clean: analyzed_count/is_known/BREAK_PRESETS (Python)/_read_body have no remaining callers; the three _entry_meta calls all used the default with_size=True, so dropping the param preserves behavior; and colour_map(DEFAULT_PACK)/colour_map(ident) resolve to the identical lookup key (_safe_id is idempotent on already-safe ids) while adding a defensive copy. The papyrus change is comment-only.

No findings.

[OPUS-REVIEWED] ef4ceb4

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

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

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ef4ceb482bbfef369098b69c2860dace1cc8ef05 — 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 verification is done. The five declared deletions check out completely (zero residual consumers each), but the diff also ships undeclared work in crew_companion and papyrus — two apps the author's own eligibility table marks "no". Here is the review:

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 ships

Intent: delete confirmed-dead code from the three ≥30-day-old builtin apps, with no behaviour change — a FIX (subtractive cleanup).

  1. analyzed_count deleted from auto_research — justified (0 consumers, verified)
  2. is_known deleted from auto_research — justified (enqueue self-dedups)
  3. Orphaned _Handler._read_body deleted from workflows — justified (0 callers, verified)
  4. _entry_meta loses always-default with_size param — justified (all 5 sites used default)
  5. Dead out initializer removed in _list_dir — justified
  6. Backend BREAK_PRESETS deleted from crew_companion — rides along; sound (0 backend consumers, grepped BREAK_PRESETS)
  7. Frontend reminders.ts copy of BREAK_PRESETS becomes a re-export of constants.ts — rides along; one consumer, generalized
  8. pack_detail colorMap now a COPY via colour_map(), not the stored dict — rides along; contradicts "No behaviour change"
  9. Papyrus ARTIFACT_SUFFIXES comment rewritten to document backend/UI drift — rides along, ineligible app per author's own gate

Watch

  • Description says "All five deletions come from the three eligible apps" and "No behaviour change", yet the diff deletes a sixth region (item 6) from crew_companion (29d, "no" in the author's table) and changes aliasing semantics (item 8). The intent file truncates at 8000 bytes so the tail may declare these, but the headline framing undercounts either way. Each rider removes real harm (a false "cannot drift" comment on a triplicated constant; a second spelling of colour_map that aliased store state), so no rider meets the zero-cost BLOCK bar.

Subtractions

  • Drop the export { BREAK_PRESETS } from './constants' shim in website/src/apps/crew-companion/reminders.ts:91 — exactly 1 consumer (PanelViews.tsx:19, grepped from './reminders'); point that import at ./constants instead.
  • Shrink the 9-line drift comment on ARTIFACT_SUFFIXES (papyrus/backend/store.py:73-79): its sole reader is_artifact has 0 production consumers (grepped is_artifact repo-wide: definition, 2 test hits, 1 docstring mention), so the dead chain belongs with the 22 deferred deletions, not a reconciliation TODO.

[FIRST-PRINCIPLES-REVIEWED] ef4ceb4

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 30, 2026
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.
@iamwhatever
iamwhatever force-pushed the chore/dead-code-app-small branch from 5dc7630 to ef4ceb4 Compare August 30, 2026 18:13
@iamwhatever
iamwhatever requested a review from a team August 30, 2026 18:13
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

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

This is a dead-code cleanup PR: removed unused Python helpers (analyzed_count, is_known, _read_body, an unused with_size parameter, BREAK_PRESETS on the backend), plus comment/docstring updates. The only frontend change consolidates BREAK_PRESETS into constants.ts with identical values (30, 45, 60, 90) and re-exports it — no rendered UI, string, flow, or state changes anywhere, and no screenshots.

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

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

CI note on Backend Tests (Windows) (2)

Classified as a pre-existing flake, not a defect in this diff. Evidence:

  • The only annotation is Event loop is closed — an asyncio teardown race, with no failing test named. That string appears in 55 existing issues/comments in this repo, so it is an established recurring flake rather than a new signal.
  • main itself failed CI twice in the hours before this run (15:27 and 15:56 UTC today), so the baseline is not green.
  • Locally, the full test scope this diff touches passes: 304 passed across crew_companion/tests/ and test_subquestion_queue.py, and 654 passed including test_auto_research.py, test_papyrus_store.py and test_workflows_app.py.
  • Nothing in the diff adds, removes or reorders an async path. The one Python behaviour change (pack_detail reading colorMap through colour_map()) is synchronous and byte-identical in output.

Not patching it. A fresh run is already in flight from the description edits; if the same shard reds again with a named failing test, I will treat that as a real signal and investigate.

On the two gates that were red for a reason

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 30, 2026

@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.

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
chenmingwei23 enabled auto-merge (squash) August 30, 2026 19:07
@chenmingwei23
chenmingwei23 merged commit df33617 into main Aug 30, 2026
129 of 135 checks passed
@chenmingwei23
chenmingwei23 deleted the chore/dead-code-app-small branch August 30, 2026 19:08

@chenmingwei23 chenmingwei23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@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.

3 participants