Skip to content

refactor(core): consolidate content parsing and cache source ownership - #413

Closed
steipete wants to merge 1 commit into
codex/shared-test-fixturesfrom
codex/core-content-ownership
Closed

refactor(core): consolidate content parsing and cache source ownership#413
steipete wants to merge 1 commit into
codex/shared-test-fixturesfrom
codex/core-content-ownership

Conversation

@steipete

@steipete steipete commented Sep 5, 2026

Copy link
Copy Markdown
Owner

What changed

Remove the DOM adapter's fictional lifetime: parseHtmlDocument now returns its Document directly, and callers no longer wrap extraction in finally blocks that invoke a no-op close. Real error handling, document normalization, and test marker cleanup remain intact.

Consolidate three balanced-JSON scanners and two initial-player parsers into one implementation shared by YouTube captions, metadata, and page descriptions. Public YouTube parsing no longer imports a caption HTTP module merely to read JSON.

Make the transcript source tuple own both the source type and cache validation. This fixes a real drift bug: SQLite discarded embedded and youtube-media sources, and core cache reads downgraded youtube-media to unknown. Unknown persisted values retain their prior handling.

Net reduction: 209 production lines and 136 lines overall, with new parser edge cases and SQLite-to-core cache round-trip regression coverage. No compatibility wrappers remain.

Proof

  • Build and full project gate pass: formatting, lint, core/CLI type checks, 3,088 tests (43 skipped), 94.27% line and 85.17% branch coverage.
  • Focused HTML, Reddit, YouTube, and cache suites: 61 tests pass.
  • New tests cover nested/quoted/escaped braces, malformed and first-assignment handling, retained newlines, and both formerly lost cache sources.
  • Independent Codex review of the final diff: no actionable P0–P2 findings.

A new whitespace test initially expected a space; validation correctly showed the existing normalizer preserves one newline. The expectation was corrected without changing production behavior, and the full gate and review were rerun.

Stacked on #412. No dependency or public CLI behavior changes beyond preserving cache-source diagnostics.

@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 P2 Normal priority bug or improvement with limited blast radius. 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: blocked before merge. Reviewed September 4, 2026, 11:06 PM ET / September 5, 2026, 03:06 UTC.

ClawSweeper review

What this changes

The PR simplifies HTML document handling, shares YouTube JSON parsing, and preserves transcript-source diagnostics across cache reads.

Merge readiness

Blocked before merge - 2 items remain

The cache-source defect remains in current main and v0.21.11, so this PR remains useful. The introduced patch has no actionable correctness findings; repository policy also excludes automatic closure of this owner-authored work.

Priority: P2
Reviewed head: 3457a218f3b38ed7b330fa4ee341975d7ef7416d

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused consolidation with meaningful regression coverage and no identified correctness defect.
Proof confidence 🌊 off-meta tidepool Not applicable: This OWNER-authored refactor is exempt from contributor runtime proof; the reported parser and SQLite round-trip tests provide supplemental validation, and no material authority change triggers additional proof.
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 refactor is exempt from contributor runtime proof; the reported parser and SQLite round-trip tests provide supplemental validation, and no material authority change triggers additional proof.
Evidence reviewed 10 items Repository policy: Read the full root policy; no nested AGENTS.md or maintainer-notes directory was found. Applied the core-library versus CLI boundary and read-only review restrictions.
Introduced patch identity: The original head records the pinned base as its parent. The verified test merge records that base followed by the exact head and has the same tree as the head; it does not establish a merge result against fetched main.
Defect remains on main: Main’s SQLite source whitelist omits embedded and youtube-media; its core cache mapper also maps youtube-media to unknown.
Findings None None.
Security None None.

How this fits together

Summarize’s core library extracts article text, media metadata, and transcripts from web content. Its CLI stores transcripts in SQLite and passes cached results back through core for summaries and diagnostics.

flowchart LR
  A[Web page HTML] --> B[HTML and YouTube parsing]
  B --> C[Article and media extraction]
  C --> D[Transcript result]
  D --> E[SQLite cache]
  E --> F[Core cache reader]
  G[Shared source inventory] --> E
  G --> F
  F --> H[Summary input and diagnostics]
Loading

Before merge

  • Resolve merge risk (P1) - The open parent stack was reported conflicted; the verified test merge validates this PR against its pinned parent, not fetched main.
  • Complete next step (P2) - Resolve the parent stack at test: centralize stream and extension lifecycle fixtures #412 before landing on main, and refresh review if restacking changes this PR’s introduced diff.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Net line changes Production −209; tests +70; documentation +3 The ownership consolidation reduces production code while adding parser and cache regression coverage.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Use one core-owned source inventory and shared parsing helpers while preserving existing cache formats, unknown-value handling, and extraction fallbacks.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Use one core-owned source inventory and shared parsing helpers while preserving existing cache formats, unknown-value handling, and extraction fallbacks.

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

Yes, from source: writing an embedded or youtube-media transcript and reading it through main’s SQLite adapter loses its source value. No runtime reproduction was executed.

Is this the best way to solve the issue?

Yes. Sharing the existing source inventory fixes both readers without a storage migration, and the parsing changes remove equivalent implementations while retaining their behavior.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: Repairs a source-proven cache diagnostics defect with limited user impact alongside bounded internal cleanup.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This OWNER-authored refactor is exempt from contributor runtime proof; the reported parser and SQLite round-trip tests provide supplemental validation, and no material authority change triggers additional proof.

Label justifications:

  • P2: Repairs a source-proven cache diagnostics defect with limited user impact alongside bounded internal cleanup.
  • 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 refactor is exempt from contributor runtime proof; the reported parser and SQLite round-trip tests provide supplemental validation, and no material authority change triggers additional proof.

Evidence

What I checked:

  • Repository policy: Read the full root policy; no nested AGENTS.md or maintainer-notes directory was found. Applied the core-library versus CLI boundary and read-only review restrictions. (AGENTS.md:1, 3457a218f3b3)
  • Introduced patch identity: The original head records the pinned base as its parent. The verified test merge records that base followed by the exact head and has the same tree as the head; it does not establish a merge result against fetched main. (f7112080ad3b)
  • Defect remains on main: Main’s SQLite source whitelist omits embedded and youtube-media; its core cache mapper also maps youtube-media to unknown. (src/cache.ts:81, 6e20e4493ffa)
  • Latest release still affected: The v0.21.11 source has the same incomplete SQLite whitelist; the tag resolves to the supplied release commit. (src/cache.ts:81, 1dcbff752de7)
  • Cache compatibility: The patch only broadens recognition of existing source values. SQLite schema, cache keys, serialized writes, and unknown-value handling remain unchanged; existing stored source strings become readable without migration. (src/cache.ts:394, 3457a218f3b3)
  • Focused regression coverage: Two added cases exercise SQLite writes and core reads for embedded and youtube-media, asserting retained source and diagnostics. Existing malformed-payload and unknown-source assertions remain. Tests were inspected, not executed during this read-only review. (tests/cache.store.test.ts:266, 3457a218f3b3)

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.

@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

P2 Normal priority bug or improvement with limited blast radius. 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