pal-e-docs

Review: API namespace + Notes controller -- 12 endpoints

review-1567-2026-06-23 Review

review approved
Project

Verdict: APPROVED

Note: Retroactive review -- PR #66 already merged (commit 311f4a4). The check-board-advance hook was fail-open so this review was skipped at gate time. Reviewing anyway for audit completeness.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- decomposed from paldocs#51, depends on #49
  • [x] Repo -- ldraney/paldocs
  • [x] User Story -- MCP layer needs Rails API endpoints
  • [x] Context -- thorough, explains consolidation motivation and tsvector need
  • [x] File Targets -- 6 files listed (create + modify), plus "do not touch" boundary
  • [x] Feature Flag -- "none" with valid justification (new namespace, no UI impact)
  • [x] Acceptance Criteria -- 6 criteria covering CRUD, search, links, revisions, shape parity
  • [x] Test Expectations -- 4 items with run command
  • [x] Constraints -- 4 constraints (no jbuilder, JSON shape match, tsvector, no migration)
  • [x] Checklist -- 9 items
  • [x] Related -- project, story, arch, upstream/sibling/downstream references

Traceability

  • [x] story:consolidation label -- present on board item
  • [ ] story note MISSING -- story-paldocs-consolidation note does not exist in pal-e-docs. However, the project page project-paldocs user-stories section lists story-paldocs-consolidation as a link. The backing note has not been created yet. [SCOPE] Create user story note story-paldocs-consolidation.
  • [x] arch:rails-api label -- present on board item
  • [ ] arch note MISSING -- search for arch-rails-api returned no results. The issue references it in the Related section but the note does not exist. [SCOPE] Create architecture note arch-rails-api for the Rails API namespace component.
  • [x] Forgejo issue -- ldraney/paldocs#57, state: closed (expected, PR merged)
  • [x] sprint:1 label -- present, consistent with sprint batch

File Targets

  • [x] app/controllers/api/base_controller.rb -- verified: created in PR #66, skip CSRF, JSON error handling
  • [x] app/controllers/api/notes_controller.rb -- verified: created, 12 public actions (index, search, show, create, from_template, update, destroy, links, update_links, revisions, toc, section)
  • [x] app/models/note_link.rb -- verified: created, belongs_to source/target
  • [x] app/models/note_revision.rb -- verified: created, belongs_to note
  • [x] app/models/note.rb -- verified: updated with tsvector search scope (keyword_search), has_many :note_links_as_source/target, has_many :note_revisions
  • [x] config/routes.rb -- verified: namespace :api with notes routes including all 12 endpoints
  • [x] db/ci_schema.sql -- bonus: updated for CI test support (not in original file targets but appropriate, adds note_links and note_revisions tables to CI schema)
  • [x] spec/requests/api/notes_spec.rb -- verified: 47 test cases covering all 12 endpoints plus CRUD lifecycle and unit search tests
  • [x] No migrations added -- constraint honored, tables exist from pg_dump
  • [x] No UI views/controllers touched -- constraint honored

Repo Placement

OK. Issue filed on ldraney/paldocs, code merged to ldraney/paldocs. Single-repo change as expected.

Dependencies

  • [x] Upstream: paldocs#49 (schema ownership) -- in done column, confirmed merged (commit 3576a91)
  • [x] Siblings: paldocs#58 (boards API, sprint:2 backlog), paldocs#59 (projects/repos/tags API, sprint:2 backlog) -- correctly sequenced after this ticket
  • [x] Downstream: paldocs#54 (MCP server, sprint:3 backlog) -- correctly waiting for all API endpoints
  • No blocking dependencies remain. Dependency chain is clean.

Acceptance Criteria

All 6 acceptance criteria are agent-verifiable:
  • [x] GET /api/notes/:slug returns correct shape -- testable via request spec
  • [x] POST /api/notes creates note -- testable via request spec
  • [x] GET /api/notes/search via tsvector -- testable via request spec
  • [x] GET /api/notes/:slug/links returns links -- testable via request spec
  • [x] GET /api/notes/:slug/revisions returns history -- testable via request spec
  • [x] JSON shape matches pal-e-docs -- partially testable (spec verifies key structure; full shape parity requires runtime comparison against live pal-e-docs, which is a downstream concern for the MCP migration ticket #54)

Blast Radius

Low blast radius. The /api/ namespace is entirely new and isolated from existing UI routes. The Note model changes (new associations, search scope) are additive and do not alter existing behavior. The ci_schema.sql change is test-only infrastructure. No downstream consumers are broken.

Decomposition Assessment

6 file targets in 1 repo, 6 acceptance criteria. Slightly above the 5-minute rule thresholds (6 AC vs 5 limit), but the work is cohesive: all 12 endpoints share one controller and one model layer. Decomposing Notes CRUD from Notes search/links/toc would create artificial coupling. The 5-point estimate was appropriate. No decomposition needed retroactively.

Recommendation

The ticket scope was solid and the implementation matches the spec. Two traceability gaps should be closed:
  • [SCOPE] Create user story note story-paldocs-consolidation in pal-e-docs -- the project page references it but the backing note does not exist.
  • [SCOPE] Create architecture note arch-rails-api in pal-e-docs -- the issue references it and the board label uses it, but no backing note exists.
These are documentation gaps, not code gaps. The implementation is complete and correct.