Skip to content

refactor: remove dead execution paths and unify slide ingestion - #414

Closed
steipete wants to merge 1 commit into
codex/core-content-ownershipfrom
codex/remove-redundant-execution
Closed

refactor: remove dead execution paths and unify slide ingestion#414
steipete wants to merge 1 commit into
codex/core-content-ownershipfrom
codex/remove-redundant-execution

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Owner

What changed

Delete the unreachable daemon chat implementation and its private, implementation-only test suite. Production chat has already used the agent endpoint for both Q&A and automation; new complete/stream tests explicitly protect the chat-only prompt, page grounding, tool exclusion, and assistant delivery on that live path.

Delete unused whole-buffer browser audio adapters and an unused slide-cache getter. The bounded chunked decoder, media guards, and expiry-aware slide access remain unchanged.

Collapse three copied slide-download workflows into one cache/progress/cleanup path. Source policy still chooses direct HTTP or yt-dlp and preserves cookies, labels, local/cache short circuits, and the existing distinct fallback rules. Pure cache-key/format helpers no longer cross the injection boundary, downloader signatures come from their implementations, and unused cachedMedia return state is gone.

Net reduction: 391 production lines and 961 lines overall. The deleted daemon tests exercised only the unreachable implementation; all live-path scenarios remain, with eight new source/policy and agent regressions.

Proof

  • Root and extension builds pass.
  • 75 focused daemon, browser-media, and slide tests pass.
  • All three native Chromium scenarios pass, including installed native-host streaming and visible-tab slide capture.
  • Independent Codex review: no actionable P0–P2 findings.
  • Hosted CI run 33941355015 passes on head 33b957d: Node 24, Chromium E2E, Firefox smoke, and security checks.
  • The full project gate passes: formatting, lint, core/CLI type checks, 3,079 tests (43 skipped), 94.26% line and 85.15% branch coverage. The new streaming test initially omitted its required assistant callback; that test setup was corrected, then focused and full validation passed without a production change.

Stacked on #413. No live endpoint, user-visible UI, dependency, or release changes.

@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 11:28 PM ET / September 5, 2026, 03:28 UTC.

ClawSweeper review

What this changes

Removes unused daemon chat and browser media helpers, consolidates slide acquisition, and adds regression coverage for retained execution paths.

Merge readiness

Ready for maintainer review

Keep open: this remains useful cleanup absent from the reviewed main and latest release. The introduced diff preserves existing behavior, and no actionable correctness or security findings remain.

Priority: P3
Reviewed head: 33b957d5b76d0a353e7bd237250a4b43d53da9d7

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, behavior-preserving reduction with relevant regression coverage and no actionable findings.
Proof confidence 🌊 off-meta tidepool Not applicable: This OWNER-authored internal refactor is exempt from contributor proof, and it introduces no material authority change. Reported native-host streaming and visible-tab capture checks supplement the ingestion-policy and agent unit tests.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This OWNER-authored internal refactor is exempt from contributor proof, and it introduces no material authority change. Reported native-host streaming and visible-tab capture checks supplement the ingestion-policy and agent unit tests.
Evidence reviewed 10 items Repository policy applied: Read the complete root policy and checked for applicable nested policies and maintainer notes; none were found. Applied the package boundaries and Chromium validation guidance. Repository State identifies an OWNER-authored PR and disables automatic closure under the current repository profile.
Introduced changes verified: The complete local base-to-head diff contains the ten supplied files. Raw head parentage matches the pinned base; the test merge has that base followed by the exact head as parents and has the same tree as the head.
Deleted helpers have no production callers: A complete pinned-base symbol search found the removed audio adapters and slide getter only at their definitions; the removed chat function was referenced only by its deleted private test suite.
Findings None None.
Security None None.

How this fits together

Summarize’s slide ingestion turns video URLs or local files into inputs for slide extraction. The cleanup also touches browser audio decoding and daemon-backed page Q&A, whose active execution paths remain intact.

flowchart TD
  A[Video URL or local file] --> B{Local or cached input}
  B -->|Yes| E[Slide extraction]
  B -->|No| C[HTTP or yt-dlp download]
  C --> D[Shared cache and fallback handling]
  D --> E
  F[Page questions or audio] --> G[Agent endpoint or chunked decoder]
  G --> H[Replies or transcripts]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test footprint Production +46/-437 (net -391); tests +133/-707 (net -574) Most test deletions belong to the unreachable chat implementation, with eight new parameterized cases covering retained behavior.

Technical review

Best possible solution:

Preserve downloader choice, cache identity, cleanup, and guarded fallback behavior while reducing the maintained execution surface.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR proposes internal cleanup rather than a reported failing behavior; source comparison found no introduced defect.

Is this the best way to solve the issue?

Yes: consolidating the repeated acquisition workflow and removing unreferenced internal helpers is a bounded approach that preserves the supported execution paths.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6e20e4493ffa.

Labels

Label justifications:

  • P3: This is behavior-preserving internal cleanup with focused regression coverage and no demonstrated user-facing regression.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This OWNER-authored internal refactor is exempt from contributor proof, and it introduces no material authority change. Reported native-host streaming and visible-tab capture checks supplement the ingestion-policy and agent unit tests.

Evidence

What I checked:

  • Repository policy applied: Read the complete root policy and checked for applicable nested policies and maintainer notes; none were found. Applied the package boundaries and Chromium validation guidance. Repository State identifies an OWNER-authored PR and disables automatic closure under the current repository profile. (AGENTS.md:1, 33b957d5b76d)
  • Introduced changes verified: The complete local base-to-head diff contains the ten supplied files. Raw head parentage matches the pinned base; the test merge has that base followed by the exact head as parents and has the same tree as the head. (423d5acc46ed)
  • Deleted helpers have no production callers: A complete pinned-base symbol search found the removed audio adapters and slide getter only at their definitions; the removed chat function was referenced only by its deleted private test suite. (src/daemon/chat.ts:87, 3457a218f3b3)
  • Reachable chat and audio paths preserved: The extension posts daemon-backed chat to /v1/agent, whose handler invokes the retained complete/stream agent functions. Offscreen transcription calls the retained chunked audio processors; input limits, chunk limits, and expiry-aware slide retrieval remain unchanged. (apps/chrome-extension/src/entrypoints/background/panel-chat.ts:175, 33b957d5b76d)
  • Slide policies preserved: Compared every ingestion branch against the pinned base: local/cache shortcuts, downloader choice, cookies, cache keys, progress mapping, cleanup callbacks, and the distinct remote fallback rules are preserved. The extraction caller never consumed the removed cachedMedia result. (src/slides/ingest.ts:89, 33b957d5b76d)
  • Current main still contains the old implementations: Current main retains the deleted chat implementation and pre-refactor ingestion/browser-media blobs. Its relevant daemon, slide, and browser-media files match the pinned PR base, so the cleanup remains distinct work. (src/slides/ingest.ts:1, 6e20e4493ffa)

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Philipp Berner: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-05T03:21:39.986Z sha 33b957d :: needs maintainer review before merge. :: none

@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Landed through #415 in 8308778. The resulting main tree was verified byte-for-byte against the complete reviewed stack (33b957d), with green combined review, Node 24, Chromium E2E, Firefox smoke, and security checks. Closing this constituent PR as superseded; its changes and review history are preserved by the integration.

@steipete steipete closed this Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant