Review: Port block parser from Python to Ruby (re-review)

review-1599-2026-06-24-r2 Review

review ready

Verdict: READY

Re-review of board item #1599 after scope refinement. Previous review: review-1599-2026-06-24 (NEEDS_REFINEMENT, 4 BODY fixes). All 4 BODY fixes have been addressed.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- Spike #75 (PR #76)
  • [x] Repo -- ldraney/paldocs
  • [x] User Story -- MCP cutover operator story
  • [x] Context -- explains gap clearly, now includes correct embedding path and reference code size
  • [x] File Targets -- 4 targets listed (including from_template)
  • [x] Feature Flag -- none (correct, internal infrastructure)
  • [x] Acceptance Criteria -- 7 criteria
  • [x] Test Expectations -- 4 expectations with correct RSpec run command
  • [x] Constraints -- 4 constraints listed (including explicit embedding path constraint)
  • [x] Checklist -- standard
  • [x] Related -- 3 references

Traceability

  • [x] story:consolidation label -- absorb pal-e-docs into Rails monolith
  • [x] story note verified -- story-paldocs-consolidation found in project-paldocs user-stories section
  • [x] arch:rails-api label -- Rails API component
  • [ ] arch note MISSING -- arch-rails-api note still does not exist. Search returns zero results. This is a cross-cutting gap affecting multiple board items (#1568, #1569, #1570, #1599, #1600, #1601, #1602). Not a blocker for this ticket individually -- the label is correct and the work is well-scoped without the note. [SCOPE] Create architecture note arch-rails-api for the Rails API component.
  • [x] Forgejo issue -- https://forgejo.tail5b443a.ts.net/ldraney/paldocs/issues/78, state: open

File Targets

  • [x] app/services/block_parser.rb (new) -- app/services/ directory exists (contains ollama_client.rb). Standard Rails convention. OK.
  • [x] app/services/block_compiler.rb (new) -- same directory. OK.
  • [x] app/controllers/api/notes_controller.rb -- verified at HEAD. Has create (line 85), update (line 167), and from_template (line 126). None currently calls any block parsing. Confirmed gap matches issue description.
  • [x] Reference: pal-e-docs blocks/ Python files -- verified in prior review (parser.py=316, compiler.py=148, sync.py=112). Repo not accessible via current API token but files were confirmed in review-1599-2026-06-24.

Previous BODY Fixes -- Resolution Check

  • [x] Fix 1: Add from_template -- RESOLVED. from_template now listed in File Targets, AC 5 added ("Api::NotesController#from_template stores Block rows for the created note"), integration test added ("note from_template via API -> blocks exist").
  • [x] Fix 2: Embedding path (NOTIFY vs after_save) -- RESOLVED. AC 6 now correctly reads "Block#after_save :enqueue_embedding callback fires for new/updated blocks (existing Rails mechanism via Solid Queue)". Context section also states this explicitly. Constraint 3 added: "Embedding path is Block#after_save -> Solid Queue EmbedBlockJob, NOT Postgres NOTIFY".
  • [x] Fix 3: parser.py line count -- RESOLVED. Context now correctly states "~576 lines across 3 Python files (parser.py=316, compiler.py=148, sync.py=112)".
  • [x] Fix 4: Test command -- RESOLVED. Run command is now "bundle exec rspec spec/services/ spec/requests/api/notes_spec.rb".

Repo Placement

OK -- issue filed on ldraney/paldocs, all file targets are in the paldocs repo. Reference files in pal-e-docs are read-only source for the port.

Dependencies

  • Upstream (done): Issue #57 (API namespace + Notes controller) -- merged. Provides the controller to modify. Controller verified at HEAD with create, update, from_template actions.
  • Upstream (done): Issue #52 (Embedding pipeline) -- merged. Block model has after_save :enqueue_embedding callback with needs_embedding? guard (fires on content/block_type change or new record). EmbedBlockJob enqueues via Solid Queue.
  • Upstream (done): Issue #49 (Schema ownership) -- blocks table owned by Rails.
  • Downstream: Issue #61 (Hybrid search) depends on blocks being created correctly.
  • No blockers: In-progress items (#56 Sprint planning UI, #22 Deploy paldocs, #75 MCP compatibility spike) are all independent of this ticket.

Acceptance Criteria

  • [x] AC 1: BlockParser.call(html_content) returns array of block hashes -- testable, clear.
  • [x] AC 2: BlockCompiler.call(blocks) reconstructs HTML from block array -- testable, clear.
  • [x] AC 3: Api::NotesController#create stores Block rows -- testable, clear.
  • [x] AC 4: Api::NotesController#update re-parses when html_content changes -- testable, clear. Controller already tracks content_changed boolean.
  • [x] AC 5: Api::NotesController#from_template stores Block rows -- testable, clear. NEW in this revision.
  • [x] AC 6: Block#after_save :enqueue_embedding callback fires -- testable, correct mechanism. Verified: Block model has this callback with needs_embedding? guard.
  • [x] AC 7: Embedding worker picks up blocks -- testable, will work automatically via existing callback.
All 7 AC are testable by an agent. The test expectations cover all paths (unit round-trip, block types, integration for create and from_template).

Blast Radius

  • compiled_pages table: Exists in the Rails DB schema (inherited from pal-e-docs migration) but has NO corresponding Rails model. The Python sync.py writes to compiled_pages, but the Rails app does not use it -- the toc and section endpoints work directly from Block rows. NOT a concern for this ticket. The compiled_pages table is a pal-e-docs artifact that can be deprecated in a future cleanup ticket.
  • Nokogiri availability: Constraint says "already in Gemfile" -- Nokogiri is NOT in the explicit Gemfile but IS available as a transitive dependency (v1.19.4 via actionview/loofah/rails-html-sanitizer in Gemfile.lock, 21 references). The agent can require 'nokogiri' without adding it to the Gemfile. Minor wording inaccuracy but does not affect implementation.
  • destroy action: The existing destroy action uses Block.where(note_id: note.id).delete_all which bypasses ActiveRecord callbacks. This is pre-existing and not affected by this ticket.
  • BeautifulSoup to Nokogiri: The port from bs4 to Nokogiri is a well-understood translation. Both libraries have equivalent HTML parsing APIs.

Decomposition Assessment

No decomposition needed.
  • File targets: 3 modified/created + references. All in 1 repo. Under threshold.
  • AC count: 7. Over the 5 threshold, but all are tightly coupled to a single concern (block parsing on note write). Splitting would create artificial coupling overhead.
  • Estimated agent time: ~8-10 minutes. Exceeds 5-minute rule, but the 8-point estimate accounts for this. The work is cohesive -- a single service extraction pattern.
  • Phasing guidance from prior review still applies: (1) port parser + compiler as services with unit tests, (2) wire into controller + integration tests.

Recommendation

  • [SCOPE] Create architecture note arch-rails-api for the Rails API component. Cross-cutting gap affecting 7+ board items. Not a blocker for this specific ticket.
No BODY or LABEL fixes needed. All previous NEEDS_REFINEMENT items have been resolved. Ticket is ready for implementation.