feat(document-api): create named paragraph and character styles - #3976
feat(document-api): create named paragraph and character styles#3976Nathaniel-260 wants to merge 4 commits into
Conversation
Nothing in the Document API brings a named style into existence. styles.apply
writes w:docDefaults and is validated as such (target.scope must be
"docDefaults"); styles.paragraph.setStyle and setStyleRef apply a style that is
already in the document, by styleId or by one of four semantic roles;
styles.getCatalog reads. So a caller who wants a "Question" or a "Quote" style
has one route left: synthesize a whole .docx and hand it to templates.apply,
whose input is { source, bodyPolicy } with no scope selector โ which parts of
the document it also adopts is then decided by the shape of the synthesized
package rather than by the caller's request.
styles.create is the missing half of styles.getCatalog: what the catalogue can
describe, this can author.
Contract only. The adapter that writes word/styles.xml is not in this
repository, so the operation reports CAPABILITY_UNAVAILABLE until the engine
side lands โ the same way styles.getCatalog fails closed when its optional
catalogue hook is absent. Two consequences are deliberate and worth stating
rather than discovering: possibleFailureCodes is NONE_FAILURES, because with no
adapter there is no code a host can currently produce and the contract must not
over-declare; and a merged contract is advertised by capabilities.get() before
it can succeed, which is a decision for the maintainers, not a side effect to
be discovered after merge.
Design notes, each of which had a plausible alternative:
- Named .create, not .define. The create.* namespace is body content, but the
.create leaf is already how this codebase names a durable object that lives
in an auxiliary part and is referenced from the body โ lists.create writes a
numbering definition into word/numbering.xml, which is structurally the same
operation on a different part.
- The input is a discriminated union rather than one flat interface with
cross-field checks, matching StylesApplyInput and ListsCreateInput. A
character style cannot carry next or paragraph properties, and that is now a
compile error rather than a runtime one.
- Fields are named id and priority, not styleId and uiPriority, so that what
StyleCatalogItem reads back is what this writes. priority stays number | null
and is not capped at Word's 0..99 UI band, because the catalogue can return
values outside it and a cap here would break the round trip.
- The exclusion list is now a property of the destination. EXCLUDED_KEYS was
the docDefaults list and the only list, so every caller inherited a
restriction only one of them was subject to. Its four run entries that Word
forbids in docDefaults and allows on a named style โ w:cs, w:highlight,
w:oMath, w:rtl โ join the registry and are reachable under the new style
scope alone. w:rtl is the property that makes a run right-to-left, so
without this split no right-to-left style could be authored through this
API at all. Each of the four is already read back off a w:style by
StyleDefinition.runProperties in the style engine.
styles.apply is unchanged in every observable way, and two of those ways took
a fix to keep. buildStateSchema is scoped as well as buildPatchSchema, or the
four keys would have landed in the published before/after maps of a
docDefaults receipt โ the exact keys that scope rejects, in objects carrying
additionalProperties: false, so receipt validation would have loosened too.
And classifyPatchKey now asks whether a key is reachable on the other channel
*in this scope*: putting the four in ALLOWED_KEYS_BY_CHANNEL.run meant a run
property sent to the paragraph channel started answering cross_channel
instead of excluded, dropping the excluded_docdefaults_key reason a caller
branches on. Both are covered by tests that fail without the fix.
What remains: same accepted keys, same rejection messages, same detail codes,
same published input and output schemas.
- before/after are per channel, in the published schema as well as in the
types. styles.apply can use a flat map because resolution.channel says which
channel it describes; one w:style carries both at once, and snapToGrid,
shading and borders exist on both โ borders with genuinely different shapes
(w:bdr, one border, against w:pBdr, six edges). buildStateSchema therefore
takes an optional channel: without it the receipt schema would have folded
the two back together and accepted rtl under `after.paragraph`, which is the
exact confusion the split exists to prevent.
- The one alias rule that is not enforced: an alias equal to the style's own
name. It is redundant rather than corrupting, and JSON Schema cannot compare
sibling fields โ so the rule would live only in the validator, and a caller
pre-validating against the published contract would get a green light and
then a throw. The comma rule, which does corrupt, is enforced in both and
expressed as a pattern.
- No new failure codes. DUPLICATE_ID, PRECONDITION_FAILED, LOCK_VIOLATION and
STYLE_CONFLICT already cover every case; STYLE_CONFLICT in particular was
unclaimed and is exactly the name collision this operation has to report.
The published failure schema types `code` as a string rather than an enum
derived from possibleFailureCodes: that list is empty while the operation
ships without an adapter, and an empty enum does not compile in a
2020-12 validator โ it would have taken the whole output oneOf with it, so a
consumer could not have validated even a success receipt. styles.apply
publishes the same open shape.
- idempotency is conditional, not idempotent. Under the default
conflictPolicy 'fail' a second identical call fails; only 'replace' makes it
repeatable. The value is published to the reference page and the agent
artifacts, so declaring it idempotent would invite an orchestrator to replay
the call after a transport timeout and take a hard conflict โ or, under
replace, silently clobber a style someone edited in between.
- highlight is the closed ST_HighlightColor enumeration, not a free string.
A free string writes an invalid w:highlight into styles.xml, which Word
reports as a damaged document rather than as a rejected call. The token list
moves to inline-semantics/token-sets.ts beside the other ST_* sets, and
SDHighlightColor is now derived from it so the two cannot drift.
- An empty run: {} or paragraph: {} is rejected, matching styles.apply and the
published minProperties: 1. Omitting the channel is how you say "no
properties"; an empty object asked the adapter to write an empty w:rPr and
disagreed with the contract a caller may pre-validate against.
- conflictPolicy is decided on both id and name. Word keys its Styles gallery
on w:name: two styles with distinct ids and one name are two identically
labelled entries, and a name that collides with a latent style is resolved
by Word against w:latentStyles, which can inherit w:semiHidden and leave a
successful call with an invisible style.
Out of scope for this first version, and deliberately: merge semantics, since
the patch types cannot express removal and the registry attaches a per-property
merge strategy on a second axis; linked pairs, since each half names the other
and one call cannot satisfy the first; table and numbering styles, which have
no patch surface here; and w:default, which is a singleton per type.
Tests: validation and routing in styles/create.test.ts, scope filtering of both
schema builders in styles/schema.test.ts, direct coverage of the public
classifyPatchKey export, an invoke parity case, and a consumer-typecheck
fixture asserting both the parameter and the return shape. The SD-2018 registry
gate pins each scope separately. Every branch was checked by mutation: each
test was confirmed to fail against the corresponding broken code, not merely to
pass against the working code.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 19 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid โ if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/document-api/src/contract/schemas.ts">
<violation number="1" location="packages/document-api/src/contract/schemas.ts:5169">
P3: The published schema hardcodes `xmlPath: { const: 'w:styles/w:style' }`, duplicating the `STYLE_XML_PATH` constant exported from create.ts and pinned as the `StylesCreateResolution.xmlPath` type. If the constant is ever updated, the schema will silently drift from the type and the resolution contract. Since schemas.ts already imports from '../styles/index.js' (which re-exports `STYLE_XML_PATH`), reference the constant here instead so the schema and the resolution type cannot diverge.</violation>
</file>
<file name="packages/document-api/src/contract/operation-definitions.ts">
<violation number="1" location="packages/document-api/src/contract/operation-definitions.ts:1290">
P2: `styles.create` returns a `StylesCreateReceipt`, but its command metadata omits `returnsReceipt: true`; metadata consumers cannot identify the result as a receipt envelope. Add the receipt marker to this operation definition.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| throws: ['INVALID_INPUT', 'CAPABILITY_UNAVAILABLE', 'REVISION_MISMATCH'], | ||
| // Writes word/styles.xml outside the document history, exactly as | ||
| // styles.apply does. | ||
| historyUnsafe: true, |
There was a problem hiding this comment.
P2: styles.create returns a StylesCreateReceipt, but its command metadata omits returnsReceipt: true; metadata consumers cannot identify the result as a receipt envelope. Add the receipt marker to this operation definition.
Prompt for AI agents
Check if this issue is valid โ if so, understand the root cause and fix it. At packages/document-api/src/contract/operation-definitions.ts, line 1290:
<comment>`styles.create` returns a `StylesCreateReceipt`, but its command metadata omits `returnsReceipt: true`; metadata consumers cannot identify the result as a receipt envelope. Add the receipt marker to this operation definition.</comment>
<file context>
@@ -1264,6 +1264,34 @@ export const OPERATION_DEFINITIONS = {
+ throws: ['INVALID_INPUT', 'CAPABILITY_UNAVAILABLE', 'REVISION_MISMATCH'],
+ // Writes word/styles.xml outside the document history, exactly as
+ // styles.apply does.
+ historyUnsafe: true,
+ }),
+ referenceDocPath: 'styles/create.mdx',
</file context>
| historyUnsafe: true, | |
| historyUnsafe: true, | |
| returnsReceipt: true, |
| id: { type: 'string', minLength: 1 }, | ||
| type: { enum: ['paragraph', 'character'] }, | ||
| xmlPart: { type: 'string' }, | ||
| xmlPath: { const: 'w:styles/w:style' }, |
There was a problem hiding this comment.
P3: The published schema hardcodes xmlPath: { const: 'w:styles/w:style' }, duplicating the STYLE_XML_PATH constant exported from create.ts and pinned as the StylesCreateResolution.xmlPath type. If the constant is ever updated, the schema will silently drift from the type and the resolution contract. Since schemas.ts already imports from '../styles/index.js' (which re-exports STYLE_XML_PATH), reference the constant here instead so the schema and the resolution type cannot diverge.
Prompt for AI agents
Check if this issue is valid โ if so, understand the root cause and fix it. At packages/document-api/src/contract/schemas.ts, line 5169:
<comment>The published schema hardcodes `xmlPath: { const: 'w:styles/w:style' }`, duplicating the `STYLE_XML_PATH` constant exported from create.ts and pinned as the `StylesCreateResolution.xmlPath` type. If the constant is ever updated, the schema will silently drift from the type and the resolution contract. Since schemas.ts already imports from '../styles/index.js' (which re-exports `STYLE_XML_PATH`), reference the constant here instead so the schema and the resolution type cannot diverge.</comment>
<file context>
@@ -5122,6 +5122,101 @@ const operationSchemas: Record<OperationId, OperationSchemaSet> = {
+ id: { type: 'string', minLength: 1 },
+ type: { enum: ['paragraph', 'character'] },
+ xmlPart: { type: 'string' },
+ xmlPath: { const: 'w:styles/w:style' },
+ },
+ ['scope', 'id', 'type', 'xmlPart', 'xmlPath'],
</file context>
Review found that capabilities.get() reported the operation available on a
host that cannot run it. createDocumentApi already gates operations on adapter
presence -- ADAPTER_GATED_PREFIXES marks a whole namespace unavailable with
NAMESPACE_UNAVAILABLE -- but it could not reach this one: adapters.styles is
present, and only its optional create hook is missing.
A second gate now runs at hook granularity with the existing
OPERATION_UNAVAILABLE reason code. It is scoped to the operation this branch
adds. styles.getCatalog and capabilities.check have the same shape, but
changing what an already-shipped operation advertises is a separate change,
not a side effect of this one.
Two details are load-bearing and pinned by tests. The check reads
adapters.styles?.create, because the namespace loop above already tolerates a
missing adapter and a JavaScript host can pass one. And an operation the
engine's snapshot does not mention is left alone rather than invented: an
absent entry already says unavailable, and adding one would claim the engine
reported something it did not.
This also corrects the branch's own claim that a merged contract would be
advertised before it can succeed. It no longer is. What remains is that a
feat: release publishes the operation into the schemas, the agent artifacts
and the reference site regardless, and OperationDefinitionEntry still has no
status field -- the runtime can now say "not implemented here", the contract
cannot.
Also from the review:
- The ST_HighlightColor block had landed between the ST_ThemeColor section
header and its doc comment, orphaning both. Moved below
ST_THEME_COLOR_VALUE_SET, and the set carries the runtime-set doc comment
its neighbours have.
- schemas.ts keeps its xmlPath literal rather than referencing
STYLE_XML_PATH, and gains a test instead. A published wire format that
follows a constant changes whenever the constant moves, silently, which is
the failure it most needs to avoid; styles.apply spells out its own two
paths for the same reason. What a schema must not do is drift from the
type, so the test asserts const === STYLE_XML_PATH, and the same assertion
is added for styles.apply against XML_PATH_BY_CHANNEL.
- returnsReceipt is deliberately still unset. The field marks a result that
follows the shared ReceiptSuccess/ReceiptFailure envelope, which every one
of the sixteen operations carrying it does -- TextMutationReceipt is
(ReceiptSuccess & { resolution }) | (ReceiptFailureResult & { resolution }).
StylesCreateReceiptSuccess deliberately does not extend ReceiptSuccess,
because inserted/updated take EntityAddress and a style definition is not
addressable as a document entity. 93 receipt-returning operations omit the
marker, styles.apply among them.
And from CI rather than review: apps/docs/config/routes.json was missing
/document-api/reference/styles/create/, which failed Docs check:redirects.
The generator discovers routes by scanning the built export, so producing it
the usual way needs a full docs build; the manifest is append-only and
written as a sorted union through JSON.stringify with two-space indent, so
the entry was produced through that same serialisation and verified
byte-identical to it rather than placed by hand. The docs redirect suite
passes, 32 of 32.
Seven mutations, each confirmed to fail the corresponding test.
|
Thanks โ four findings, two fixed, one fixed differently, one I want to push back on with evidence.
for (const { operationId, hasHook } of HOOK_GATED_OPERATIONS) {
if (hasHook(adapters)) continue;
const cap = caps.operations[operationId];
if (!cap) continue; // an engine predating the operation reports no entry at all
cap.available = false;
cap.tracked = false;
cap.dryRun = false;
cap.reasons = [...(cap.reasons ?? []), 'OPERATION_UNAVAILABLE'];
}Scoped to the operation this PR adds. Other optional hooks ( Two details the tests pin: This also corrects the PR description, which said a merged contract would be advertised before it can succeed. With the gate it is not โ that objection is now smaller than I wrote it, and the description is updated.
The convention bears it out: 93 operations whose Happy to be overruled if the field means something broader than its doc says. Also in this push, from CI rather than review: |
apps/docs/tests/export.test.mjs pins the reference landing copy, and the page renders the count from the contract -- Object.keys(model.operations).length in components/document-api-reference/index.tsx. Adding an operation moves it from 427 to 428, so the assertion has to move with it. This is the third CI failure on this branch and all three were the same mistake, not three mistakes: every one of them lived in the Docs job, and every one of them was invisible without running that job. The document-api gates I did run locally passed on all three pushes, which is exactly why they were reassuring and wrong -- what they measured was "the gates I know how to run pass", not "the branch is green". Five of the fourteen Docs steps read apps/docs/out and therefore need a full docs build: check:links, check:redirects, test:export, test:migration-agent-prompt, and the build itself, which writes _redirects. All three failures were in that group. test:migration-agent-prompt is the one worth naming: nothing about its name says it reads build output, and run without a build it fails with ENOENT on out/md/**, which reads as a regression rather than a missing precondition. The manifest entry in the previous commit and this count are both facts about the operation set that live in committed files, so adding an operation to this package is not a packages/document-api change alone.
|
Three red runs on this branch, all the same mistake rather than three of them โ worth a note since the history looks flaky otherwise. Every failure was in the The two facts that had to change, both in committed files, neither in
I have now run the whole
One unrelated thing I noticed and could not un-notice. |
|
The three jobs red on this run are not, and I think they will be red on every PR until a release stamp lands, so flagging it rather than sitting on it.
What I can see from this repo:
So the requirement landed one commit after the engine it needs was pinned, and a stamp to I have deliberately not bumped the pin here. That is release automation's job and doing it in a feature PR would just collide with the next stamp. Say the word if you would rather I did. For what it is worth on this branch: |
1 participant
Relates to #3975
What changed
styles.createโ define or redefine a named paragraph or character style. It isthe missing half of
styles.getCatalog: what the catalogue can describe, thiscan author. #3975 has the full argument for why the gap exists and why
templates.applyis the wrong tool for it; this PR is the contract.Additive, 20 files. New operation in
packages/document-api/src/styles/create.ts, registered the way every otheroperation is:
operation-definitions.ts(viamutationOperation()),operation-registry.ts, hand-written schemas incontract/schemas.ts,invoke/invoke.ts, and the four sites insrc/index.ts. Nothing generated iscommitted.
The decisions worth arguing with are in the commit message; the two that touch
existing code are below.
The exclusion list is now a property of the destination
EXCLUDED_KEYSwas thedocDefaultsexclusion list and the only list, soevery caller inherited a restriction only one of them is subject to. Four of its
run entries โ
w:cs,w:highlight,w:oMath,w:rtlโ are disallowed inw:docDefaultsand legal on a namedw:style; the header comment inregistry.tsalready says so ("intentionally disallowed in WorddocDefaults"), and all four are read back off a
w:stylebyStyleDefinition.runPropertiesin@superdoc/style-engine.w:rtlis the property that makes a run right-to-left. Without the split, noright-to-left style is expressible through this API โ the operation would ship
unable to author a style for Hebrew, Arabic, Persian or Urdu.
So
EXCLUDED_KEYS_BY_SCOPEkeys the list by'docDefaults' | 'style', thedocDefaultslist is byte-for-byte what it was, and the four new registryentries are reachable under the style scope alone.
styles.applyis unchanged, and two things had to be fixed to keep it that wayBoth were caught by tests written for this PR, and both fail without the fix:
buildStateSchemais scoped as well asbuildPatchSchema. Otherwise thefour keys land in the published
before/aftermaps of adocDefaultsreceipt โ the exact keys that scope rejects, in objects carrying
additionalProperties: false, so receipt validation would have loosened too.classifyPatchKeyasks whether a key is reachable on the other channelin this scope. Putting the four in
ALLOWED_KEYS_BY_CHANNEL.runmeant arun property sent to the paragraph channel started answering
cross_channelinstead of
excluded, dropping theexcluded_docdefaults_keyreason acaller branches on.
Verified rather than asserted. Generating
document-api-contract.jsonon thisbranch and on the merge-base and deep-diffing per operation:
styles.createcapabilities.getonly, and only because it enumerates operation idsstyles.*inherited from main, changedstyles.applyis byte-identical. Same accepted keys, same rejection messages,same detail codes, same published input and output schemas.
Two more gates run against the generated contract:
styles.createschemas compile in a 2020-12 validator, with nodangling
$ref. This is why the failure schema typescodeas a stringrather than an enum derived from
possibleFailureCodes: that list is emptywhile the operation ships without an adapter, and
enum: []does not compileโ it takes the whole output
oneOfwith it, so a consumer could not validateeven a success receipt.
styles.applypublishes the same open shape.schema side by side: 0 disagreements. That gate is why one rule is not
enforced โ an alias equal to the style's own name. It is redundant rather than
corrupting, and JSON Schema cannot compare sibling fields, so the rule would
live only in the validator and a caller pre-validating against the published
contract would get a green light and then a throw. The comma rule, which does
corrupt
w:aliases, is enforced on both sides and expressed as apattern.What is not in this PR
The adapter. The mutation lives in
@superdoc/docx-engine, which is not inthis repository, so
executeStylesCreatethrowsCAPABILITY_UNAVAILABLEuntilthe engine side lands โ the same way
styles.getCatalogfails closed when itsoptional hook is absent.
Two consequences are deliberate, and I would rather state them than have them
discovered after merge:
possibleFailureCodesisNONE_FAILURES, because with no adapter there is nocode a host can currently produce and the contract must not over-declare.
capabilities.get()reports it unavailable while the hook is absent. Anearlier revision of this description said the opposite, and the review was
right to catch it:
createDocumentApialready post-processes the enginesnapshot through
ADAPTER_GATED_PREFIXES, which could not reach thisoperation because
adapters.stylesis present and only its optionalcreatehook is not. There is now a second gate at hook granularity, using the
existing
OPERATION_UNAVAILABLEreason code. So the operation is notadvertised as available before it can succeed.
What remains is smaller, and still yours to decide: a
feat:release publishesthe operation into the JSON schemas, the agent artifacts and the reference site,
and
OperationDefinitionEntryhas nostatus/plannedfield, so thecontract cannot say "defined, not yet implemented" even though the runtime
now can. That is the question at the bottom of #3975. Please do not merge this
until it is answered. If you would rather implement contract and adapter
together and drop this, say so and I will close it; the shape is yours to use.
Review round
Four findings from the automated review; three changed the code.
to fail against the broken code.
token-sets.tsโ the newST_HighlightColorblock had landed between theST_ThemeColorheader and its doc comment, orphaning both. Moved belowST_THEME_COLOR_VALUE_SET.xmlPathinschemas.tsโ the drift risk is real, the fix is atest rather than a reference.
schemas.tsis a hand-written wire format andstyles.applyspells out its own paths two entries above; a schema thatfollows a constant changes the published contract silently whenever the
constant moves. The literal stays and a test pins it to
STYLE_XML_PATH, withthe same assertion added for
styles.applyagainstXML_PATH_BY_CHANNEL.returnsReceipt: trueโ not added, and I think this one is mistaken. Thefield's doc says it marks a result that "follows the Receipt success/failure
envelope", and every one of the 16 operations that sets it does,
TextMutationReceiptincluded.
StylesCreateReceiptSuccessdeliberately does not extendReceiptSuccess, becauseinserted/updatedtakeEntityAddressand astyle definition is not a document entity. 93 receipt-returning operations
omit the marker,
styles.applyamong them. Happy to be overruled.CI also failed on
Docs โ check:redirects:apps/docs/config/routes.jsonwasmissing
/document-api/reference/styles/create/. Added in the exact formgenerateRouteManifestwrites โ sorted union, two-space JSON, trailing newline โand verified byte-identical to that serialisation rather than hand-placed.
Checks
pnpm run <script>does not run on Windows in this repo (the rootpreparehook is POSIX, and
scripts/test.mjscallsspawnSync('pnpm.cmd')withoutshell: true), so each stage was invoked directly. CI runs the real scripts.tsc -b packages/document-apiโ cleanbun test packages/document-api/srcโ 1807 pass, 1808 run. The one failure,executeParagraphsSetTabStop > rejects clear as a setTabStop alignment, failsidentically on the merge-base and is untouched by this branch
check-contract-parityโ 428 operations, 428 API memberscheck-contract-outputsโ 5 generated filescheck-agent-artifactsโ 3 filescheck-stable-schemasโ 2 filescheck-documented-operationsโ 428 operations, 468 pagesvp fmt --checkon all 20 files โ cleanvp linton all 20 files โ 0 errors; the 24 warnings are pre-existing inindex.tsandschemas.tsand name no symbol this PR addstests/consumer-typecheck/src/document-api-styles-create.tsโ new fixture,asserting both the parameter and the return shapes; compiles clean against the
built
.d.tsunder the fixture's own strict settingsTests: validation and routing in
styles/create.test.ts, scope filtering ofboth schema builders in
styles/schema.test.ts, direct coverage of the publicclassifyPatchKeyexport, aninvokeparity case, and the SD-2018 registrygate pinned per scope. Every branch was checked by mutation โ each test was
confirmed to fail against the corresponding broken code, not merely to pass
against the working code.