feat(onboarding): structural numbering profile at formatting ingress (#299)

[thewrz]

Why

The 5-signal DOCX inference engine assumes the CSI integer-PART model and guesses each document's numbering/hierarchy per file. For a genuinely non-standard source that posture is wrong: the operator should be able to declare the source's structural numbering scheme once at ingress and have it applied deterministically — the structural sibling of #125's visual style template. This adds a saveable, library-scoped structural numbering profile: its data model, the ingress snapshot→edit→assign→apply loop, a deterministic override of inference signals 1–2, and the REST + MCP surface.

Indentation (Signal 5) stays the engine's job (out of scope). Firm/client/project/package scope tiers, the onboarding UI, MCP write tools, auto-detection, and re-rendering deviant specs (#146) are all deliberately deferred.

What

A NumberingProfile is the serializable, operator-editable projection of the parser's internal NumberingMap classification fields (articleIlvl, pStyle ladders, specShapedNumIds) plus tier bounds. The parser stays pure — it gains an optional numberingProfile through ParseOptions; the API/ingress layer resolves spec.numbering_profile_id ?? built-in 'CSI Default' and injects it (the #304 "engine stays pure, caller resolves context" contract).

• ast/ — NumberingProfileSchema (Zod v4, open via .catchall), the foundational type consumed by db/parser/api.
• db/ — migration 038 (numbering_profiles table + specs.numbering_profile_id FK ON DELETE RESTRICT + a frozen, seeded built-in 'CSI Default' singleton), CRUD queries, and getEffectiveNumberingProfile resolution.
• parser/ — extractNumberingProfile (snapshot: map → profile) and applyNumberingProfile (override: profile → map), round-trip inverses; the optional profile threaded through parse → parseDocx → buildClassification. When a profile is present, paragraphs are classified twice (overridden vs. base) and per-paragraph disagreements are recorded in paragraphs.conflicts/meta.conflicts — losing inference persisted, never dropped.
• api/ — library CRUD, spec assign/clear, a snapshot endpoint, with openapi.yaml updated in lockstep (contract gate green).
• mcp/ — read-only get_numbering_profile.

The hard invariant: absent a profile (or with the empty built-in 'CSI Default'), the produced AST is byte-for-byte today's behavior. This holds two ways — the no-profile path is the unchanged code, and an empty profile is a reference-preserving passthrough (overrides a field only when the profile actually specifies it).

Design decisions (plan deviations and why)

• Migration number is 038, not the next-free 037. 037 is claimed by the open PR feat(db): keynote master table + project-filtered keynote query (ADR-016) #315 (feat/issue-98); 038 avoids a collision when that merges.
• The snapshot endpoint is POST /numbering-profiles/snapshot (multipart DOCX upload), not the planned GET /specs/:id/numbering-profile/snapshot. SpecR discards raw OOXML after ingest (ADR-021) — the AST is the source of truth, so there is no stored source DOCX to re-parse for an existing spec, and the lossy AST cannot reconstruct numId/lvlText/pStyle ladders faithfully. An upload endpoint is the faithful realization of the spec's ingress-snapshot intent ("emit a draft from the real source," flow step 1, before the spec is saved). A thin parser-barrel extractNumberingProfileFromDocx(buffer) keeps the API off docx internals.
• Built-in 'CSI Default' is protected from deletion. This table is the first to combine a built-in fallback with a public DELETE endpoint; an unassigned spec stores NULL (not an FK to the built-in), so the RESTRICT FK never fires for it. Without a guard, deleting the built-in would 500 every unassigned spec's resolution. Guarded at the DB layer (AND library_id IS NOT NULL) and the handler (clear 409), with a regression test.
• numbering[] is filtered to spec-shaped numIds in the snapshot, so specShapedNumIds reconstructs exactly on round-trip (generic list numIds don't get a spurious part tier).

Non-blocking follow-ups (future issues)

• Extract a shared src/mcp/tool-result.ts to retire per-handler ToolResult redefinitions.
• End-to-end test asserting a disagreeing profile surfaces in a parsed tree's meta.conflicts (today pinned at the mergeProfileConflicts unit boundary).
• The continuation/note node channel doesn't carry meta.conflicts (pre-existing); a profile that demotes a structural node to a continuation drops that conflict.
• DRY the DOCX upload MIME guard shared with parse.ts.

Testing

• Unit tests pass — 1252/1252 (vitest --project unit), incl. byte-for-byte INV1/INV2, override INV3, conflict INV4, and round-trip symmetry.
• Integration tests pass — 751 passed, 0 failed (vitest --project integration); 115 skipped are the copyrighted, gitignored ARCAT/CPI fixtures (absent locally; they run in CI) plus other fixture-gated suites.
• ARCAT + CPI no-profile golden inference tests are unchanged — the new optional parameter and empty-profile passthrough leave their parse path identical; they were not edited and stay green (they run in CI where the fixtures are present).
• Contract gate green (src/api/contract.integration.test.ts) — bidirectional route↔spec coverage for all 8 new routes; openapi.yaml updated in the same PR.
• Lint clean (pnpm lint: eslint + tsc --noEmit + prettier) and build clean (pnpm build).
• CI green

🤖 Co-authored by Claude Opus 4.8. Closes #299.

Summary by CodeRabbit

• New Features
  • Added structural numbering profiles (#299) with library-scoped CRUD, DOCX snapshot extraction, and per-spec assignment/clearing, including an immutable built-in “CSI Default”.
  • Updated POST /parse to accept optional numberingProfileId for deterministic DOCX parse-time overrides (returns 404 when missing) and to record resolved conflicts.
  • Added REST endpoints and an MCP tool to fetch a spec’s effective numbering profile.
• Documentation
  • Added design and rollout plan documentation for numbering profiles.
• Tests
  • Added integration/unit tests covering REST API behavior, DOCX extraction/apply invariants, and parse-worker override wiring.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds structural numbering profiles across docs, schema, database, parser, REST/OpenAPI, and MCP layers. The parser can now extract and apply profiles, the API exposes CRUD and assignment endpoints, and tests cover the new data model and parse-time override flow.

Changes

Numbering Profile Feature

Layer / File(s)	Summary
Design and implementation plan docs docs/superpowers/plans/..., docs/superpowers/specs/...	Adds the implementation plan and design spec describing the numbering profile architecture, data model, workflow, API surface, and test plan.
NumberingProfile schema and AST exports src/ast/numbering-profile-schema.ts, src/ast/numbering-profile-schema.test.ts, src/ast/schemas.ts, src/ast/schemas.test.ts, src/ast/index.ts	Defines NumberingProfileSchema/TierNameSchema, request-body schemas, and AST barrel exports, with schema validation tests.
Database migration and query layer src/db/migrations/038_create_numbering_profiles.ts, src/db/queries/numbering-profiles.ts, src/db/queries/numbering-profiles.integration.test.ts, src/db/index.ts	Creates the numbering_profiles table with seeded CSI Default and a specs.numbering_profile_id FK, plus CRUD and getEffectiveNumberingProfile query functions and integration tests.
Parser snapshot extraction src/parser/docx/numbering-profile.ts, src/parser/docx/numbering-profile.test.ts	Implements extractNumberingProfile building tier, numbering, and style ladder data from DOCX numbering/style maps.
Parser override apply and parse threading src/parser/docx/numbering-profile.ts, src/parser/docx/numbering-profile-apply.test.ts, src/parser/docx/numbering-profile-apply.integration.test.ts, src/parser/docx/index.ts, src/parser/index.ts, src/lib/parse-worker.ts, src/lib/parse-worker.test.ts	Implements applyNumberingProfile/mergeProfileConflicts, threads numberingProfile through parse options and worker input, and adds backward-compatibility tests.
REST API and OpenAPI src/api/numbering-profiles.ts, src/api/router.ts, src/api/numbering-profiles.integration.test.ts, src/api/contract.integration.test.ts, src/api/parse.ts, src/api/parse.integration.test.ts, openapi.yaml	Adds endpoints for profile CRUD, DOCX snapshot extraction, spec assignment/clearing, and parse-time profile resolution, with matching OpenAPI updates and integration coverage.
MCP read tool src/mcp/numbering-profile-handler.ts, src/mcp/handlers.ts, src/mcp/tools.ts, src/mcp/handlers.test.ts	Adds handleGetNumberingProfile and registers the get_numbering_profile MCP tool.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• wrzonance/SpecR#21: Both PRs extend the DOCX POST /parse pipeline by modifying the DOCX parser entrypoint and parse orchestration.
• wrzonance/SpecR#55: Both PRs modify the MCP tooling layer by adding new tool registration and handler wiring.
• wrzonance/SpecR#71: Both PRs touch the parse worker pipeline and option threading into the shared parser.

Suggested labels

style-fidelity:3

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR covers the structural numbering profile flow, but the linked warning requirement for non-conforming PART numbering is not shown here.	Add a ParseWarning path for decimal/dotted PART numbers and tiers above 5 when no custom profile is present, with regression tests.
Docstring Coverage	⚠️ Warning	Docstring coverage is 39.47% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is specific, concise, and matches the main numbering-profile ingress change.
Out of Scope Changes check	✅ Passed	The changes stay focused on numbering profiles, parser ingress, API/DB wiring, tests, and docs, with no clear unrelated additions.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/issue-299

---

_{Comment @coderabbitai help to get the list of available commands.}

[thewrz]

Independent adversarial review — Codex (GPT-5.5, codex exec review --base main)

Ran as a second reviewer alongside the prior Opus whole-branch review. 3 findings, all rated P2, no Critical/High. One was a genuine integrity bug and is fixed; two are declined with rationale below.

Fixed

• PATCH could mutate the built-in CSI Default (src/api/numbering-profiles.ts) — the symmetric partner of the deletable-built-in bug the earlier review caught. PATCH /numbering-profiles/:id had no guard, so a caller could list the default's id and rewrite its name/rules; getEffectiveNumberingProfile then serves that mutated row as the fallback for every unassigned spec. Fixed in 5c393d1: a shared rejectMissingOrBuiltIn guard (now used by both PATCH and DELETE, removing the inline duplication), a regression test (PATCH on the built-in → 409, rules untouched), and the openapi.yaml PATCH op documents the 409 + immutability.

Declined (with rationale)

• P2 — "Reparse base paragraphs before comparing conflicts" (src/parser/docx/index.ts). When a profile overrides styleLadder, the base-map conflict comparison runs over already-profile-resolved paragraphs, so a style-ladder-only disagreement may not surface in meta.conflicts. This is the same item the Opus whole-branch review already dispositioned as informational, conforms to the brief — the plan explicitly mandated the "parse-once, classify-twice on the same paragraph array" approach, and tier-boundary disagreements (the common case) do surface correctly. Declined here as a plan-level design choice; logged as a non-blocking follow-up (an end-to-end meta.conflicts test on a disagreeing profile would pin it).
• P2 — "Reject profiles from a different library on assignment" (src/api/numbering-profiles.ts). A library-A profile can be assigned to a library-B spec. This is a scoping policy the spec did not mandate (cross-library reuse may even be desirable), and SpecR has no tenancy boundary today that it would breach. Declined as out-of-scope for this PR; noted as a design question for the owner.

Status

Local checks green after the fix: pnpm lint clean, pnpm build clean, 1252/1252 unit, 52 integration (numbering-profiles + contract gate, incl. the new PATCH-built-in 409 test). PR remains a draft — owner reviews/merges.

[coderabbitai]

Actionable comments posted: 10

🧹 Nitpick comments (2)

docs/superpowers/plans/2026-06-30-issue-299-numbering-profile.md (1)

194-221: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

CASCADE/RESTRICT interaction could block library deletion indirectly.

numbering_profiles.library_id cascades on library delete, but specs.numbering_profile_id is RESTRICT. If a spec in a different library is assigned a profile owned by this library (cross-library assignment is allowed per the design), deleting the library will attempt to cascade-delete that profile, which will then hit the RESTRICT FK on the referencing spec and fail. Worth a comment/note in the design doc so the API layer can surface a clear error rather than a raw 23503 from an unexpected cascade path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/superpowers/plans/2026-06-30-issue-299-numbering-profile.md` around
lines 194 - 221, Add a note in the design doc around the numbering profile
migration to call out the CASCADE/RESTRICT interaction between
numbering_profiles.library_id and specs.numbering_profile_id. Explain that
deleting a library can fail if any spec, even in another library, references a
profile owned by that library, and reference the numbering_profiles and specs
relations so the API layer can surface a clear user-facing error instead of a
raw foreign-key failure.

src/mcp/handlers.test.ts (1)

44-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add a rejected-dependency path test.

This suite covers found/missing specs, but not the contract that MCP tools return { isError: true } when a dependency throws. Please add a case where getSpecTree() or getEffectiveNumberingProfile() rejects and assert handleGetNumberingProfile() still resolves to an error payload instead of throwing. As per coding guidelines, src/mcp/**/*.ts: MCP tools must never throw; on failure they must return { isError: true, content: [...] }.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mcp/handlers.test.ts` around lines 44 - 73, The handleGetNumberingProfile
test suite is missing coverage for the MCP failure contract when a dependency
rejects. Add a case in handleGetNumberingProfile that mocks either
db.getSpecTree or db.getEffectiveNumberingProfile to throw/reject, then assert
the handler still resolves to an { isError: true, content: [...] } payload
instead of propagating the exception. Use the existing
handleGetNumberingProfile, getSpecTree, and getEffectiveNumberingProfile symbols
to place the new test alongside the current known/unknown spec cases.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@openapi.yaml`:
- Around line 3671-3675: The endpoint contract is describing the wrong fallback
behavior for clearing a spec profile. Update the OpenAPI text for the
spec-profile clear operation (the one that clears specs.numbering_profile_id) so
it says the spec falls back to the built-in CSI Default rather than a library
default, and keep the description aligned with the actual behavior implemented
by the clear/reset flow. Check the operation summary/description in openapi.yaml
for this endpoint and make the wording consistent with the rest of the
numbering-profile API.

In `@src/api/numbering-profiles.ts`:
- Around line 197-199: The MIME equality check in the upload handling path is
too strict and rejects valid DOCX files sent as generic multipart parts. Update
the `req.file.mimetype` validation in `numbering profiles` so it does not
require an exact match to `DOCX_MIME`; instead rely on the existing
filename/extension gating and the `assertDocxSafe(buffer)` payload validation
already used in this flow. Keep the rest of the upload logic unchanged and
preserve the current error handling around the DOCX parsing path.

In `@src/db/queries/numbering-profiles.integration.test.ts`:
- Around line 18-23: The cleanup in the numbering profiles integration test is
too broad because the afterEach block in numbering-profiles.integration.test.ts
deletes every custom numbering_profiles row, not just this suite’s np-test-*
fixtures. Update the cleanup logic around afterEach to scope deletions to the
test data created here, preferably by relying on the DELETE FROM libraries WHERE
name LIKE 'np-test-%' cascade or by narrowing the numbering_profiles delete to
rows tied to those same libraries. Ensure the existing FK-safe order in
afterEach remains intact while avoiding cross-suite fixture removal.

In `@src/db/queries/numbering-profiles.ts`:
- Around line 129-136: Reject built-in numbering profile updates in the DB
helper: updateNumberingProfile currently updates any row by id, which can still
mutate the shared CSI Default fallback row when library_id IS NULL. Update the
SQL in updateNumberingProfile to exclude built-in rows the same way
deleteNumberingProfile does, and make sure any internal callers via
src/db/index.ts cannot modify profiles without a library_id.
- Around line 208-217: The spec lookup in numbering-profiles is treating a
missing spec the same as an unassigned spec by always falling back to
resolveBuiltInRules in get numbering profile flow. Update the query handling
around result.rows[0], rawRules, and parseRules so you first detect when no spec
row was returned for the given specId and surface a not-found error/404 path
before applying the CSI Default fallback; keep the built-in fallback only for
existing specs with null rules.

In `@src/mcp/numbering-profile-handler.ts`:
- Around line 26-28: The catch block in get_numbering_profile currently logs and
returns a generic MCP error while exposing only the raw exception; introduce a
module-owned typed error class for this handler that extends SpecrError, wrap
the caught failure with the original err as cause, and use that typed error for
logging/returning the toolErr payload. Update the numbering-profile handler path
so any added context is preserved through the error chain rather than swallowing
it or surfacing a plain Error.
- Around line 22-24: The existence check in the numbering profile handler is
split across getSpecTree and getEffectiveNumberingProfile, which causes an
unnecessary full spec load and can race with deletion. Update the handler in
numbering-profile-handler.ts to use a single lookup path that determines both
whether the spec exists and what profile to return, ideally by folding the “spec
missing” case into getEffectiveNumberingProfile or a cheap existence helper.
Make sure the existing logic in the handler and the getEffectiveNumberingProfile
helper remains the place where the missing-spec vs default-profile distinction
is handled.

In `@src/parser/docx/index.ts`:
- Around line 204-216: The `numberingProfile === undefined` flow in
`src/parser/docx/index.ts` is no longer preserving the original parse behavior
when `comments.xml` is present because `commentsById` is always populated and
passed into `parseDocument()`/`parseParagraphSources(...)`. Update
`classifyWithOptionalProfile` so the no-profile path continues to use the
pre-profile parsing logic from `parseDocument()` without comments affecting
source parsing, while the profile-enabled branch can still use `commentsById` as
needed. Use the `commentsById`, `classifyWithOptionalProfile`, and
`parseDocument`/`parseParagraphSources` symbols to keep the fix localized.

In `@src/parser/docx/numbering-profile-apply.integration.test.ts`:
- Around line 35-59: The backward-compat invariant tests in
numbering-profile-apply.integration.test.ts only cover one LibreOffice CSI
fixture, so the new inference override path is still missing ARCAT and CPI
coverage. Extend this suite with representative ARCAT and CPI DOCX fixtures and
assert the same no-profile vs explicit default-profile invariants through parse,
project, renderMarkdown, and sectionInference. Use the existing parse-based test
pattern and keep the checks centered on the numbering-profile behavior so
regressions in the ARCAT/CPI normalization split are caught.

In `@src/parser/docx/numbering-profile.ts`:
- Around line 48-65: `buildStyleLadder()` is serializing all `pStyleToNumId` and
`resolvedNumPr` entries, including non-spec-shaped list styles that should not
become structural overrides. Update `buildStyleLadder()` to filter entries
against the spec-shaped numbering set (the same `specShapedNumIds`/numbering
criteria used by `buildNumbering()`), so only valid structural styles are added
to `ladder`; keep `applyNumberingProfile()` unchanged except for consuming the
corrected ladder. Add a regression test covering a generic bullet/flat-list
`pStyle` mapping at `ilvl=0` to verify it is excluded and does not replay as a
`tier: 'part'` override.

---

Nitpick comments:
In `@docs/superpowers/plans/2026-06-30-issue-299-numbering-profile.md`:
- Around line 194-221: Add a note in the design doc around the numbering profile
migration to call out the CASCADE/RESTRICT interaction between
numbering_profiles.library_id and specs.numbering_profile_id. Explain that
deleting a library can fail if any spec, even in another library, references a
profile owned by that library, and reference the numbering_profiles and specs
relations so the API layer can surface a clear user-facing error instead of a
raw foreign-key failure.

In `@src/mcp/handlers.test.ts`:
- Around line 44-73: The handleGetNumberingProfile test suite is missing
coverage for the MCP failure contract when a dependency rejects. Add a case in
handleGetNumberingProfile that mocks either db.getSpecTree or
db.getEffectiveNumberingProfile to throw/reject, then assert the handler still
resolves to an { isError: true, content: [...] } payload instead of propagating
the exception. Use the existing handleGetNumberingProfile, getSpecTree, and
getEffectiveNumberingProfile symbols to place the new test alongside the current
known/unknown spec cases.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cc653075-4879-418c-adae-0e4fbfabd44f

📥 Commits

Reviewing files that changed from the base of the PR and between e655c77 and 5c393d1.

📒 Files selected for processing (25)

• docs/superpowers/plans/2026-06-30-issue-299-numbering-profile.md
• docs/superpowers/specs/2026-06-29-issue-299-numbering-profile-design.md
• openapi.yaml
• src/api/contract.integration.test.ts
• src/api/numbering-profiles.integration.test.ts
• src/api/numbering-profiles.ts
• src/api/router.ts
• src/ast/index.ts
• src/ast/numbering-profile-schema.test.ts
• src/ast/numbering-profile-schema.ts
• src/ast/schemas.ts
• src/db/index.ts
• src/db/migrations/038_create_numbering_profiles.ts
• src/db/queries/numbering-profiles.integration.test.ts
• src/db/queries/numbering-profiles.ts
• src/mcp/handlers.test.ts
• src/mcp/handlers.ts
• src/mcp/numbering-profile-handler.ts
• src/mcp/tools.ts
• src/parser/docx/index.ts
• src/parser/docx/numbering-profile-apply.integration.test.ts
• src/parser/docx/numbering-profile-apply.test.ts
• src/parser/docx/numbering-profile.test.ts
• src/parser/docx/numbering-profile.ts
• src/parser/index.ts

[thewrz]

Codex adversarial review — disposition (round 2)

Following maintainer review of the Codex pass, here is the final disposition of the 3 findings. 2 fixed, 1 declined.

Fixed

• [P1] Built-in CSI Default was mutable via PATCH — the twin of the deletable-built-in bug. Now guarded at both layers: the handler rejects a PATCH/DELETE on the built-in with 409 (rejectMissingOrBuiltIn, shared by both endpoints — 5c393d1), and updateNumberingProfile scopes its UPDATE … WHERE id = $1 AND library_id IS NOT NULL so a direct query caller can't mutate it either (d0ebe6b). Regression tests at both the API (PATCH → 409, rules unchanged) and DB (updateNumberingProfile returns null, seed untouched) layers. openapi documents the PATCH 409 + immutability.

• [P1] Assigned profile was never applied in production — until now applyNumberingProfile ran only in tests; POST /parse applied nothing. Wired the last mile (740ec99): POST /parse accepts an optional multipart numberingProfileId; the handler resolves it (404 if missing) before the job starts and threads the profile's rules through processParseJob → parsePool → parse-worker → parse() as the numberingProfile parse option. Absent the field the call shape is byte-for-byte unchanged (no default injected). WorkerInput carries the profile as a plain JSON object (structured-cloneable across the Piscina boundary). openapi documents the field + 404.

  • Known limitation (documented): application is at parse/upload time. Re-applying a profile to an already-parsed spec is out of scope — SpecR stores no source DOCX (ADR-021), so there is nothing to re-parse.
  • Tests: parse-worker unit asserts the profile is threaded into parse() options (and omitted when absent); parse integration asserts a missing profileId → 404 and the built-in default threads through as a passthrough (same node count as no profile). The override's tier/conflict behavior remains pinned at the parser unit level (INV3/INV4); a numbering-driven golden e2e needs the gitignored ARCAT fixtures (CI-only), since the only committed DOCX fixtures are text-driven (Signal 4) and immune to a numbering override.

Declined (with rationale)

• [P2] No library-scope check on profile assignment — setSpecNumberingProfile sets the FK without checking the profile belongs to the spec's library_id. Declined as a per-feature fix: setSpecStyleSource (feat(api): style-source assignment — manual template pick for non-DOCX masters #138) has the identical gap, so this is a cross-module consistency concern better handled by one hardening change across both subsystems. A cross-cutting issue will track it.

Status

Local checks green: pnpm lint clean, pnpm build clean, 1254/1254 unit, 756 integration (0 failed; 115 skipped = absent-fixture suites), contract gate green. PR remains a draft — owner reviews/merges.

[coderabbitai]

🧹 Nitpick comments (1)

src/api/parse.ts (1)

182-197: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift

Type assertion across the worker boundary masks a schema gap.

workerOutputSchema.parse(workerRaw) as WorkerOutput casts past the fact that tree.parts is only validated as z.array(z.unknown()), not as the actual SpecNode[] shape SpecTree/WorkerOutput require. The strict-TS guideline bans type assertions across module boundaries precisely because this kind of cast hides a structural mismatch that later flows unchecked into finalTree/persistParsedSpec.

This is a pre-existing schema-completeness gap relocated into the new runParseWorker helper, not a regression from this PR's numbering-profile change, so it's a recommended (not essential) follow-up — e.g., tightening workerOutputSchema's parts typing (or a documented, narrower assertion) would remove the need for the boundary-wide cast.

♻️ Illustrative direction (schema tightening, not a full fix)

-  tree: z.object({
-    id: z.string(),
-    section: z.union([SectionNumberSchema, z.literal('unknown')]),
-    title: z.string(),
-    parts: z.array(z.unknown()),
-    warnings: z.array(ParseWarningSchema).optional(),
-  }),
+  tree: z.object({
+    id: z.string(),
+    section: z.union([SectionNumberSchema, z.literal('unknown')]),
+    title: z.string(),
+    parts: z.array(SpecNodeSchema), // replace z.unknown() with a real shape check
+    warnings: z.array(ParseWarningSchema).optional(),
+  }),

As per coding guidelines, "TypeScript must stay strict... no type assertions across module boundaries."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/api/parse.ts` around lines 182 - 197, The new runParseWorker helper is
crossing the worker boundary with a type assertion that hides an incomplete
schema. Remove the boundary-wide cast in runParseWorker and make
workerOutputSchema validate the full WorkerOutput shape, especially tree.parts
as SpecNode[] rather than z.array(z.unknown()). Tighten the schema (or introduce
a narrower, documented conversion inside the worker contract) so parsePool.run
results are fully type-safe before they flow into finalTree and
persistParsedSpec.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/api/parse.ts`:
- Around line 182-197: The new runParseWorker helper is crossing the worker
boundary with a type assertion that hides an incomplete schema. Remove the
boundary-wide cast in runParseWorker and make workerOutputSchema validate the
full WorkerOutput shape, especially tree.parts as SpecNode[] rather than
z.array(z.unknown()). Tighten the schema (or introduce a narrower, documented
conversion inside the worker contract) so parsePool.run results are fully
type-safe before they flow into finalTree and persistParsedSpec.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d3d0be25-ec40-460f-ac87-9bdcbaeeb383

📥 Commits

Reviewing files that changed from the base of the PR and between 5c393d1 and 740ec99.

📒 Files selected for processing (7)

• openapi.yaml
• src/api/parse.integration.test.ts
• src/api/parse.ts
• src/db/queries/numbering-profiles.integration.test.ts
• src/db/queries/numbering-profiles.ts
• src/lib/parse-worker.test.ts
• src/lib/parse-worker.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• src/db/queries/numbering-profiles.integration.test.ts
• openapi.yaml
• src/db/queries/numbering-profiles.ts

[thewrz]

Wired the numbering profile into POST /parse — #299 override path now runs in production

POST /parse now applies an assigned numbering profile end to end (the override previously ran only in tests):

• Schema/handler (740ec99): ParseBody gains optional multipart numberingProfileId (z.uuid()); the handler resolves it (404 "numbering profile not found" before the job starts) and threads the profile's rules through processParseJob → parsePool → parse-worker → parse() as the numberingProfile option. Absent the field, the call shape is byte-for-byte unchanged (no default injected — backward-compat preserved). WorkerInput.numberingProfile carries it as plain JSON (structured-cloneable across the Piscina boundary). openapi.yaml documents the field + 404.
• e2e proof (ef29d0e): a real numbering-driven document is built in-test (no committed binary) — an abstractNum whose ilvl-0 lvlText declares PART makes its numId spec-shaped, so structure is Signal-1-driven and therefore sensitive to a numbering override (the only committed DOCX fixtures are text-driven and immune). With no profile the ilvl-2 paragraph persists as pr1; re-parsed through POST /parse with a profile that sets articleIlvl=2, it shifts to article and the losing base inference (pr1) is persisted in meta.conflicts. Plus 404-on-missing and built-in-default-passthrough (same node count as no profile).

Scope: the onboarding bulk-import path (src/api/onboarding.ts) stays on default no-profile behavior for now — this change is the POST /parse single-upload path.

Local checks green: pnpm lint clean, pnpm build clean, 1254/1254 unit, 757 integration (0 failed; 115 skipped = absent-fixture suites), contract gate green. PR remains a draft.

[coderabbitai]

🧹 Nitpick comments (2)

src/api/parse.integration.test.ts (2)

526-533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Created library/profile rows aren't cleaned up.

cleanupIds only tracks specId (line 520); the library created at line 526 and the numbering profile created at line 533 are never deleted. Unlike the specs cleanup at line 512 (which is idempotent across runs because it's keyed by a fixed section), these rows use a random suffix and will accumulate indefinitely in the test database across runs.

♻️ Proposed cleanup

   const profile = await createNumberingProfile(lib.id, 'Shift articleIlvl', overrideRules);
+  cleanupIds.push(profile.id); // if cleanup logic supports multiple id "kinds", or add dedicated teardown

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/api/parse.integration.test.ts` around lines 526 - 533, The cleanup in
this test only tracks the spec identifier, so the library created with
createLibrary and the numbering profile created with createNumberingProfile are
left behind and accumulate across runs. Update the test setup/teardown around
these calls to register both lib.id and profile.id in the cleanup list (or
otherwise delete them in the existing cleanup path), using the existing
createLibrary, createNumberingProfile, and cleanupIds flow so the test remains
idempotent.

---

508-550: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Tight timeout margin for the chained two-job e2e flow.

This test runs two sequential postBuffer + waitForJob cycles under a single 30s test timeout (line 550), but waitForJob's own internal deadline (line 56) defaults to 20s per call. If either parse job is slow under load (e.g., CI contention), the combined wall-clock time can approach or exceed the outer 30s timeout before either waitForJob call has a chance to throw its own descriptive error, producing a generic Vitest timeout failure instead of the more informative job ${jobId} did not complete within ${maxMs}ms error.

♻️ Proposed fix — widen the outer timeout margin

-  }, 30_000);
+  }, 50_000);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/api/parse.integration.test.ts` around lines 508 - 550, The chained
end-to-end test in parse.integration.test.ts is too close to the 30s outer
timeout when it runs two sequential postBuffer and waitForJob cycles. Update the
timeout on this specific it(...) case to leave enough margin above waitForJob’s
internal deadline so slow CI runs fail with the descriptive job timeout instead
of Vitest’s generic timeout. Keep the test logic and assertions in place; only
widen the test-level timeout for this scenario.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/api/parse.integration.test.ts`:
- Around line 526-533: The cleanup in this test only tracks the spec identifier,
so the library created with createLibrary and the numbering profile created with
createNumberingProfile are left behind and accumulate across runs. Update the
test setup/teardown around these calls to register both lib.id and profile.id in
the cleanup list (or otherwise delete them in the existing cleanup path), using
the existing createLibrary, createNumberingProfile, and cleanupIds flow so the
test remains idempotent.
- Around line 508-550: The chained end-to-end test in parse.integration.test.ts
is too close to the 30s outer timeout when it runs two sequential postBuffer and
waitForJob cycles. Update the timeout on this specific it(...) case to leave
enough margin above waitForJob’s internal deadline so slow CI runs fail with the
descriptive job timeout instead of Vitest’s generic timeout. Keep the test logic
and assertions in place; only widen the test-level timeout for this scenario.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: abd577fb-2a18-4644-8eef-a94badac3284

📥 Commits

Reviewing files that changed from the base of the PR and between 740ec99 and ef29d0e.

📒 Files selected for processing (1)

• src/api/parse.integration.test.ts

[thewrz]

CodeRabbit review — round handled (commit b2c1b2a)

All 9 threads resolved: 8 fixed, 1 declined with evidence.

Fixed:

• openapi clear-profile op → built-in CSI Default fallback (was 'library default')
• snapshot upload MIME relaxed (accepts octet-stream/empty; extension + assertDocxSafe are the real validation)
• integration teardown scoped to np-test-% fixtures (was wiping every custom profile in the shared DB)
• getEffectiveNumberingProfile returns null for a missing spec (no longer masks it as CSI Default)
• get_numbering_profile MCP tool → single lookup (no full-tree existence fetch / delete-race) + McpError wrapping with cause
• buildStyleLadder filters to spec-shaped numIds (generic flat-list styles no longer serialized as 'part')
• added ARCAT + CPI no-profile backward-compat cases (skip-if-absent, CI-only fixtures)

Declined (evidence in-thread): 'preserve no-profile path when comments.xml exists' — main already populated commentsById identically; the suggested gate would itself regress vs main by dropping comment parsing on the no-profile path. No behavior change; the no-profile path stays byte-for-byte unchanged.

Local: lint + build clean, 632 unit + 55 integration pass (2 ARCAT/CPI suites skip locally, run in CI), contract gate green. Still your merge — not merging.

[coderabbitai]

🧹 Nitpick comments (1)

src/parser/docx/numbering-profile-apply.integration.test.ts (1)

73-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add sectionInference equality to the new ARCAT/CPI suites for parity with the LibreOffice suite.

The LibreOffice backward-compat suite asserts sectionInference equality in addition to tree/markdown, but these new ARCAT and CPI suites only check project() and renderMarkdown(). Since sectionInference is part of ParseResult and should also be byte-for-byte unaffected by the no-op profile path, add the same check here for full parity and to catch any inference-metadata regression these two riskier fixture families might introduce.

♻️ Proposed addition

       expect(project(explicitNoProfile.tree)).toEqual(project(without.tree));
       expect(project(withDefault.tree)).toEqual(project(without.tree));
       expect(renderMarkdown(withDefault.tree)).toBe(renderMarkdown(without.tree));
+      expect(withDefault.sectionInference).toEqual(without.sectionInference);

Also applies to: 87-96

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/parser/docx/numbering-profile-apply.integration.test.ts` around lines 73
- 81, The ARCAT/CPI backward-compat tests in the numbering-profile suite are
missing the same ParseResult parity check used by the LibreOffice suite. Update
the relevant cases in the docx numbering-profile integration tests to also
assert equality on `sectionInference` for the `parse(...)` results, alongside
the existing `project(...)` and `renderMarkdown(...)` comparisons, so the no-op
profile path is validated across tree, markdown, and inference metadata.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/parser/docx/numbering-profile-apply.integration.test.ts`:
- Around line 73-81: The ARCAT/CPI backward-compat tests in the
numbering-profile suite are missing the same ParseResult parity check used by
the LibreOffice suite. Update the relevant cases in the docx numbering-profile
integration tests to also assert equality on `sectionInference` for the
`parse(...)` results, alongside the existing `project(...)` and
`renderMarkdown(...)` comparisons, so the no-op profile path is validated across
tree, markdown, and inference metadata.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6fe542f0-85e8-4491-a8b2-b0a7edf94cc9

📥 Commits

Reviewing files that changed from the base of the PR and between ef29d0e and b2c1b2a.

📒 Files selected for processing (8)

• openapi.yaml
• src/api/numbering-profiles.ts
• src/db/queries/numbering-profiles.integration.test.ts
• src/db/queries/numbering-profiles.ts
• src/mcp/handlers.test.ts
• src/mcp/numbering-profile-handler.ts
• src/parser/docx/numbering-profile-apply.integration.test.ts
• src/parser/docx/numbering-profile.ts

🚧 Files skipped from review as they are similar to previous changes (5)

• src/mcp/handlers.test.ts
• src/parser/docx/numbering-profile.ts
• src/api/numbering-profiles.ts
• src/db/queries/numbering-profiles.integration.test.ts
• openapi.yaml

[thewrz]

🤖 Codex (GPT-5.5, xhigh) adversarial review — additional eyes

CodeRabbit reviewed this PR normally; Codex ran as a second, independent adversarial pass and surfaced 4 correctness gaps in the numbering-profile feature. All were verified against the code and fixed in 1c9ae66 with regression tests:

Sev	Finding	Fix
P2	Base paragraphs weren't re-parsed → style-ladder profile-vs-inference conflicts were silently dropped from meta.conflicts (parseParagraph bakes style-inherited numId/ilvl from the map at parse time)	Parse a second time against the base map before the un-profiled comparison in classifyWithOptionalProfile; regression test in numbering-profile-apply.test.ts
P2	setSpecNumberingProfile allowed assigning a profile owned by another library — breaks library scoping and blocks the owning library's deletion via the RESTRICT FK	DB guard restricts assignment to the built-in CSI Default or the spec's own library; handler maps the mismatch → 409; openapi documents 409; integration regression test
P2	OpenAPI documented tiers as a generic object while Zod requires tiers.part.{numberStyle:'integer', maxCount 1–5} → a docs-following client got a surprise 422	openapi.yaml now mirrors the required part shape
P3	Whitespace-only profile name passed minLength(1) but tripped the DB trim() CHECK → 500 instead of 422	Zod .trim() before the length check; schema regression test

Verification: pnpm lint ✅ · unit 1258 ✅ · integration (numbering-profiles + OpenAPI contract gate) 55 ✅.

[thewrz]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[thewrz]

🤖 Codex (GPT-5.5, xhigh) re-review of the fix batch 1c9ae66 — a second adversarial pass over my own fixes surfaced 3 more findings. Resolved in 7cffe5b:

Sev	Finding	Resolution
P2	makeContinuationNode omitted meta.conflicts — a profile that demotes a paragraph to continuation while base inference disagreed lost that conflict at serialization	Fixed — continuation/note nodes now carry conflicts (mirrors makeNode); regression test added
P2	OpenAPI numbering[] / styleLadder[] items were arbitrary objects while Zod requires specific fields → a docs-generated client gets a surprise 422	Fixed — item schemas now mirror the Zod contract; contract gate green
P2	Profile tier field is written by the extractor (tierForIlvl) and never read on apply → editing tier alone is a silent no-op (design doc says "authoritative for numId→tier")	Deferred by decision → #319 — pinned by a KNOWN-AMBIGUITY test + design-doc note; ilvl+articleIlvl remain authoritative (internally consistent for extracted/round-tripped profiles)

Verification: pnpm lint ✅ · unit 1260 ✅ · integration (numbering-profiles + OpenAPI contract gate) 57 ✅.