diff --git a/openspec/changes/add-markdoc-structural-validation/design.md b/openspec/changes/add-markdoc-structural-validation/design.md new file mode 100644 index 00000000..e1ef82d8 --- /dev/null +++ b/openspec/changes/add-markdoc-structural-validation/design.md @@ -0,0 +1,46 @@ +# Design: Structural validation at the resolved-operation boundary + +## Decision + +Validators consume a resolved, read-only operation view: source paragraph, positional anchor, intended insertion level/style peer, and the ordered outline window around the anchor. This is late enough to know the actual DOCX structure and early enough to remain transactional. + +The shared result is a structured diagnostic rather than harness control flow: stable `code` and `severity` (`warning` or `error`), operation and source/anchor IDs, observed and intended hierarchy levels, human/agent-facing message, and an optional deterministic `suggested_anchor_id`. + +Markdoc compilation treats unsafe structural placement as fail-closed by default. Interactive editing tools return the same diagnostic as a warning or retryable error according to the tool's mutation contract. Retry counts and "warn once, then allow" policy remain application concerns and are not ported. + +## Parent-child slicing + +For a section-level insertion, scan forward from the positional anchor until a shallower ancestor boundary. If deeper descendants occur before that boundary and the inserted level would separate them from their parent, diagnose slicing and identify the last descendant as the corrective anchor. Insertion at or below the first child's level does not slice the hierarchy. + +Content-based section-header detection from the harness is not authoritative in Safe DOCX. Markdoc operation kind plus resolved numbering/style hierarchy must drive applicability; content heuristics may only provide advisory evidence. + +## Rollout + +1. Introduce the diagnostic/result contract and parent-child slicing validator. +2. Integrate it with Markdoc `validate` and compile preflight. +3. Expose matching diagnostics from insertion/edit tools. +4. Port level and list-renumbering rules one at a time with shared fixtures. + +## Bonded run-in paragraph pairs + +The NVCA form represents a run-in provision as two adjacent paragraphs with +different roles: a heading paragraph and a body follower. Repeated adjacent +style transitions in the source establish that pairing. Validation requires +both insertions, distinct structural peers, and an application order that +produces heading then body. Text casing and punctuation are not authoritative. +Pairing is one-to-one. `AFTER` operations name the body first because repeated +insertion reverses around the anchor; `BEFORE` operations name the heading +first. Multiple repeated followers for one heading style are resolved only when +the submitted body peer makes the choice unique, otherwise validation emits an +ambiguity error. + +`batch_edit` keeps same-slot collisions hard by default. The sole exception is +an explicit two-step `bonded_pair_id` group that the source-derived validator +recognizes as a correctly ordered heading/body pair with distinct style peers. +Three-step groups and unrelated inserts at that slot remain conflicts. + +Junior Harness's current live hooks do not enforce this exact two-paragraph +construction. Its same-paragraph regex is heuristic and its header consistency +hook checks sibling formatting. Safe DOCX therefore implements the structural +pair rule directly; harness retry state, Aspose bindings, legal-content +classification, and warn-once policy remain unported. diff --git a/openspec/changes/add-markdoc-structural-validation/proposal.md b/openspec/changes/add-markdoc-structural-validation/proposal.md new file mode 100644 index 00000000..53e42ec9 --- /dev/null +++ b/openspec/changes/add-markdoc-structural-validation/proposal.md @@ -0,0 +1,20 @@ +# Change: Add Markdoc structural validation and edit warnings + +## Why + +Safe DOCX preserves formatting reliably when the caller supplies the correct structural peer, but a syntactically valid insertion can still choose an anchor that slices a parent from its descendants or inherits the wrong hierarchy. The Junior harness already contains battle-tested deterministic rules for these mistakes, but those rules currently live above Safe DOCX and cannot protect Markdoc authors or other editing-tool callers. + +## What Changes + +- Add a product-neutral structural-validator contract to `docx-markdoc` with stable codes, severity/outcome, source location, evidence, and a suggested corrective anchor when one is deterministic. +- Port the semantics of the harness parent-child-slicing rule to the Safe DOCX document/operation model; do not couple Safe DOCX to the harness's Python hook registry, retry state, Aspose objects, or legal-content classifiers. +- Run structural validation after Markdoc schema validation and source resolution, before mutation or output writes. +- Surface the same diagnostics from `docx-markdoc validate`, compilation, and applicable editing-tool responses so agents receive actionable warnings. +- Start with parent-child slicing, then migrate level mismatch and mid-list renumbering rules behind the same registry when their semantics are proven against Safe DOCX fixtures. + +## Impact + +- Affected specs: `docx-markdoc`, `mcp-server` +- Affected code: `packages/docx-markdoc`, `packages/docx-mcp`, shared document outline/numbering inspection primitives +- Dependency: builds on `add-brownfield-markdoc-authoring` +- Compatibility: diagnostics are additive; strict compilation may newly reject structurally unsafe operations before writing output diff --git a/openspec/changes/add-markdoc-structural-validation/specs/docx-markdoc/spec.md b/openspec/changes/add-markdoc-structural-validation/specs/docx-markdoc/spec.md new file mode 100644 index 00000000..7757390d --- /dev/null +++ b/openspec/changes/add-markdoc-structural-validation/specs/docx-markdoc/spec.md @@ -0,0 +1,32 @@ +## ADDED Requirements + +### Requirement: Resolved Markdoc operations receive structural validation + +The system SHALL validate resolved Markdoc operations against the pinned source document's ordered hierarchy before mutating a document or writing output. Diagnostics SHALL have stable codes, severity, operation and anchor identity, structural evidence, and a corrective anchor when one is deterministic. + +#### Scenario: Parent-child slicing fails before mutation + +- **GIVEN** a section-level insertion anchored between a parent paragraph and its existing descendants +- **WHEN** the inserted level would separate those descendants from their parent +- **THEN** validation SHALL emit a parent-child-slicing diagnostic +- **AND** SHALL identify the last descendant before the ancestor boundary as the suggested anchor +- **AND** strict compilation SHALL write no output + +#### Scenario: Nested peer insertion is not misdiagnosed + +- **GIVEN** an insertion whose intended level is at or below the first following child's level +- **WHEN** structural validation runs +- **THEN** the parent-child-slicing validator SHALL pass + +#### Scenario: Validation output is actionable and stable + +- **GIVEN** a structurally unsafe resolved operation +- **WHEN** `docx-markdoc validate` or compilation preflight reports it +- **THEN** both surfaces SHALL use the same stable diagnostic code and evidence fields + +#### Scenario: Bonded run-in subsection requires two paragraphs + +- **GIVEN** the source repeatedly pairs a deterministic heading style with a distinct body-follower style +- **WHEN** an insertion supplies only the heading half or orders the two insertions incorrectly +- **THEN** strict validation SHALL fail before mutation +- **AND** SHALL identify both structural peer styles without relying on title-case text diff --git a/openspec/changes/add-markdoc-structural-validation/specs/mcp-server/spec.md b/openspec/changes/add-markdoc-structural-validation/specs/mcp-server/spec.md new file mode 100644 index 00000000..08e9bda5 --- /dev/null +++ b/openspec/changes/add-markdoc-structural-validation/specs/mcp-server/spec.md @@ -0,0 +1,19 @@ +## ADDED Requirements + +### Requirement: Editing tools surface structural placement warnings + +Applicable paragraph insertion and restructuring tools SHALL expose structural diagnostics derived from the same product-neutral validators used by Markdoc. + +#### Scenario: Unsafe insertion returns corrective guidance + +- **GIVEN** an insertion request that would slice a parent from existing descendants +- **WHEN** the tool resolves the requested anchor and intended hierarchy +- **THEN** the tool response SHALL identify the unsafe relationship +- **AND** SHALL include a deterministic suggested anchor when available + +#### Scenario: Atomic bonded pair shares one insertion slot + +- **GIVEN** exactly two batch insertion steps declare one bonded-pair identity, one anchor and position, and distinct source-proven heading/body peers +- **WHEN** the requested operation order yields heading followed by body +- **THEN** the batch SHALL permit the shared slot and apply both steps atomically +- **AND** an unrelated third insertion at that slot SHALL remain a hard conflict diff --git a/openspec/changes/add-markdoc-structural-validation/tasks.md b/openspec/changes/add-markdoc-structural-validation/tasks.md new file mode 100644 index 00000000..a1c0e21a --- /dev/null +++ b/openspec/changes/add-markdoc-structural-validation/tasks.md @@ -0,0 +1,28 @@ +## 1. Shared contract + +- [x] 1.1 Define stable structural diagnostic and validator interfaces +- [x] 1.2 Build a resolved, read-only outline/operation context +- [x] 1.3 Add registry aggregation and deterministic ordering tests + +## 2. Parent-child slicing + +- [x] 2.1 Port the position-based slicing algorithm without harness dependencies +- [x] 2.2 Cover parent, child, sibling, ancestor-boundary, and intentional nested insertion cases +- [x] 2.3 Return a deterministic last-descendant anchor suggestion + +## 3. Surfaces + +- [x] 3.1 Include structural diagnostics in Markdoc validation output +- [x] 3.2 Run strict compile preflight before any mutation or output write +- [x] 3.3 Surface equivalent warnings/retry guidance from applicable MCP editing tools + +## 4. Follow-on validators + +- [x] 4.1 Port list-level mismatch validation with Safe DOCX fixtures +- [x] 4.2 Port mid-list renumbering avoidance with Safe DOCX fixtures +- [x] 4.3 Document which harness hooks remain product-specific and intentionally unported + +## 5. Verification + +- [x] 5.1 Add NVCA SPA Section 2 insertion regression cases +- [x] 5.2 Run package tests and repository pre-submit checks diff --git a/packages/docx-core/src/primitives/index.ts b/packages/docx-core/src/primitives/index.ts index 09af917e..6c6ec071 100644 --- a/packages/docx-core/src/primitives/index.ts +++ b/packages/docx-core/src/primitives/index.ts @@ -1,5 +1,6 @@ export * from './document.js'; export * from './document_view.js'; +export * from './structural_validation.js'; export * from './errors.js'; export * from './list_labels.js'; export * from './layout.js'; diff --git a/packages/docx-core/src/primitives/structural_validation.test.ts b/packages/docx-core/src/primitives/structural_validation.test.ts new file mode 100644 index 00000000..62667afa --- /dev/null +++ b/packages/docx-core/src/primitives/structural_validation.test.ts @@ -0,0 +1,128 @@ +import { describe, expect } from 'vitest'; +import { testAllure as test } from '../testing/allure-test.js'; +import type { DocumentViewNode } from './document_view-types.js'; +import { validateStructuralInsertion, validateStructuralInsertions } from './structural_validation.js'; + +function node(id: string, level: number | null, numId = level == null ? null : '1'): DocumentViewNode { + return { + id, list_label: '', header: '', style: level == null ? 'body' : `Heading${level}`, + text: id, clean_text: id, tagged_text: id, + list_metadata: { list_level: level == null ? -1 : level - 1, label_type: null, label_string: '', header_text: null, header_style: null, header_formatting: null, is_auto_numbered: level != null }, + style_fingerprint: { list_level: level == null ? -1 : level - 1, left_indent_pt: 0, first_line_indent_pt: 0, style_name: '', alignment: 'LEFT' }, + paragraph_style_id: null, paragraph_style_name: '', paragraph_alignment: 'LEFT', + paragraph_indents_pt: { left: 0, first_line: 0 }, + numbering: { num_id: numId, ilvl: level == null ? null : level - 1, is_auto_numbered: level != null }, + heading: level == null ? undefined : { text: id, source: 'word_style', level }, + header_formatting: null, body_run_formatting: null, + }; +} + +describe('structural insertion validation', () => { + test('detects a parent/child slice and suggests the last descendant before the boundary', () => { + const nodes = [node('parent', 1), node('child', 2), node('grandchild', 3), node('body', null), node('sibling', 1)]; + expect(validateStructuralInsertion(nodes, { operationId: 'op', position: 'AFTER', anchorId: 'parent' })) + .toContainEqual(expect.objectContaining({ code: 'PARENT_CHILD_SLICE', suggested_anchor_id: 'body' })); + }); + + test('does not diagnose child-peer, sibling, or ancestor-boundary placement', () => { + const cases: Array<[DocumentViewNode[], string, string]> = [ + [[node('parent', 1), node('child', 2)], 'parent', 'child'], + [[node('first', 1), node('second', 1)], 'first', 'first'], + [[node('parent', 1), node('next', 1), node('child', 2)], 'parent', 'parent'], + ]; + for (const [nodes, anchorId, sourceId] of cases) { + expect(validateStructuralInsertion(nodes, { operationId: 'op', position: 'AFTER', anchorId, styleSourceId: sourceId }) + .filter((item) => item.code === 'PARENT_CHILD_SLICE')).toEqual([]); + } + }); + + test('reports an intentional nested level as advisory, not parent slicing', () => { + const diagnostics = validateStructuralInsertion([node('parent', 1), node('child', 2)], { + operationId: 'op', position: 'AFTER', anchorId: 'parent', styleSourceId: 'child', + }); + expect(diagnostics.map((item) => [item.code, item.severity])).toEqual([['LIST_LEVEL_MISMATCH', 'warning']]); + }); + + test('detects a foreign numbering definition inserted into the middle of a list', () => { + const diagnostics = validateStructuralInsertion([node('a', 1, '1'), node('b', 1, '1'), node('foreign', 1, '9')], { + operationId: 'op', position: 'AFTER', anchorId: 'a', styleSourceId: 'foreign', + }); + expect(diagnostics).toContainEqual(expect.objectContaining({ code: 'MID_LIST_RENUMBERING', severity: 'error' })); + }); + + test('aggregates in operation and registry order', () => { + const nodes = [node('p1', 1), node('c1', 2), node('p2', 1), node('c2', 2)]; + const diagnostics = validateStructuralInsertions(nodes, [ + { operationId: 'z', position: 'AFTER', anchorId: 'p2' }, + { operationId: 'a', position: 'AFTER', anchorId: 'p1' }, + ]); + expect(diagnostics.map((item) => item.operation_id)).toEqual(['z', 'a']); + }); + + test('requires both halves of a repeated run-in style pair without inspecting title text', () => { + const nodes = [ + node('h1', 2), { ...node('b1', null), style: 'HeadingPara2' }, + node('h2', 2), { ...node('b2', null), style: 'HeadingPara2' }, + node('anchor', 1), + ]; + expect(validateStructuralInsertions(nodes, [{ + operationId: 'heading', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'h1', + }])).toContainEqual(expect.objectContaining({ code: 'BONDED_PARAGRAPH_PAIR_REQUIRED' })); + }); + + test('accepts a complete heading/body pair in the insertion order needed for AFTER', () => { + const nodes = [ + node('h1', 2), { ...node('b1', null), style: 'HeadingPara2' }, + node('h2', 2), { ...node('b2', null), style: 'HeadingPara2' }, + node('anchor', 1), + ]; + const diagnostics = validateStructuralInsertions(nodes, [ + { operationId: 'body', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'b1' }, + { operationId: 'heading', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'h1' }, + ]); + expect(diagnostics.filter((item) => item.code === 'BONDED_PARAGRAPH_PAIR_REQUIRED' || item.code === 'RUN_IN_PAIR_ORDER')).toEqual([]); + }); + + test('rejects pair order that would put the body before its heading', () => { + const nodes = [ + node('h1', 2), { ...node('b1', null), style: 'HeadingPara2' }, + node('h2', 2), { ...node('b2', null), style: 'HeadingPara2' }, + node('anchor', 1), + ]; + expect(validateStructuralInsertions(nodes, [ + { operationId: 'heading', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'h1' }, + { operationId: 'body', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'b1' }, + ])).toContainEqual(expect.objectContaining({ code: 'RUN_IN_PAIR_ORDER' })); + }); + + test('uses each body operation for only one heading operation', () => { + const nodes = [node('h1', 2), { ...node('b1', null), style: 'HeadingPara2' }, node('h2', 2), { ...node('b2', null), style: 'HeadingPara2' }, node('anchor', 1)]; + const diagnostics = validateStructuralInsertions(nodes, [ + { operationId: 'body', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'b1' }, + { operationId: 'heading-one', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'h1' }, + { operationId: 'heading-two', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'h2' }, + ]); + expect(diagnostics).toContainEqual(expect.objectContaining({ code: 'BONDED_PARAGRAPH_PAIR_REQUIRED', operation_id: 'heading-two' })); + }); + + test('enforces the opposite source order for repeated BEFORE insertion', () => { + const nodes = [node('h1', 2), { ...node('b1', null), style: 'HeadingPara2' }, node('h2', 2), { ...node('b2', null), style: 'HeadingPara2' }, node('anchor', 1)]; + expect(validateStructuralInsertions(nodes, [ + { operationId: 'heading', position: 'BEFORE', anchorId: 'anchor', styleSourceId: 'h1' }, + { operationId: 'body', position: 'BEFORE', anchorId: 'anchor', styleSourceId: 'b1' }, + ]).some((item) => item.code === 'RUN_IN_PAIR_ORDER')).toBe(false); + expect(validateStructuralInsertions(nodes, [ + { operationId: 'body', position: 'BEFORE', anchorId: 'anchor', styleSourceId: 'b1' }, + { operationId: 'heading', position: 'BEFORE', anchorId: 'anchor', styleSourceId: 'h1' }, + ])).toContainEqual(expect.objectContaining({ code: 'RUN_IN_PAIR_ORDER' })); + }); + + test('fails explicitly when one heading style has ambiguous repeated followers', () => { + const nodes = [ + node('h1', 2), { ...node('a1', null), style: 'BodyA' }, node('h2', 2), { ...node('a2', null), style: 'BodyA' }, + node('h3', 2), { ...node('b1', null), style: 'BodyB' }, node('h4', 2), { ...node('b2', null), style: 'BodyB' }, node('anchor', 1), + ]; + expect(validateStructuralInsertions(nodes, [{ operationId: 'heading', position: 'AFTER', anchorId: 'anchor', styleSourceId: 'h1' }])) + .toContainEqual(expect.objectContaining({ code: 'BONDED_PARAGRAPH_PAIR_AMBIGUOUS' })); + }); +}); diff --git a/packages/docx-core/src/primitives/structural_validation.ts b/packages/docx-core/src/primitives/structural_validation.ts new file mode 100644 index 00000000..9c4f7cf4 --- /dev/null +++ b/packages/docx-core/src/primitives/structural_validation.ts @@ -0,0 +1,283 @@ +import type { DocumentViewNode } from './document_view-types.js'; + +export type StructuralDiagnosticSeverity = 'warning' | 'error'; + +export type StructuralDiagnosticEvidence = { + anchor_level: number | null; + intended_level: number | null; + first_descendant_id?: string; + first_descendant_level?: number; + style_source_id?: string; + anchor_num_id?: string; + style_source_num_id?: string; + bonded_heading_style?: string; + bonded_body_style?: string; + bonded_body_style_candidates?: string[]; +}; + +export type StructuralDiagnostic = { + code: 'PARENT_CHILD_SLICE' | 'LIST_LEVEL_MISMATCH' | 'MID_LIST_RENUMBERING' + | 'BONDED_PARAGRAPH_PAIR_REQUIRED' | 'RUN_IN_PAIR_ORDER' + | 'BONDED_PARAGRAPH_PAIR_AMBIGUOUS'; + severity: StructuralDiagnosticSeverity; + operation_id: string; + anchor_id: string; + message: string; + evidence: StructuralDiagnosticEvidence; + suggested_anchor_id?: string; +}; + +export type ResolvedInsertionContext = { + operationId: string; + position: 'BEFORE' | 'AFTER'; + anchorId: string; + styleSourceId?: string; +}; + +export type StructuralValidator = ( + nodes: readonly DocumentViewNode[], + context: ResolvedInsertionContext, +) => StructuralDiagnostic[]; + +function hierarchyLevel(node: DocumentViewNode | undefined): number | null { + if (!node) return null; + if (node.heading?.level != null && ( + node.heading.source === 'word_style' + || node.heading.source === 'list_metadata' + || node.heading.source === 'outline_level' + )) return node.heading.level; + if (node.numbering.is_auto_numbered && node.numbering.ilvl != null) return node.numbering.ilvl + 1; + return null; +} + +function structuralStyle(node: DocumentViewNode | undefined): string { + return node?.paragraph_style_id ?? node?.style ?? ''; +} + +const parentChildSlicing: StructuralValidator = (nodes, context) => { + if (context.position !== 'AFTER') return []; + const anchorIndex = nodes.findIndex((node) => node.id === context.anchorId); + const source = nodes.find((node) => node.id === (context.styleSourceId ?? context.anchorId)); + if (anchorIndex < 0 || !source) return []; + const anchorLevel = hierarchyLevel(nodes[anchorIndex]); + const intendedLevel = hierarchyLevel(source); + if (anchorLevel == null || intendedLevel == null || intendedLevel > anchorLevel) return []; + + const descendants: Array<{ id: string; level: number }> = []; + let lastDescendantId: string | undefined; + for (let index = anchorIndex + 1; index < nodes.length; index += 1) { + const level = hierarchyLevel(nodes[index]); + if (level == null) { + if (descendants.length > 0) lastDescendantId = nodes[index]!.id; + continue; + } + if (level <= anchorLevel) break; + descendants.push({ id: nodes[index]!.id, level }); + lastDescendantId = nodes[index]!.id; + } + if (descendants.length === 0) return []; + const first = descendants[0]!; + const suggestedAnchorId = lastDescendantId!; + return [{ + code: 'PARENT_CHILD_SLICE', + severity: 'error', + operation_id: context.operationId, + anchor_id: context.anchorId, + message: `Insertion ${context.operationId} would separate ${context.anchorId} from its existing descendants; insert after ${suggestedAnchorId} instead.`, + evidence: { + anchor_level: anchorLevel, + intended_level: intendedLevel, + first_descendant_id: first.id, + first_descendant_level: first.level, + style_source_id: context.styleSourceId, + }, + suggested_anchor_id: suggestedAnchorId, + }]; +}; + +const listLevelMismatch: StructuralValidator = (nodes, context) => { + const anchor = nodes.find((node) => node.id === context.anchorId); + const source = nodes.find((node) => node.id === (context.styleSourceId ?? context.anchorId)); + if (!anchor?.numbering.is_auto_numbered || !source?.numbering.is_auto_numbered) return []; + if (anchor.numbering.ilvl == null || source.numbering.ilvl == null || anchor.numbering.ilvl === source.numbering.ilvl) return []; + return [{ + code: 'LIST_LEVEL_MISMATCH', + severity: 'warning', + operation_id: context.operationId, + anchor_id: context.anchorId, + message: `Insertion ${context.operationId} uses list level ${source.numbering.ilvl} beside level ${anchor.numbering.ilvl}; confirm that nesting is intentional.`, + evidence: { + anchor_level: anchor.numbering.ilvl + 1, + intended_level: source.numbering.ilvl + 1, + style_source_id: context.styleSourceId, + anchor_num_id: anchor.numbering.num_id ?? undefined, + style_source_num_id: source.numbering.num_id ?? undefined, + }, + }]; +}; + +const midListRenumbering: StructuralValidator = (nodes, context) => { + const anchorIndex = nodes.findIndex((node) => node.id === context.anchorId); + const source = nodes.find((node) => node.id === (context.styleSourceId ?? context.anchorId)); + if (anchorIndex < 0 || !source?.numbering.is_auto_numbered) return []; + const anchor = nodes[anchorIndex]!; + const neighbor = context.position === 'AFTER' ? nodes[anchorIndex + 1] : nodes[anchorIndex - 1]; + if (!anchor.numbering.is_auto_numbered || !neighbor?.numbering.is_auto_numbered) return []; + const sameListWindow = anchor.numbering.num_id != null + && anchor.numbering.num_id === neighbor.numbering.num_id + && anchor.numbering.ilvl === neighbor.numbering.ilvl; + if (!sameListWindow || source.numbering.num_id == null || source.numbering.num_id === anchor.numbering.num_id) return []; + return [{ + code: 'MID_LIST_RENUMBERING', + severity: 'error', + operation_id: context.operationId, + anchor_id: context.anchorId, + message: `Insertion ${context.operationId} would introduce numbering ${source.numbering.num_id} inside list ${anchor.numbering.num_id}; use a peer from the surrounding list.`, + evidence: { + anchor_level: hierarchyLevel(anchor), + intended_level: hierarchyLevel(source), + style_source_id: context.styleSourceId, + anchor_num_id: anchor.numbering.num_id ?? undefined, + style_source_num_id: source.numbering.num_id ?? undefined, + }, + suggested_anchor_id: anchor.id, + }]; +}; + +export const structuralValidators: readonly StructuralValidator[] = [ + parentChildSlicing, + listLevelMismatch, + midListRenumbering, +]; + +export function validateStructuralInsertion( + nodes: readonly DocumentViewNode[], + context: ResolvedInsertionContext, +): StructuralDiagnostic[] { + return structuralValidators.flatMap((validator) => validator(nodes, context)); +} + +export function validateStructuralInsertions( + nodes: readonly DocumentViewNode[], + contexts: readonly ResolvedInsertionContext[], +): StructuralDiagnostic[] { + const diagnostics = contexts.flatMap((context) => validateStructuralInsertion(nodes, context)); + + // A repeated deterministic-heading → follower-style transition is document + // evidence that the two paragraphs form one run-in structural unit. This is + // intentionally style/position based: title casing and punctuation are not + // reliable structural authorities. + const transitions = new Map(); + for (let index = 0; index < nodes.length - 1; index += 1) { + const heading = nodes[index]!; + const body = nodes[index + 1]!; + if (hierarchyLevel(heading) == null || hierarchyLevel(body) != null) continue; + if (Math.abs(heading.paragraph_indents_pt.left - body.paragraph_indents_pt.left) > 0.5) continue; + const headingStyle = structuralStyle(heading); + const bodyStyle = structuralStyle(body); + if (!headingStyle || !bodyStyle || headingStyle === bodyStyle) continue; + const key = `${headingStyle}\u0000${bodyStyle}`; + const current = transitions.get(key); + transitions.set(key, { headingStyle, bodyStyle, count: (current?.count ?? 0) + 1 }); + } + const bonded = [...transitions.values()].filter((transition) => transition.count >= 2); + const consumedBodyOperations = new Set(); + contexts.forEach((context, headingOperationIndex) => { + const source = nodes.find((node) => node.id === (context.styleSourceId ?? context.anchorId)); + const candidatePairs = bonded.filter((transition) => transition.headingStyle === structuralStyle(source)); + if (candidatePairs.length === 0) return; + const availableBodies = contexts.map((candidate, index) => ({ candidate, index })).filter(({ candidate, index }) => { + if (consumedBodyOperations.has(index)) return false; + return candidate.anchorId === context.anchorId && candidate.position === context.position; + }); + const suppliedBodyStyles = new Set(availableBodies.map(({ candidate }) => { + const candidateSource = nodes.find((node) => node.id === (candidate.styleSourceId ?? candidate.anchorId)); + return structuralStyle(candidateSource); + })); + const matchingPairs = candidatePairs.filter((pair) => suppliedBodyStyles.has(pair.bodyStyle)); + if (candidatePairs.length > 1 && matchingPairs.length !== 1) { + diagnostics.push({ + code: 'BONDED_PARAGRAPH_PAIR_AMBIGUOUS', severity: 'error', operation_id: context.operationId, + anchor_id: context.anchorId, + message: `Style ${structuralStyle(source)} has multiple repeated body followers (${candidatePairs.map((pair) => pair.bodyStyle).sort().join(', ')}); supply exactly one matching body peer in this insertion slot.`, + evidence: { + anchor_level: hierarchyLevel(nodes.find((node) => node.id === context.anchorId)), + intended_level: hierarchyLevel(source), + style_source_id: context.styleSourceId, + bonded_heading_style: structuralStyle(source), + bonded_body_style_candidates: candidatePairs.map((pair) => pair.bodyStyle).sort(), + }, + }); + return; + } + const pair = matchingPairs[0] ?? candidatePairs[0]!; + const bodyOperation = availableBodies.find(({ candidate }) => { + const candidateSource = nodes.find((node) => node.id === (candidate.styleSourceId ?? candidate.anchorId)); + return structuralStyle(candidateSource) === pair.bodyStyle; + }); + const bodyOperationIndex = bodyOperation?.index ?? -1; + const evidence = { + anchor_level: hierarchyLevel(nodes.find((node) => node.id === context.anchorId)), + intended_level: hierarchyLevel(source), + style_source_id: context.styleSourceId, + bonded_heading_style: pair.headingStyle, + bonded_body_style: pair.bodyStyle, + }; + if (bodyOperationIndex < 0) { + diagnostics.push({ + code: 'BONDED_PARAGRAPH_PAIR_REQUIRED', severity: 'error', operation_id: context.operationId, + anchor_id: context.anchorId, + message: `Style ${pair.headingStyle} is repeatedly followed by ${pair.bodyStyle}; insert both paragraphs with distinct structural peers.`, + evidence, + }); + } else { + consumedBodyOperations.add(bodyOperationIndex); + } + const wrongOrder = bodyOperationIndex >= 0 && ( + (context.position === 'AFTER' && bodyOperationIndex > headingOperationIndex) + || (context.position === 'BEFORE' && headingOperationIndex > bodyOperationIndex) + ); + if (wrongOrder) { + const requiredOrder = context.position === 'AFTER' + ? `${pair.bodyStyle} before ${pair.headingStyle}` + : `${pair.headingStyle} before ${pair.bodyStyle}`; + diagnostics.push({ + code: 'RUN_IN_PAIR_ORDER', severity: 'error', operation_id: context.operationId, + anchor_id: context.anchorId, + message: `For repeated ${context.position} insertion, order operations ${requiredOrder} so the document yields heading then body.`, + evidence, + }); + } + }); + return diagnostics; +} + +/** True only for the explicit two-operation form of a source-proven bonded pair. */ +export function isRecognizedBondedInsertionPair( + nodes: readonly DocumentViewNode[], + contexts: readonly ResolvedInsertionContext[], +): boolean { + if (contexts.length !== 2) return false; + const [first, second] = contexts; + if (!first || !second || first.anchorId !== second.anchorId || first.position !== second.position) return false; + const sources = contexts.map((context) => nodes.find((node) => node.id === (context.styleSourceId ?? context.anchorId))); + if (!sources[0] || !sources[1] || structuralStyle(sources[0]) === structuralStyle(sources[1])) return false; + const headingIndex = sources.findIndex((source) => hierarchyLevel(source) != null); + const bodyIndex = sources.findIndex((source) => hierarchyLevel(source) == null); + if (headingIndex < 0 || bodyIndex < 0) return false; + const headingStyle = structuralStyle(sources[headingIndex]); + const bodyStyle = structuralStyle(sources[bodyIndex]); + let transitionCount = 0; + for (let index = 0; index < nodes.length - 1; index += 1) { + const heading = nodes[index]!; + const body = nodes[index + 1]!; + if (structuralStyle(heading) === headingStyle && structuralStyle(body) === bodyStyle + && hierarchyLevel(heading) != null && hierarchyLevel(body) == null + && Math.abs(heading.paragraph_indents_pt.left - body.paragraph_indents_pt.left) <= 0.5) transitionCount += 1; + } + if (transitionCount < 2) return false; + return !validateStructuralInsertions(nodes, contexts).some((diagnostic) => + diagnostic.code === 'BONDED_PARAGRAPH_PAIR_REQUIRED' + || diagnostic.code === 'BONDED_PARAGRAPH_PAIR_AMBIGUOUS' + || diagnostic.code === 'RUN_IN_PAIR_ORDER'); +} diff --git a/packages/docx-markdoc/README.md b/packages/docx-markdoc/README.md index 0787b48c..9762485e 100644 --- a/packages/docx-markdoc/README.md +++ b/packages/docx-markdoc/README.md @@ -121,6 +121,26 @@ while `start`, `end`, `paragraphPropertySha256`, `runPropertySha256`, and the readable view tied to the source formatting without copying raw OOXML into canonical Markdoc. Inspection output is diagnostic and cannot be compiled. +## Structural diagnostics + +When an anchored source is available, `validateMarkdocAgainstSource` and +compile preflight run the same deterministic validators used by DOCX insertion +tools. Diagnostics have stable codes, severity, operation/anchor identity, +structural evidence, and a corrective anchor when one is unambiguous. + +The registry detects parent/child slicing, list-level mismatch, foreign +numbering inserted into a continuous list, and incomplete bonded paragraph +pairs. A repeated deterministic-heading-to-body-style transition (for example, +`Heading2` followed by `HeadingPara2`) is treated as a two-paragraph structural +unit: both halves need distinct style sources, one body operation cannot satisfy +multiple headings, and operation order is checked separately for `BEFORE` and +`AFTER`. Ambiguous repeated follower styles fail with an explicit diagnostic. +No title-case or legal-content regex is used as structural authority. + +Junior Harness retry state, warn-once policy, Aspose adapters, legal section +classifiers, and content-specific remediation remain application concerns and +are intentionally not ported. + Leading or trailing spaces in operative text must be written as ` ` because Markdown treats ordinary boundary spaces as syntax. The importer does this automatically, including escaping literal `&` first, so import and replay remain diff --git a/packages/docx-markdoc/src/cli.ts b/packages/docx-markdoc/src/cli.ts index 7e095ddf..f412bf42 100644 --- a/packages/docx-markdoc/src/cli.ts +++ b/packages/docx-markdoc/src/cli.ts @@ -1,7 +1,7 @@ #!/usr/bin/env node import { mkdir, readFile, writeFile } from 'node:fs/promises'; import path from 'node:path'; -import { compileMarkdoc } from './compile.js'; +import { compileMarkdoc, validateMarkdocAgainstSource } from './compile.js'; import { exportEditPairs } from './export.js'; import { importDocxToMarkdoc } from './import.js'; import { inspectMarkdocSource } from './inspect.js'; @@ -12,7 +12,7 @@ function usage(): never { throw new Error([ 'Usage:', ' docx-markdoc import ', - ' docx-markdoc validate ', + ' docx-markdoc validate [anchored.docx]', ' docx-markdoc inspect [paragraph-id ...]', ' docx-markdoc compile ', ' docx-markdoc verify ', @@ -31,10 +31,13 @@ async function main(): Promise { return; } if (command === 'validate') { - const [markdocPath] = args; + const [markdocPath, sourcePath] = args; if (!markdocPath) usage(); - const ir = requireMarkdoc(await readFile(markdocPath, 'utf8')); - process.stdout.write(`${JSON.stringify(ir, null, 2)}\n`); + const markdoc = await readFile(markdocPath, 'utf8'); + const result = sourcePath + ? await validateMarkdocAgainstSource(await readFile(sourcePath), markdoc) + : requireMarkdoc(markdoc); + process.stdout.write(`${JSON.stringify(result, null, 2)}\n`); return; } if (command === 'inspect') { diff --git a/packages/docx-markdoc/src/compile.ts b/packages/docx-markdoc/src/compile.ts index acb01694..61b5decf 100644 --- a/packages/docx-markdoc/src/compile.ts +++ b/packages/docx-markdoc/src/compile.ts @@ -3,6 +3,8 @@ import { DocxDocument, computeContentFingerprint, getParagraphRuns, + validateStructuralInsertions, + type StructuralDiagnostic, type ReplacementPart, } from '@usejunior/docx-core'; import { compareDocuments } from '@usejunior/docx-compare'; @@ -217,7 +219,7 @@ async function unchangedPartsEqual(source: Buffer, clean: Buffer): Promise ({ + operationId: operation.operationId, + position: operation.kind === 'insert-before' ? 'BEFORE' : 'AFTER', + anchorId: operation.anchorId, + styleSourceId: operation.styleSourceId, + }))); + return { unsupported: [...unsupported].sort(), structuralDiagnostics }; +} + +export async function validateMarkdocAgainstSource( + sourceBuffer: Buffer, + markdoc: string, +): Promise<{ ir: MarkdocEditIR; diagnostics: StructuralDiagnostic[] }> { + const ir = requireMarkdoc(markdoc); + if (sha256(sourceBuffer) !== ir.source.sha256) { + throw new DocxMarkdocError('SOURCE_HASH_DRIFT', 'Source DOCX hash does not match canonical Markdoc.'); + } + const sourceDocument = await DocxDocument.load(sourceBuffer); + const { structuralDiagnostics } = validateAgainstSource(ir, sourceDocument); + return { ir, diagnostics: structuralDiagnostics }; } async function applyOperations(sourceBuffer: Buffer, ir: MarkdocEditIR): Promise { @@ -345,7 +366,15 @@ export async function compileMarkdoc( throw new DocxMarkdocError('EXISTING_REVISIONS_UNSUPPORTED', 'V1 cannot compile a source DOCX that already contains tracked changes.'); } const sourceDocument = await DocxDocument.load(sourceBuffer); - const { unsupported } = validateAgainstSource(ir, sourceDocument); + const { unsupported, structuralDiagnostics } = validateAgainstSource(ir, sourceDocument); + const structuralErrors = structuralDiagnostics.filter((diagnostic) => diagnostic.severity === 'error'); + if (structuralErrors.length > 0) { + throw new DocxMarkdocError( + 'STRUCTURAL_VALIDATION_FAILED', + 'Resolved operations would create an unsafe document structure.', + structuralErrors, + ); + } const declaredOperationIds = ir.operations.map((operation) => operation.operationId); const atomicPreflight = assessDraftCompleteness(ir, declaredOperationIds); const incompleteAtomicSets = atomicPreflight.changeSets.filter((set) => !set.complete); @@ -413,5 +442,5 @@ export async function compileMarkdoc( cleanText, acceptedText, }); - return { clean, tracked, ir, certificate }; + return { clean, tracked, ir, certificate, structuralDiagnostics }; } diff --git a/packages/docx-markdoc/src/docx-markdoc.test.ts b/packages/docx-markdoc/src/docx-markdoc.test.ts index aef9f1c8..ad13b121 100644 --- a/packages/docx-markdoc/src/docx-markdoc.test.ts +++ b/packages/docx-markdoc/src/docx-markdoc.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from 'vitest'; import JSZip from 'jszip'; import { buildDocxFromParts, buildSyntheticDocx, DocxDocument, parseXml } from '@usejunior/docx-core'; -import { compileMarkdoc } from './compile.js'; +import { compileMarkdoc, validateMarkdocAgainstSource } from './compile.js'; import { DocxMarkdocError } from './errors.js'; import { exportAdjacentRevisionPairs, exportEditPairs } from './export.js'; import { importDocxToMarkdoc } from './import.js'; @@ -85,6 +85,42 @@ describe('brownfield Markdoc authoring', () => { await expect(compileMarkdoc(other, imported.markdoc)).rejects.toMatchObject({ code: 'SOURCE_HASH_DRIFT' }); }); + it('rejects a parent-child slice before mutation and returns the last descendant', async () => { + const imported = await importDocxToMarkdoc(await structuralFixture()); + const [parent] = requireMarkdoc(imported.markdoc).scaffold; + if (!parent) throw new Error('fixture parent missing'); + const insertion = `${imported.markdoc}\n{% insert-after anchor="${parent.id}" operation="new-section" style-source="${parent.id}" %}\n{% after %}\nNew section.\n{% /after %}\n{% /insert-after %}\n`; + const validated = await validateMarkdocAgainstSource(imported.anchoredSource, insertion); + expect(validated.diagnostics).toContainEqual(expect.objectContaining({ + code: 'PARENT_CHILD_SLICE', suggested_anchor_id: validated.ir.scaffold[2]!.id, + })); + await expect(compileMarkdoc(imported.anchoredSource, insertion)).rejects.toMatchObject({ + code: 'STRUCTURAL_VALIDATION_FAILED', + }); + }); + + it('requires the NVCA-style Heading2 and HeadingPara2 pair and preserves their boundary', async () => { + const imported = await importDocxToMarkdoc(await bondedRunInFixture()); + const [headingPeer, bodyPeer, , , parent] = requireMarkdoc(imported.markdoc).scaffold; + if (!headingPeer || !bodyPeer || !parent) throw new Error('run-in fixture missing'); + const loneHeading = `${imported.markdoc}\n{% insert-after anchor="${parent.id}" operation="heading" style-source="${headingPeer.id}" %}\n{% after %}\n2.1 Additional Representation. The Company shall provide notice.\n{% /after %}\n{% /insert-after %}\n`; + await expect(compileMarkdoc(imported.anchoredSource, loneHeading)).rejects.toMatchObject({ + code: 'STRUCTURAL_VALIDATION_FAILED', + details: expect.arrayContaining([expect.objectContaining({ code: 'BONDED_PARAGRAPH_PAIR_REQUIRED' })]), + }); + + // Repeated AFTER insertions reverse at the same anchor, so body is applied + // first and heading second to produce heading → body in the clean document. + const paired = `${imported.markdoc}\n{% insert-after anchor="${parent.id}" operation="body" style-source="${bodyPeer.id}" %}\n{% after %}\nThe Company shall provide notice.\n{% /after %}\n{% /insert-after %}\n{% insert-after anchor="${parent.id}" operation="heading" style-source="${headingPeer.id}" %}\n{% after %}\n2.1 Additional Representation.\n{% /after %}\n{% /insert-after %}\n`; + const result = await compileMarkdoc(imported.anchoredSource, paired); + const clean = await DocxDocument.load(result.clean); + expect(clean.buildDocumentView().nodes.slice(-3).map((node) => [node.raw_text, node.paragraph_style_id])).toEqual([ + ['Parent section.', 'Heading1'], + ['2.1 Additional Representation.', 'Heading2'], + ['The Company shall provide notice.', 'HeadingPara2'], + ]); + }); + it('[SDX-MDOC-01] preserves source-significant boundary spaces and literal entities', async () => { const text = ' # Price * & value '; const original = await buildSyntheticDocx({ paragraphs: [text] }); @@ -485,6 +521,31 @@ async function numberedFixture(): Promise { }); } +async function structuralFixture(): Promise { + const paragraph = (style: string, text: string) => `${text}`; + return buildDocxFromParts({ + bodyXml: [paragraph('Heading1', 'Parent.'), paragraph('Heading2', 'Child.'), paragraph('Heading3', 'Grandchild.'), paragraph('Heading1', 'Sibling.')].join(''), + stylesXml: headingStylesXml(), + }); +} + +async function bondedRunInFixture(): Promise { + const paragraph = (style: string, text: string) => `${text}`; + return buildDocxFromParts({ + bodyXml: [ + paragraph('Heading2', 'Existing heading one.'), paragraph('HeadingPara2', 'Existing body one.'), + paragraph('Heading2', 'Existing heading two.'), paragraph('HeadingPara2', 'Existing body two.'), + paragraph('Heading1', 'Parent section.'), + ].join(''), + stylesXml: headingStylesXml(true), + }); +} + +function headingStylesXml(includeRunIn = false): string { + const style = (id: string, name: string, level?: number) => `${level == null ? '' : ``}`; + return `${style('Normal', 'Normal')}${style('Heading1', 'heading 1', 0)}${style('Heading2', 'heading 2', 1)}${style('Heading3', 'heading 3', 2)}${includeRunIn ? style('HeadingPara2', 'Heading Para 2') : ''}`; +} + async function numberingTopology(buffer: Buffer): Promise> { const zip = await JSZip.loadAsync(buffer); const xml = await zip.file('word/document.xml')!.async('string'); diff --git a/packages/docx-markdoc/src/types.ts b/packages/docx-markdoc/src/types.ts index f2c2938b..735ff906 100644 --- a/packages/docx-markdoc/src/types.ts +++ b/packages/docx-markdoc/src/types.ts @@ -101,6 +101,14 @@ export type ValidationResult = | { valid: true; ir: MarkdocEditIR } | { valid: false; issues: ValidationIssue[] }; +export type { + ResolvedInsertionContext, + StructuralDiagnostic, + StructuralDiagnosticEvidence, + StructuralDiagnosticSeverity, + StructuralValidator, +} from '@usejunior/docx-core'; + export type VerificationCertificate = { version: 1; sourceSha256Matches: boolean; @@ -152,6 +160,7 @@ export type CompileResult = { tracked: Buffer; ir: MarkdocEditIR; certificate: VerificationCertificate; + structuralDiagnostics: import('@usejunior/docx-core').StructuralDiagnostic[]; }; export type ImportResult = { diff --git a/packages/docx-mcp/docs/tool-reference.generated.md b/packages/docx-mcp/docs/tool-reference.generated.md index ff581899..87c0f935 100644 --- a/packages/docx-mcp/docs/tool-reference.generated.md +++ b/packages/docx-mcp/docs/tool-reference.generated.md @@ -75,7 +75,7 @@ Search paragraphs with regex. Use file_path for session-based search, file_paths ## `batch_edit` -Single-agent front door for applying multiple edit steps (replace_text, insert_paragraph) to a document in one call. Validates all steps first, rejects conflicts before applying anything, then executes valid steps sequentially. Accepts inline steps or a plan_file_path JSON array. Surface: revisionable — every applied step emits native OOXML tracked changes. +Single-agent front door for applying multiple edit steps (replace_text, insert_paragraph) to a document in one call. Validates all steps first, rejects conflicts before applying anything, then executes valid steps sequentially. Two source-proven bonded heading/body insertions may share an anchor and position when both declare the same bonded_pair_id and distinct style_source_id values; unrelated same-slot inserts remain conflicts. Accepts inline steps or a plan_file_path JSON array. Surface: revisionable — every applied step emits native OOXML tracked changes. - readOnly: `false` - destructive: `true` @@ -105,7 +105,7 @@ Replace text in a paragraph by provider paragraph id, preserving formatting wher ## `insert_paragraph` -Insert a paragraph before/after an anchor paragraph by paragraph id. Supports DOCX, ODT, and Google Docs. (ODT paragraph ids are positional and shift after insertion — re-read before further edits.) Surface: revisionable — DOCX insertions emit native OOXML tracked changes. +Insert a paragraph before/after an anchor paragraph by paragraph id. DOCX responses include structural_warnings when the placement may slice a hierarchy, mismatch list levels, or renumber a list. Supports DOCX, ODT, and Google Docs. (ODT paragraph ids are positional and shift after insertion — re-read before further edits.) Surface: revisionable — DOCX insertions emit native OOXML tracked changes. - readOnly: `false` - destructive: `true` diff --git a/packages/docx-mcp/src/add_typescript_mcp_server.test.ts b/packages/docx-mcp/src/add_typescript_mcp_server.test.ts index 962dd5d4..f5088ba1 100644 --- a/packages/docx-mcp/src/add_typescript_mcp_server.test.ts +++ b/packages/docx-mcp/src/add_typescript_mcp_server.test.ts @@ -290,6 +290,25 @@ describe('TypeScript MCP server behavior', () => { expect(String(inserted.new_paragraph_id)).toMatch(/^_bk_[0-9a-f]{12}$/); }); + humanReadableTest.openspec('Unsafe insertion returns corrective guidance')('Scenario: Unsafe insertion returns corrective guidance', async () => { + const paragraph = (level: number, text: string) => `${text}`; + const xml = `${paragraph(0, 'Parent.')}${paragraph(1, 'Child.')}${paragraph(2, 'Grandchild.')}${paragraph(0, 'Sibling.')}`; + const session = await openSession([], { xml }); + const inserted = await insertParagraph(session.mgr, { + file_path: session.inputPath, + positional_anchor_node_id: session.paraIds[0]!, + style_source_id: session.paraIds[0]!, + new_string: 'New parent.', + instruction: 'structural warning test', + position: 'AFTER', + }); + assertSuccess(inserted, 'insert'); + expect(inserted.structural_warnings).toContainEqual(expect.objectContaining({ + code: 'PARENT_CHILD_SLICE', + suggested_anchor_id: session.paraIds[2], + })); + }); + humanReadableTest.openspec('download tool')('Scenario: download tool', async () => { const mgr = createTestSessionManager(); const tmpDir = await createTrackedTempDir('safe-docx-download-tool-'); diff --git a/packages/docx-mcp/src/replace_plan_tools_with_batch_edit.test.ts b/packages/docx-mcp/src/replace_plan_tools_with_batch_edit.test.ts index 8366578e..92ac6f24 100644 --- a/packages/docx-mcp/src/replace_plan_tools_with_batch_edit.test.ts +++ b/packages/docx-mcp/src/replace_plan_tools_with_batch_edit.test.ts @@ -185,6 +185,38 @@ describe('Traceability: replace plan tools with batch_edit', () => { expect(String(read.content)).not.toContain('Second'); }); + test('permits exactly one source-proven bonded heading/body pair in a shared insert slot', async () => { + const paragraph = (style: string, level: number | null, text: string) => `${level == null ? '' : ``}${text}`; + const xml = `${paragraph('Heading2', 1, 'Heading one.')}${paragraph('HeadingPara2', null, 'Body one.')}${paragraph('Heading2', 1, 'Heading two.')}${paragraph('HeadingPara2', null, 'Body two.')}${paragraph('Heading1', 0, 'Anchor.')}`; + const opened = await openSession([], { xml }); + const result = await batchEdit(opened.mgr, { + file_path: opened.inputPath, + steps: [ + { step_id: 'body', operation: 'insert_paragraph', positional_anchor_node_id: opened.paraIds[4], style_source_id: opened.paraIds[1], bonded_pair_id: 'new-subsection', new_string: 'New body.', instruction: 'body half', position: 'AFTER' }, + { step_id: 'heading', operation: 'insert_paragraph', positional_anchor_node_id: opened.paraIds[4], style_source_id: opened.paraIds[0], bonded_pair_id: 'new-subsection', new_string: 'New heading.', instruction: 'heading half', position: 'AFTER' }, + ], + }); + assertSuccess(result, 'bonded batch'); + expect(result.completed_step_ids).toEqual(['body', 'heading']); + }); + + test('keeps an unrelated third insertion in a bonded slot as a hard collision', async () => { + const paragraph = (style: string, level: number | null, text: string) => `${level == null ? '' : ``}${text}`; + const xml = `${paragraph('Heading2', 1, 'Heading one.')}${paragraph('HeadingPara2', null, 'Body one.')}${paragraph('Heading2', 1, 'Heading two.')}${paragraph('HeadingPara2', null, 'Body two.')}${paragraph('Heading1', 0, 'Anchor.')}`; + const opened = await openSession([], { xml }); + const base = { operation: 'insert_paragraph', positional_anchor_node_id: opened.paraIds[4], position: 'AFTER', bonded_pair_id: 'new-subsection' }; + const result = await batchEdit(opened.mgr, { + file_path: opened.inputPath, + steps: [ + { ...base, step_id: 'body', style_source_id: opened.paraIds[1], new_string: 'New body.', instruction: 'body half' }, + { ...base, step_id: 'heading', style_source_id: opened.paraIds[0], new_string: 'New heading.', instruction: 'heading half' }, + { ...base, step_id: 'unrelated', style_source_id: opened.paraIds[1], new_string: 'Unrelated.', instruction: 'unrelated insert' }, + ], + }); + assertFailure(result, 'BATCH_CONFLICT'); + expect((result.conflicts as Array<{ code: string }>)).toContainEqual(expect.objectContaining({ code: 'INSERT_SLOT_COLLISION' })); + }); + humanReadableTest.openspec('batch_edit preserves run formatting on replace')('Scenario: batch_edit preserves run formatting on replace', async () => { const xml = `` + diff --git a/packages/docx-mcp/src/structural_validation_openspec.test.ts b/packages/docx-mcp/src/structural_validation_openspec.test.ts new file mode 100644 index 00000000..b3593902 --- /dev/null +++ b/packages/docx-mcp/src/structural_validation_openspec.test.ts @@ -0,0 +1,64 @@ +import { describe, expect } from 'vitest'; +import { isRecognizedBondedInsertionPair, validateStructuralInsertion, validateStructuralInsertions, type DocumentViewNode } from '@usejunior/docx-core'; +import { testAllure } from './testing/allure-test.js'; + +const TEST_FEATURE = 'add-markdoc-structural-validation'; +const scenario = testAllure.epic('Document Editing').withLabels({ feature: TEST_FEATURE }); + +function node(id: string, level: number | null, style?: string): DocumentViewNode { + const resolvedStyle = style ?? (level == null ? 'Normal' : `Heading${level}`); + return { + id, list_label: '', header: '', style: resolvedStyle, text: id, clean_text: id, tagged_text: id, + list_metadata: { list_level: level == null ? -1 : level - 1, label_type: null, label_string: '', header_text: null, header_style: null, header_formatting: null, is_auto_numbered: level != null }, + style_fingerprint: { list_level: level == null ? -1 : level - 1, left_indent_pt: 0, first_line_indent_pt: 0, style_name: resolvedStyle, alignment: 'LEFT' }, + paragraph_style_id: resolvedStyle, paragraph_style_name: resolvedStyle, paragraph_alignment: 'LEFT', + paragraph_indents_pt: { left: 0, first_line: 0 }, + numbering: { num_id: level == null ? null : '1', ilvl: level == null ? null : level - 1, is_auto_numbered: level != null }, + heading: level == null ? undefined : { text: id, source: 'word_style', level }, + header_formatting: null, body_run_formatting: null, + }; +} + +describe('OpenSpec traceability: add-markdoc-structural-validation', () => { + scenario.openspec('Parent-child slicing fails before mutation')('Scenario: Parent-child slicing fails before mutation', () => { + const diagnostics = validateStructuralInsertion([node('p', 1), node('c', 2), node('g', 3)], { + operationId: 'op', position: 'AFTER', anchorId: 'p', + }); + expect(diagnostics).toContainEqual(expect.objectContaining({ code: 'PARENT_CHILD_SLICE', severity: 'error', suggested_anchor_id: 'g' })); + }); + + scenario.openspec('Nested peer insertion is not misdiagnosed')('Scenario: Nested peer insertion is not misdiagnosed', () => { + const diagnostics = validateStructuralInsertion([node('p', 1), node('c', 2)], { + operationId: 'op', position: 'AFTER', anchorId: 'p', styleSourceId: 'c', + }); + expect(diagnostics.some((item) => item.code === 'PARENT_CHILD_SLICE')).toBe(false); + }); + + scenario.openspec('Validation output is actionable and stable')('Scenario: Validation output is actionable and stable', () => { + const [diagnostic] = validateStructuralInsertion([node('p', 1), node('c', 2)], { + operationId: 'op', position: 'AFTER', anchorId: 'p', + }); + expect(diagnostic).toMatchObject({ operation_id: 'op', anchor_id: 'p', evidence: { anchor_level: 1, intended_level: 1 } }); + }); + + scenario.openspec('Bonded run-in subsection requires two paragraphs')('Scenario: Bonded run-in subsection requires two paragraphs', () => { + const nodes = [node('h1', 2), node('b1', null, 'HeadingPara2'), node('h2', 2), node('b2', null, 'HeadingPara2'), node('p', 1)]; + expect(validateStructuralInsertions(nodes, [{ operationId: 'heading', position: 'AFTER', anchorId: 'p', styleSourceId: 'h1' }])) + .toContainEqual(expect.objectContaining({ code: 'BONDED_PARAGRAPH_PAIR_REQUIRED' })); + }); + + scenario.openspec('Unsafe insertion returns corrective guidance')('Scenario: Unsafe insertion returns corrective guidance', () => { + const [diagnostic] = validateStructuralInsertion([node('p', 1), node('c', 2)], { + operationId: 'insert_paragraph', position: 'AFTER', anchorId: 'p', + }); + expect(diagnostic?.suggested_anchor_id).toBe('c'); + }); + + scenario.openspec('Atomic bonded pair shares one insertion slot')('Scenario: Atomic bonded pair shares one insertion slot', () => { + const nodes = [node('h1', 2), node('b1', null, 'HeadingPara2'), node('h2', 2), node('b2', null, 'HeadingPara2'), node('p', 1)]; + expect(isRecognizedBondedInsertionPair(nodes, [ + { operationId: 'body', position: 'AFTER', anchorId: 'p', styleSourceId: 'b1' }, + { operationId: 'heading', position: 'AFTER', anchorId: 'p', styleSourceId: 'h1' }, + ])).toBe(true); + }); +}); diff --git a/packages/docx-mcp/src/tool_catalog.ts b/packages/docx-mcp/src/tool_catalog.ts index ab6fc90b..a64bb785 100644 --- a/packages/docx-mcp/src/tool_catalog.ts +++ b/packages/docx-mcp/src/tool_catalog.ts @@ -162,7 +162,7 @@ export const SAFE_DOCX_TOOL_CATALOG = [ name: 'batch_edit', surface: 'revisionable', description: - 'Single-agent front door for applying multiple edit steps (replace_text, insert_paragraph) to a document in one call. Validates all steps first, rejects conflicts before applying anything, then executes valid steps sequentially. Accepts inline steps or a plan_file_path JSON array. Surface: revisionable — every applied step emits native OOXML tracked changes.', + 'Single-agent front door for applying multiple edit steps (replace_text, insert_paragraph) to a document in one call. Validates all steps first, rejects conflicts before applying anything, then executes valid steps sequentially. Two source-proven bonded heading/body insertions may share an anchor and position when both declare the same bonded_pair_id and distinct style_source_id values; unrelated same-slot inserts remain conflicts. Accepts inline steps or a plan_file_path JSON array. Surface: revisionable — every applied step emits native OOXML tracked changes.', input: z.object({ ...FILE_FIELD, steps: z @@ -202,7 +202,7 @@ export const SAFE_DOCX_TOOL_CATALOG = [ { name: 'insert_paragraph', surface: 'revisionable', - description: 'Insert a paragraph before/after an anchor paragraph by paragraph id. Supports DOCX, ODT, and Google Docs. (ODT paragraph ids are positional and shift after insertion — re-read before further edits.) Surface: revisionable — DOCX insertions emit native OOXML tracked changes.', + description: 'Insert a paragraph before/after an anchor paragraph by paragraph id. DOCX responses include structural_warnings when the placement may slice a hierarchy, mismatch list levels, or renumber a list. Supports DOCX, ODT, and Google Docs. (ODT paragraph ids are positional and shift after insertion — re-read before further edits.) Surface: revisionable — DOCX insertions emit native OOXML tracked changes.', input: z.object({ ...FILE_FIELD_OPTIONAL, ...GOOGLE_DOC_ID_FIELD, diff --git a/packages/docx-mcp/src/tools/batch_edit.ts b/packages/docx-mcp/src/tools/batch_edit.ts index 3cdc4888..b0a4a53f 100644 --- a/packages/docx-mcp/src/tools/batch_edit.ts +++ b/packages/docx-mcp/src/tools/batch_edit.ts @@ -4,6 +4,7 @@ import { SafeDocxError, findUniqueSubstringMatch, replaceParagraphTextRange, + isRecognizedBondedInsertionPair, type RevisionContext, } from '@usejunior/docx-core'; import { SessionManager, getRevisionContextForSession } from '../session/manager.js'; @@ -29,6 +30,7 @@ const INSERT_PARAGRAPH_FIELDS = new Set([ 'instruction', 'position', 'style_source_id', + 'bonded_pair_id', ]); const SUPPORTED_OPERATIONS = new Set(['replace_text', 'insert_paragraph']); @@ -77,6 +79,8 @@ type ConflictStep = { target_paragraph_id?: string; positional_anchor_node_id?: string; position?: 'BEFORE' | 'AFTER'; + style_source_id?: string; + bonded_pair_id?: string; range?: { start: number; end: number }; }; @@ -206,6 +210,10 @@ function validateSteps( if (pos !== undefined && pos !== 'BEFORE' && pos !== 'AFTER') { validation.errors.push(`Invalid position '${String(pos)}'. Must be 'BEFORE' or 'AFTER'.`); } + const bondedPairId = step.fields.bonded_pair_id; + if (bondedPairId !== undefined && (typeof bondedPairId !== 'string' || !bondedPairId.trim())) { + validation.errors.push('bonded_pair_id must be a non-empty string when provided.'); + } } if (validation.errors.length > 0) validation.valid = false; @@ -329,7 +337,7 @@ function detectReplaceConflicts(steps: ConflictStep[]): Conflict[] { return conflicts; } -function detectInsertSlotCollisions(steps: ConflictStep[]): Conflict[] { +function detectInsertSlotCollisions(steps: ConflictStep[], allowedBondedSlots = new Set()): Conflict[] { const insertSteps = steps.filter( (s) => s.operation === 'insert_paragraph' && !!s.positional_anchor_node_id && !!s.position, ); @@ -344,6 +352,7 @@ function detectInsertSlotCollisions(steps: ConflictStep[]): Conflict[] { const conflicts: Conflict[] = []; for (const [slotKey, slotSteps] of bySlot.entries()) { if (slotSteps.length < 2) continue; + if (slotSteps.length === 2 && allowedBondedSlots.has(slotKey)) continue; const anchorId = slotSteps[0]!.positional_anchor_node_id!; const position = slotSteps[0]!.position!; conflicts.push({ @@ -383,10 +392,33 @@ function buildConflictView(steps: NormalizedStep[]): ConflictStep[] { source_step_index: index, positional_anchor_node_id: step.fields.positional_anchor_node_id as string | undefined, position: (step.fields.position as 'BEFORE' | 'AFTER' | undefined) ?? 'AFTER', + style_source_id: step.fields.style_source_id as string | undefined, + bonded_pair_id: step.fields.bonded_pair_id as string | undefined, }; }); } +function recognizedBondedSlots(steps: ConflictStep[], doc: DocxDocument): Set { + const result = new Set(); + const inserts = steps.filter((step) => step.operation === 'insert_paragraph' && step.bonded_pair_id); + const groups = new Map(); + for (const step of inserts) groups.set(step.bonded_pair_id!, [...(groups.get(step.bonded_pair_id!) ?? []), step]); + const nodes = doc.buildDocumentView({ includeSemanticTags: false, showFormatting: true }).nodes; + for (const group of groups.values()) { + if (group.length !== 2 || group.some((step) => !step.style_source_id || !step.positional_anchor_node_id || !step.position)) continue; + const contexts = group.map((step) => ({ + operationId: step.step_id, + position: step.position!, + anchorId: step.positional_anchor_node_id!, + styleSourceId: step.style_source_id, + })); + if (isRecognizedBondedInsertionPair(nodes, contexts)) { + result.add(`${group[0]!.positional_anchor_node_id}::${group[0]!.position}`); + } + } + return result; +} + async function executeSteps( manager: SessionManager, filePath: string, @@ -546,7 +578,7 @@ export async function batchEdit( const conflicts = [ ...detectDuplicateStepIdConflicts(conflictSteps), ...detectReplaceConflicts(conflictSteps), - ...detectInsertSlotCollisions(conflictSteps), + ...detectInsertSlotCollisions(conflictSteps, recognizedBondedSlots(conflictSteps, session.doc)), ]; if (conflicts.length > 0) { return { diff --git a/packages/docx-mcp/src/tools/insert_paragraph.ts b/packages/docx-mcp/src/tools/insert_paragraph.ts index 68fb8e0c..96620b9f 100644 --- a/packages/docx-mcp/src/tools/insert_paragraph.ts +++ b/packages/docx-mcp/src/tools/insert_paragraph.ts @@ -6,6 +6,7 @@ import { stripHyperlinkTags, stripAllInlineTags, type ReplacementPart, + validateStructuralInsertions, } from '@usejunior/docx-core'; import { SessionManager, getRevisionContextForSession } from '../session/manager.js'; import { errorMessage } from "../error_utils.js"; @@ -166,6 +167,16 @@ export async function insertParagraph( } } + const structuralWarnings = validateStructuralInsertions( + session.doc.buildDocumentView({ includeSemanticTags: false, showFormatting: true }).nodes, + [{ + operationId: 'insert_paragraph', + position: positionUpper as 'BEFORE' | 'AFTER', + anchorId: params.positional_anchor_node_id, + styleSourceId, + }], + ); + let inputText = params.new_string; if (hasHyperlinkTags(inputText)) inputText = stripHyperlinkTags(inputText); @@ -231,6 +242,7 @@ export async function insertParagraph( position: positionUpper, inserted_text: previewText(plainParagraphs.join('\n\n'), RESULT_PREVIEW_CHARS), }; + if (structuralWarnings.length > 0) responseData.structural_warnings = structuralWarnings; if (res.styleSourceFallback) { responseData.style_source_warning = `style_source_id '${params.style_source_id}' not found; fell back to anchor paragraph formatting.`; }