Skip to content

refactor: land reviewed architecture cleanup stack - #415

Merged
steipete merged 3 commits into
mainfrom
codex/land-architecture-cleanups
Sep 5, 2026
Merged

refactor: land reviewed architecture cleanup stack#415
steipete merged 3 commits into
mainfrom
codex/land-architecture-cleanups

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Land the reviewed architecture cleanup stack

Integrate the reviewed code from #412, #413, and #414 without changing their tested tree. This PR targets main because the local GitHub wrapper does not support retargeting an existing PR. Its supported checked-head squash merge preserves the code; the constituent PRs retain their review history and will be closed with links to this integration.

The combined changes remove copied stream/browser test lifecycles, no-op DOM cleanup, duplicate YouTube parsers, the unreachable daemon chat pipeline, unused browser media adapters, and repeated slide ingestion. Core now owns the transcript-source inventory, fixing lost embedded-caption and native YouTube source diagnostics on cache hits.

Compared with current main, this removes 2,547 lines overall and 600 production lines. The already-landed panel state cleanup is #411 and is not duplicated here.

Proof

  • The integration tree is exactly 33b957d, already verified by full Node 24, Chromium E2E, Firefox smoke, and security CI in run 33941355015.
  • Root and extension builds pass. The full local gate passes 3,079 tests (43 skipped), with 94.26% line and 85.15% branch coverage.
  • Each constituent change and the combined diff have independent Codex reviews with no actionable P0–P2 findings. This PR's CI run 33942296172 also passes on the exact integration head: Node 24, Chromium E2E, Firefox smoke, and security checks.
  • Four local browser-startup timeouts passed unchanged when both affected suites were rerun; the full hosted browser suite subsequently passed.

The original PRs will be closed as superseded after verifying that main matches their combined tree exactly. No release or dependency 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:53 PM ET / September 5, 2026, 03:53 UTC.

ClawSweeper review

What this changes

Consolidates content parsing, transcript-source validation, slide ingestion, and test fixtures while removing unused daemon-chat and browser-media code.

Merge readiness

Ready for maintainer review

This remains a useful integration PR: main and the latest release still contain the cache-source defect, and the constituent changes remain unmerged. No actionable correctness or security findings were identified.

Priority: P3
Reviewed head: 33b957d5b76d0a353e7bd237250a4b43d53da9d7

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A coherent cleanup with targeted regression coverage, successful supplied checks, and no blocking defect found.
Proof confidence 🌊 off-meta tidepool Not applicable: This OWNER-authored cleanup is exempt from ordinary contributor proof requirements and introduces no material authority change. Supplied regression and browser checks support the parsing, cache, slide, and fixture changes as supplemental validation.
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 cleanup is exempt from ordinary contributor proof requirements and introduces no material authority change. Supplied regression and browser checks support the parsing, cache, slide, and fixture changes as supplemental validation.
Evidence reviewed 9 items Repository policy: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-note directory was found. Applied the core/CLI boundary and supported browser-validation guidance. Repository policy and OWNER authorship also prevent automatic closure.
Pinned integration and re-review continuity: The checkout matches the previously reviewed head. Both earlier stack commits are ancestors of it. The test merge has exactly the pinned main and PR-head parents, and its tree matches the reviewed head.
Still necessary on main: Main's SQLite validator omits embedded and youtube-media despite both being existing transcript sources; cache reads consequently discard those source diagnostics.
Findings None None.
Security None None.

How this fits together

Summarize extracts text, transcripts, and slides from URLs and local files for its CLI and browser extension. These changes consolidate the extraction and caching paths and the fixtures used to test their consumers.

flowchart TD
  A[URLs and local files] --> B[Core content parsers]
  B --> C[Transcript cache]
  B --> E[CLI and extension output]
  C --> E
  A --> D[Slide ingestion]
  D --> E
  F[Test scenarios] --> G[Shared test fixtures]
  G --> E
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC Production +351/−951 (net −600); tests +5,728/−7,683 (net −1,955) Production additions consolidate existing workflows and preserve source diagnostics; most test churn moves repeated fixture lifecycles.

Root-cause cluster

Relationship: canonical
Canonical: #415
Summary: This is the explicitly designated integration path for three open stack PRs; the earlier panel cleanup is already part of the base.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Technical review

Best possible solution:

Retain one implementation per workflow and preserve the three reviewed stack commits through the requested merge-commit landing.

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

Yes for the included cache defect: main rejects existing embedded and youtube-media source values during cache reads. This is established from source, without executing a reproduction.

Is this the best way to solve the issue?

Yes. Shared implementations remove duplication while preserving current routing and fallback behavior; the cache repair changes readers without requiring a stored-data migration.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P3: This is primarily architecture and test cleanup, with a bounded correction to cache-source diagnostics and no urgent 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 cleanup is exempt from ordinary contributor proof requirements and introduces no material authority change. Supplied regression and browser checks support the parsing, cache, slide, and fixture changes as supplemental validation.

Evidence

What I checked:

  • Repository policy: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-note directory was found. Applied the core/CLI boundary and supported browser-validation guidance. Repository policy and OWNER authorship also prevent automatic closure. (AGENTS.md:1, 33b957d5b76d)
  • Pinned integration and re-review continuity: The checkout matches the previously reviewed head. Both earlier stack commits are ancestors of it. The test merge has exactly the pinned main and PR-head parents, and its tree matches the reviewed head. (bfd9f5305c17)
  • Still necessary on main: Main's SQLite validator omits embedded and youtube-media despite both being existing transcript sources; cache reads consequently discard those source diagnostics. (src/cache.ts:81, 6e20e4493ffa)
  • Latest release retains the defect: The v0.21.11 source contains the same incomplete validator. The annotated release tag resolves to the supplied release commit. (src/cache.ts:81, 1dcbff752de7)
  • Cache compatibility: The patch shares the existing source inventory between readers without changing serialized writes, keys, or schemas. Regression coverage exercises both affected sources through SQLite and core cache reads; unknown-value handling remains covered. (tests/cache.store.test.ts:266, 33b957d5b76d)
  • Removed paths and preserved fallback policy: Main-tree searches found no production callers for the deleted chat and browser-media helpers. The active agent route remains unchanged, and the consolidated slide workflow preserves the distinct YouTube, extracted-remote, and direct-media fallback rules. (src/slides/ingest.ts:139, 33b957d5b76d)

Likely related people:

  • steipete: 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:42:19.331Z sha 33b957d :: needs maintainer review before merge. :: none

@steipete
steipete merged commit 8308778 into main Sep 5, 2026
7 checks passed
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